Re: PATCH: Add x32 support to dynamic linker audit

["Carlos O'Donell" <carlos at systemhalted dot org>]

On Wed, Mar 21, 2012 at 5:12 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> The meaning of ptrdiff_t is very specific.
>>
>> I know that I'm outside of your original patch and that the code
>> in question is not code specifically related to x32.
>>
>> Do you see anything wrong with changing all of these %t's to
>> %lx and cast to uint64_t?
>
> I don't see anything wrong with that. I tried to minimize my
> changes.

Please clean this up to avoid using %t and ptrdiff_t.

We should be printing variants of %x.

>>>> I would have expected uintptr_t to be used.
>>>>
>>>>> ? ? ? ?* elf/tst-auditmod3b.c: Likewise.
>>>>> ? ? ? ?* elf/tst-auditmod4b.c: Likewise.
>>>>> ? ? ? ?* elf/tst-auditmod5b.c: Likewise.
>>>>> ? ? ? ?* elf/tst-auditmod6b.c: Likewise.
>>>>> ? ? ? ?* elf/tst-auditmod6c.c: Likewise.
>>>>> ? ? ? ?* elf/tst-auditmod7b.c: Likewise.
>>>>
>>>> You don't need to modify the other tests also?
>>>>
>>>> e.g. tst-auditmod1.c
>>>
>>> Change for tst-auditmod1.c is in my patch. ?Other tests are OK.
>>
>> Is in which patch? I don't see any changes to tst-auditmod1.c in *this* patch.
>
> http://sourceware.org/ml/libc-alpha/2012-03/msg00786.html
> Did I miss something?

No, apparently I'm blind.

>> In tst-auditmod1.c we still have outregs->int_retval printed via %t.
>>
>>>> It seems odd to me that you wouldn't just redefine a set of
>>>> La_x32_regs and La_x32_retval for the sake of consistency. I guess the
>>>> argument can be made that it *is* the same as the x86-64 version, but
>>>> that doesn't make it any easier to document or tell users that in this
>>>> case you use function name X and arguments named Y.
>>>>
>>>> My preference would be to define an La_x32_regs and La_x32_retval for
>>>> use everywhere (even if they are just aliases to the x86-64 versions).
>>>>
>>>
>>> Will
>>>
>>> #define La_x32_regs La_x86_64_regs
>>> #define ?La_x32_retval ?La_x86_64_retval
>>>
>>> work for you?
>>
>> Yes, as long as the names are consistent with the function being
>> called e.g. La_x32_regs is used with la_gnu_x32_pltenter and
>> la_gnu_x32_pltexit.
>>
>
> I will make the change.

Thanks!

I'll give your change a final review once I see the patch.

Cheers,
Carlos.