Re: PATCH: Add x32 support to dynamic linker audit

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Wed, Mar 21, 2012 at 7:05 AM, Carlos O'Donell
<carlos@systemhalted.org> wrote:
> On Tue, Mar 20, 2012 at 12:54 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:
>> Hi,
>>
>> This patch adds x32 support to dynamic linker audit. It checks __LP64__ and
>> __x86_64__ for x32 support. ?It casts value to ptrdiff_t for printf %t.
>> OK to install?
>>
>> Thanks.
>>
>>
>> H.J.
>> 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.

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

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

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

Thanks.

-- 
H.J.