| Message ID | 9dff16aac870a046c7cc768eef84bcbd6fd4d65c.camel@mengyan1223.wang |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mips: fix elf/tst-dlmopen4 | expand |
| 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 |
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; > }
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.
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. >
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. >> > >
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. >
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 --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; }