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 1:24 PM, Carlos O'Donell
<carlos@systemhalted.org> wrote:
> 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?

Yes.

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

I don't know.

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

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

From

http://sourceware.org/ml/libc-alpha/2012-03/msg00786.html

---
diff --git a/elf/tst-auditmod1.c b/elf/tst-auditmod1.c
index 69da278..f762027 100644
--- a/elf/tst-auditmod1.c
+++ b/elf/tst-auditmod1.c
@@ -109,8 +109,13 @@ la_symbind64 (Elf64_Sym *sym, unsigned int ndx,
uintptr_t *refcook,
 # define La_retval La_i86_retval
 # define int_retval lrv_eax
 #elif defined __x86_64__
-# define pltenter la_x86_64_gnu_pltenter
-# define pltexit la_x86_64_gnu_pltexit
+# ifdef __LP64__
+#  define pltenter la_x86_64_gnu_pltenter
+#  define pltexit la_x86_64_gnu_pltexit
+# else
+#  define pltenter la_x32_gnu_pltenter
+#  define pltexit la_x32_gnu_pltexit
+# endif
 # define La_regs La_x86_64_regs
 # define La_retval La_x86_64_retval
 # define int_retval lrv_rax
@@ -188,7 +193,8 @@ pltexit (ElfW(Sym) *sym, unsigned int ndx,
uintptr_t *refcook,
 	 const char *symname)
 {
   printf ("pltexit: symname=%s, st_value=%#lx, ndx=%u, retval=%tu\n",
-	  symname, (long int) sym->st_value, ndx, outregs->int_retval);
+	  symname, (long int) sym->st_value, ndx,
+	  (ptrdiff_t) outregs->int_retval);

   return 0;
 }
---

Did I miss something?

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

-- 
H.J.