Re: PATCH: Add x32 support to dynamic linker audit

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

On Wed, Mar 21, 2012 at 11:23 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> 2012-03-20 ?H.J. Lu ?<hongjiu.lu@intel.com>
>>>
>>> ? ? ? ?* elf/tst-auditmod1.c: Support la_x32_gnu_pltenter and
>>> ? ? ? ?la_x32_gnu_pltexit.
>>> ? ? ? ?(pltexit): Cast int_retval to ptrdiff_t.
>>
>> Why are you casting this to ptrdiff_t?
>>
>> The value of int_retval is actually lrv_eax which is uint32_t.
>
> X32 return value is lrv_rax, not lrv_eax.

You are correct, I read the wrong structure, it's a uint64_t then?

>> Did you choose ptrdiff_t because it *becomes* the right size and
>> signedness for all targets?
>
> No, it is because %t for printf and %t is for ptrdiff_t.

Why isn't this a %ld or %lx with a cast to uint64_t?

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 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.

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.

Cheers,
Carlos.