diff mbox series

mips: fix elf/tst-dlmopen4

Message ID 9dff16aac870a046c7cc768eef84bcbd6fd4d65c.camel@mengyan1223.wang
State Superseded
Headers
Series mips: fix elf/tst-dlmopen4 |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

Xi Ruoyao Jan. 28, 2022, 6:54 p.m. UTC 
  MIPS uses DT_MIPS_RLD_MAP and DT_MIPS_RLD_MAP_REL instead of DT_DEBUG,
to provide access to r_debug.
---
 elf/tst-dlmopen4.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

Comments

Carlos O'Donell Jan. 28, 2022, 7:04 p.m. UTC | #1 
On 1/28/22 13:54, Xi Ruoyao via Libc-alpha wrote:
> MIPS uses DT_MIPS_RLD_MAP and DT_MIPS_RLD_MAP_REL instead of DT_DEBUG,
> to provide access to r_debug.

This looks correct to me. OK for glibc 2.35.

I'm going to push this today if there are no other objections.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  elf/tst-dlmopen4.c | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/elf/tst-dlmopen4.c b/elf/tst-dlmopen4.c
> index d8bcf7e9d5..de67a30a4b 100644
> --- a/elf/tst-dlmopen4.c
> +++ b/elf/tst-dlmopen4.c
> @@ -25,17 +25,26 @@
>  #include <support/check.h>
>  #include <support/test-driver.h>
>  
> -#ifndef ELF_MACHINE_GET_R_DEBUG
> -# define ELF_MACHINE_GET_R_DEBUG(d) \
> -    (__extension__ ({ 						\
> -      struct r_debug_extended *debug;				\
> -      if ((d)->d_tag == DT_DEBUG)				\
> -	debug = (struct r_debug_extended *) (d)->d_un.d_ptr;	\
> -      else							\
> -	debug = NULL;						\
> -      debug; }))
> +static struct r_debug_extended *
> +elf_get_r_debug (ElfW(Dyn) *d)
> +{
> +#ifdef __mips__
> +  if (d->d_tag == DT_MIPS_RLD_MAP_REL)
> +    {
> +      char *ptr = (char *) d;
> +      ptr += d->d_un.d_val;
> +      return *(struct r_debug_extended **) ptr;
> +    }
> +  else if (d->d_tag == DT_MIPS_RLD_MAP)
> +    return *(struct r_debug_extended **) d->d_un.d_ptr;

OK. Matches the logic in sysdeps/mips/dl-debug.h

> +#else
> +  if (d->d_tag == DT_DEBUG)
> +    return (struct r_debug_extended *) d->d_un.d_ptr;

OK.

>  #endif
>  
> +  return NULL;

OK.

> +}
> +
>  static int
>  do_test (void)
>  {
> @@ -44,7 +53,7 @@ do_test (void)
>  
>    for (d = _DYNAMIC; d->d_tag != DT_NULL; ++d)
>      {
> -      debug = ELF_MACHINE_GET_R_DEBUG (d);
> +      debug = elf_get_r_debug (d);

OK.

>        if (debug != NULL)
>  	break;
>      }
Xi Ruoyao Jan. 28, 2022, 8:12 p.m. UTC | #2 
On Fri, 2022-01-28 at 14:04 -0500, Carlos O'Donell wrote:
> On 1/28/22 13:54, Xi Ruoyao via Libc-alpha wrote:
> > MIPS uses DT_MIPS_RLD_MAP and DT_MIPS_RLD_MAP_REL instead of
> > DT_DEBUG,
> > to provide access to r_debug.
> 
> This looks correct to me. OK for glibc 2.35.
> 
> I'm going to push this today if there are no other objections.
> 
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>

Thanks!

Please review my patch for pldd on MIPS
(https://sourceware.org/pipermail/libc-alpha/2022-January/135821.html).
The logic is same.
H.J. Lu Jan. 28, 2022, 8:41 p.m. UTC | #3 
On Fri, Jan 28, 2022 at 11:04 AM Carlos O'Donell via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> On 1/28/22 13:54, Xi Ruoyao via Libc-alpha wrote:
> > MIPS uses DT_MIPS_RLD_MAP and DT_MIPS_RLD_MAP_REL instead of DT_DEBUG,
> > to provide access to r_debug.
>
> This looks correct to me. OK for glibc 2.35.
>
> I'm going to push this today if there are no other objections.
>
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
>
> > ---
> >  elf/tst-dlmopen4.c | 29 +++++++++++++++++++----------
> >  1 file changed, 19 insertions(+), 10 deletions(-)
> >
> > diff --git a/elf/tst-dlmopen4.c b/elf/tst-dlmopen4.c
> > index d8bcf7e9d5..de67a30a4b 100644
> > --- a/elf/tst-dlmopen4.c
> > +++ b/elf/tst-dlmopen4.c
> > @@ -25,17 +25,26 @@
> >  #include <support/check.h>
> >  #include <support/test-driver.h>
> >
> > -#ifndef ELF_MACHINE_GET_R_DEBUG
> > -# define ELF_MACHINE_GET_R_DEBUG(d) \
> > -    (__extension__ ({                                                \
> > -      struct r_debug_extended *debug;                                \
> > -      if ((d)->d_tag == DT_DEBUG)                            \
> > -     debug = (struct r_debug_extended *) (d)->d_un.d_ptr;    \
> > -      else                                                   \
> > -     debug = NULL;                                           \
> > -      debug; }))
> > +static struct r_debug_extended *
> > +elf_get_r_debug (ElfW(Dyn) *d)
> > +{
> > +#ifdef __mips__

Shouldn't MIPS simply define a proper ELF_MACHINE_GET_R_DEBUG?
It can be done with a header file.

> > +  if (d->d_tag == DT_MIPS_RLD_MAP_REL)
> > +    {
> > +      char *ptr = (char *) d;
> > +      ptr += d->d_un.d_val;
> > +      return *(struct r_debug_extended **) ptr;
> > +    }
> > +  else if (d->d_tag == DT_MIPS_RLD_MAP)
> > +    return *(struct r_debug_extended **) d->d_un.d_ptr;
>
> OK. Matches the logic in sysdeps/mips/dl-debug.h
>
> > +#else
> > +  if (d->d_tag == DT_DEBUG)
> > +    return (struct r_debug_extended *) d->d_un.d_ptr;
>
> OK.
>
> >  #endif
> >
> > +  return NULL;
>
> OK.
>
> > +}
> > +
> >  static int
> >  do_test (void)
> >  {
> > @@ -44,7 +53,7 @@ do_test (void)
> >
> >    for (d = _DYNAMIC; d->d_tag != DT_NULL; ++d)
> >      {
> > -      debug = ELF_MACHINE_GET_R_DEBUG (d);
> > +      debug = elf_get_r_debug (d);
>
> OK.
>
> >        if (debug != NULL)
> >       break;
> >      }
>
>
> --
> Cheers,
> Carlos.
>
Carlos O'Donell Jan. 28, 2022, 8:49 p.m. UTC | #4 
On 1/28/22 15:41, H.J. Lu wrote:
> On Fri, Jan 28, 2022 at 11:04 AM Carlos O'Donell via Libc-alpha
> <libc-alpha@sourceware.org> wrote:
>>
>> On 1/28/22 13:54, Xi Ruoyao via Libc-alpha wrote:
>>> MIPS uses DT_MIPS_RLD_MAP and DT_MIPS_RLD_MAP_REL instead of DT_DEBUG,
>>> to provide access to r_debug.
>>
>> This looks correct to me. OK for glibc 2.35.
>>
>> I'm going to push this today if there are no other objections.
>>
>> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
>>
>>> ---
>>>  elf/tst-dlmopen4.c | 29 +++++++++++++++++++----------
>>>  1 file changed, 19 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/elf/tst-dlmopen4.c b/elf/tst-dlmopen4.c
>>> index d8bcf7e9d5..de67a30a4b 100644
>>> --- a/elf/tst-dlmopen4.c
>>> +++ b/elf/tst-dlmopen4.c
>>> @@ -25,17 +25,26 @@
>>>  #include <support/check.h>
>>>  #include <support/test-driver.h>
>>>
>>> -#ifndef ELF_MACHINE_GET_R_DEBUG
>>> -# define ELF_MACHINE_GET_R_DEBUG(d) \
>>> -    (__extension__ ({                                                \
>>> -      struct r_debug_extended *debug;                                \
>>> -      if ((d)->d_tag == DT_DEBUG)                            \
>>> -     debug = (struct r_debug_extended *) (d)->d_un.d_ptr;    \
>>> -      else                                                   \
>>> -     debug = NULL;                                           \
>>> -      debug; }))
>>> +static struct r_debug_extended *
>>> +elf_get_r_debug (ElfW(Dyn) *d)
>>> +{
>>> +#ifdef __mips__
> 
> Shouldn't MIPS simply define a proper ELF_MACHINE_GET_R_DEBUG?
> It can be done with a header file.

The only use of ELF_MACHINE_GET_R_DEBUG is in the test, and the static function
is easier to read, edit, and debug, all of which is better for a test?

 
>>> +  if (d->d_tag == DT_MIPS_RLD_MAP_REL)
>>> +    {
>>> +      char *ptr = (char *) d;
>>> +      ptr += d->d_un.d_val;
>>> +      return *(struct r_debug_extended **) ptr;
>>> +    }
>>> +  else if (d->d_tag == DT_MIPS_RLD_MAP)
>>> +    return *(struct r_debug_extended **) d->d_un.d_ptr;
>>
>> OK. Matches the logic in sysdeps/mips/dl-debug.h
>>
>>> +#else
>>> +  if (d->d_tag == DT_DEBUG)
>>> +    return (struct r_debug_extended *) d->d_un.d_ptr;
>>
>> OK.
>>
>>>  #endif
>>>
>>> +  return NULL;
>>
>> OK.
>>
>>> +}
>>> +
>>>  static int
>>>  do_test (void)
>>>  {
>>> @@ -44,7 +53,7 @@ do_test (void)
>>>
>>>    for (d = _DYNAMIC; d->d_tag != DT_NULL; ++d)
>>>      {
>>> -      debug = ELF_MACHINE_GET_R_DEBUG (d);
>>> +      debug = elf_get_r_debug (d);
>>
>> OK.
>>
>>>        if (debug != NULL)
>>>       break;
>>>      }
>>
>>
>> --
>> Cheers,
>> Carlos.
>>
> 
>
H.J. Lu Jan. 28, 2022, 9:20 p.m. UTC | #5 
On Fri, Jan 28, 2022 at 12:49 PM Carlos O'Donell <carlos@redhat.com> wrote:
>
> On 1/28/22 15:41, H.J. Lu wrote:
> > On Fri, Jan 28, 2022 at 11:04 AM Carlos O'Donell via Libc-alpha
> > <libc-alpha@sourceware.org> wrote:
> >>
> >> On 1/28/22 13:54, Xi Ruoyao via Libc-alpha wrote:
> >>> MIPS uses DT_MIPS_RLD_MAP and DT_MIPS_RLD_MAP_REL instead of DT_DEBUG,
> >>> to provide access to r_debug.
> >>
> >> This looks correct to me. OK for glibc 2.35.
> >>
> >> I'm going to push this today if there are no other objections.
> >>
> >> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> >>
> >>> ---
> >>>  elf/tst-dlmopen4.c | 29 +++++++++++++++++++----------
> >>>  1 file changed, 19 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/elf/tst-dlmopen4.c b/elf/tst-dlmopen4.c
> >>> index d8bcf7e9d5..de67a30a4b 100644
> >>> --- a/elf/tst-dlmopen4.c
> >>> +++ b/elf/tst-dlmopen4.c
> >>> @@ -25,17 +25,26 @@
> >>>  #include <support/check.h>
> >>>  #include <support/test-driver.h>
> >>>
> >>> -#ifndef ELF_MACHINE_GET_R_DEBUG
> >>> -# define ELF_MACHINE_GET_R_DEBUG(d) \
> >>> -    (__extension__ ({                                                \
> >>> -      struct r_debug_extended *debug;                                \
> >>> -      if ((d)->d_tag == DT_DEBUG)                            \
> >>> -     debug = (struct r_debug_extended *) (d)->d_un.d_ptr;    \
> >>> -      else                                                   \
> >>> -     debug = NULL;                                           \
> >>> -      debug; }))
> >>> +static struct r_debug_extended *
> >>> +elf_get_r_debug (ElfW(Dyn) *d)
> >>> +{
> >>> +#ifdef __mips__
> >
> > Shouldn't MIPS simply define a proper ELF_MACHINE_GET_R_DEBUG?
> > It can be done with a header file.
>
> The only use of ELF_MACHINE_GET_R_DEBUG is in the test, and the static function
> is easier to read, edit, and debug, all of which is better for a test?

I don't think __mips__ should be in generic code.  There
another place where we need to handle _r_debug differences.
A single header file can cover both.

>
> >>> +  if (d->d_tag == DT_MIPS_RLD_MAP_REL)
> >>> +    {
> >>> +      char *ptr = (char *) d;
> >>> +      ptr += d->d_un.d_val;
> >>> +      return *(struct r_debug_extended **) ptr;
> >>> +    }
> >>> +  else if (d->d_tag == DT_MIPS_RLD_MAP)
> >>> +    return *(struct r_debug_extended **) d->d_un.d_ptr;
> >>
> >> OK. Matches the logic in sysdeps/mips/dl-debug.h
> >>
> >>> +#else
> >>> +  if (d->d_tag == DT_DEBUG)
> >>> +    return (struct r_debug_extended *) d->d_un.d_ptr;
> >>
> >> OK.
> >>
> >>>  #endif
> >>>
> >>> +  return NULL;
> >>
> >> OK.
> >>
> >>> +}
> >>> +
> >>>  static int
> >>>  do_test (void)
> >>>  {
> >>> @@ -44,7 +53,7 @@ do_test (void)
> >>>
> >>>    for (d = _DYNAMIC; d->d_tag != DT_NULL; ++d)
> >>>      {
> >>> -      debug = ELF_MACHINE_GET_R_DEBUG (d);
> >>> +      debug = elf_get_r_debug (d);
> >>
> >> OK.
> >>
> >>>        if (debug != NULL)
> >>>       break;
> >>>      }
> >>
> >>
> >> --
> >> Cheers,
> >> Carlos.
> >>
> >
> >
>
>
> --
> Cheers,
> Carlos.
>
Xi Ruoyao Jan. 28, 2022, 9:23 p.m. UTC | #6 
On Fri, 2022-01-28 at 15:49 -0500, Carlos O'Donell wrote:
> On 1/28/22 15:41, H.J. Lu wrote:
> > On Fri, Jan 28, 2022 at 11:04 AM Carlos O'Donell via Libc-alpha
> > <libc-alpha@sourceware.org> wrote:
> > > 
> > > On 1/28/22 13:54, Xi Ruoyao via Libc-alpha wrote:
> > > > MIPS uses DT_MIPS_RLD_MAP and DT_MIPS_RLD_MAP_REL instead of
> > > > DT_DEBUG,
> > > > to provide access to r_debug.
> > > 
> > > This looks correct to me. OK for glibc 2.35.
> > > 
> > > I'm going to push this today if there are no other objections.
> > > 
> > > Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> > > 
> > > > ---
> > > >  elf/tst-dlmopen4.c | 29 +++++++++++++++++++----------
> > > >  1 file changed, 19 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/elf/tst-dlmopen4.c b/elf/tst-dlmopen4.c
> > > > index d8bcf7e9d5..de67a30a4b 100644
> > > > --- a/elf/tst-dlmopen4.c
> > > > +++ b/elf/tst-dlmopen4.c
> > > > @@ -25,17 +25,26 @@
> > > >  #include <support/check.h>
> > > >  #include <support/test-driver.h>
> > > > 
> > > > -#ifndef ELF_MACHINE_GET_R_DEBUG
> > > > -# define ELF_MACHINE_GET_R_DEBUG(d) \
> > > > -    (__extension__
> > > > ({                                                \
> > > > -      struct r_debug_extended
> > > > *debug;                                \
> > > > -      if ((d)->d_tag == DT_DEBUG)                            \
> > > > -     debug = (struct r_debug_extended *) (d)->d_un.d_ptr;    \
> > > > -      else                                                   \
> > > > -     debug = NULL;                                           \
> > > > -      debug; }))
> > > > +static struct r_debug_extended *
> > > > +elf_get_r_debug (ElfW(Dyn) *d)
> > > > +{
> > > > +#ifdef __mips__
> > 
> > Shouldn't MIPS simply define a proper ELF_MACHINE_GET_R_DEBUG?
> > It can be done with a header file.
> 
> The only use of ELF_MACHINE_GET_R_DEBUG is in the test, and the static
> function
> is easier to read, edit, and debug, all of which is better for a test?

I don't want to add a new header for a test.  And among the existing
headers, it seems the alternative definition can only go in
sysdeps/mips/bits/link.h, which is a part of the public API.

(And I'm really not a fan of "macros" like "{...; retval;}" :).
diff mbox series

Patch

diff --git a/elf/tst-dlmopen4.c b/elf/tst-dlmopen4.c
index d8bcf7e9d5..de67a30a4b 100644
--- a/elf/tst-dlmopen4.c
+++ b/elf/tst-dlmopen4.c
@@ -25,17 +25,26 @@ 
 #include <support/check.h>
 #include <support/test-driver.h>
 
-#ifndef ELF_MACHINE_GET_R_DEBUG
-# define ELF_MACHINE_GET_R_DEBUG(d) \
-    (__extension__ ({ 						\
-      struct r_debug_extended *debug;				\
-      if ((d)->d_tag == DT_DEBUG)				\
-	debug = (struct r_debug_extended *) (d)->d_un.d_ptr;	\
-      else							\
-	debug = NULL;						\
-      debug; }))
+static struct r_debug_extended *
+elf_get_r_debug (ElfW(Dyn) *d)
+{
+#ifdef __mips__
+  if (d->d_tag == DT_MIPS_RLD_MAP_REL)
+    {
+      char *ptr = (char *) d;
+      ptr += d->d_un.d_val;
+      return *(struct r_debug_extended **) ptr;
+    }
+  else if (d->d_tag == DT_MIPS_RLD_MAP)
+    return *(struct r_debug_extended **) d->d_un.d_ptr;
+#else
+  if (d->d_tag == DT_DEBUG)
+    return (struct r_debug_extended *) d->d_un.d_ptr;
 #endif
 
+  return NULL;
+}
+
 static int
 do_test (void)
 {
@@ -44,7 +53,7 @@  do_test (void)
 
   for (d = _DYNAMIC; d->d_tag != DT_NULL; ++d)
     {
-      debug = ELF_MACHINE_GET_R_DEBUG (d);
+      debug = elf_get_r_debug (d);
       if (debug != NULL)
 	break;
     }