diff mbox series

[3/3] elf/tst-dl_find_object: Disable subtests for non-contiguous maps (bug 28732)

Message ID 636c6da259e612258791a6e816bfc7bbfed97e3a.1641228666.git.fweimer@redhat.com
State Committed
Commit 06200aac9bec34dbcac28b8c60e49a77e7851c1f
Headers
Series Fix elf/tst-dl_find_objects with --enable-hardcoded-path-in-tests |

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

Florian Weimer Jan. 3, 2022, 5:11 p.m. UTC 
  ---
 elf/tst-dl_find_object.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

Comments

H.J. Lu Jan. 14, 2022, 3:06 p.m. UTC | #1 
On Mon, Jan 3, 2022 at 9:13 AM Florian Weimer via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> ---
>  elf/tst-dl_find_object.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/elf/tst-dl_find_object.c b/elf/tst-dl_find_object.c
> index 21cdc0f848..2ad1924088 100644
> --- a/elf/tst-dl_find_object.c
> +++ b/elf/tst-dl_find_object.c
> @@ -71,19 +71,24 @@ check (void *address,
>                __FILE__, line, address,
>                actual.dlfo_flags, expected->dlfo_flags);
>      }
> -  if (actual.dlfo_flags != expected->dlfo_flags)
> +  if (expected->dlfo_link_map->l_contiguous)
>      {
> -      support_record_failure ();
> -      printf ("%s:%d: error: %p: map start is %p, expected %p\n",
> -              __FILE__, line,
> -              address, actual.dlfo_map_start, expected->dlfo_map_start);
> -    }
> -  if (actual.dlfo_map_end != expected->dlfo_map_end)
> -    {
> -      support_record_failure ();
> -      printf ("%s:%d: error: %p: map end is %p, expected %p\n",
> -              __FILE__, line,
> -              address, actual.dlfo_map_end, expected->dlfo_map_end);
> +      /* If the mappings are not contiguous, the actual and execpted
> +         mappings may differ, so this subtest will not work.  */
> +      if (actual.dlfo_flags != expected->dlfo_flags)
> +        {
> +          support_record_failure ();
> +          printf ("%s:%d: error: %p: map start is %p, expected %p\n",
> +                  __FILE__, line,
> +                  address, actual.dlfo_map_start, expected->dlfo_map_start);
> +        }
> +      if (actual.dlfo_map_end != expected->dlfo_map_end)
> +        {
> +          support_record_failure ();
> +          printf ("%s:%d: error: %p: map end is %p, expected %p\n",
> +                  __FILE__, line,
> +                  address, actual.dlfo_map_end, expected->dlfo_map_end);
> +        }
>      }
>    if (actual.dlfo_link_map != expected->dlfo_link_map)
>      {
> --
> 2.33.1
>

I still see

FAIL: elf/tst-dl_find_object

even when using the new linker with the fix for

https://sourceware.org/bugzilla/show_bug.cgi?id=28743

to remove the 1-page gap.  Which file doesn't have
non-contiguous mapping?
H.J. Lu Jan. 14, 2022, 3:09 p.m. UTC | #2 
On Fri, Jan 14, 2022 at 7:06 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Mon, Jan 3, 2022 at 9:13 AM Florian Weimer via Libc-alpha
> <libc-alpha@sourceware.org> wrote:
> >
> > ---
> >  elf/tst-dl_find_object.c | 29 +++++++++++++++++------------
> >  1 file changed, 17 insertions(+), 12 deletions(-)
> >
> > diff --git a/elf/tst-dl_find_object.c b/elf/tst-dl_find_object.c
> > index 21cdc0f848..2ad1924088 100644
> > --- a/elf/tst-dl_find_object.c
> > +++ b/elf/tst-dl_find_object.c
> > @@ -71,19 +71,24 @@ check (void *address,
> >                __FILE__, line, address,
> >                actual.dlfo_flags, expected->dlfo_flags);
> >      }
> > -  if (actual.dlfo_flags != expected->dlfo_flags)
> > +  if (expected->dlfo_link_map->l_contiguous)
> >      {
> > -      support_record_failure ();
> > -      printf ("%s:%d: error: %p: map start is %p, expected %p\n",
> > -              __FILE__, line,
> > -              address, actual.dlfo_map_start, expected->dlfo_map_start);
> > -    }
> > -  if (actual.dlfo_map_end != expected->dlfo_map_end)
> > -    {
> > -      support_record_failure ();
> > -      printf ("%s:%d: error: %p: map end is %p, expected %p\n",
> > -              __FILE__, line,
> > -              address, actual.dlfo_map_end, expected->dlfo_map_end);
> > +      /* If the mappings are not contiguous, the actual and execpted
> > +         mappings may differ, so this subtest will not work.  */
> > +      if (actual.dlfo_flags != expected->dlfo_flags)
> > +        {
> > +          support_record_failure ();
> > +          printf ("%s:%d: error: %p: map start is %p, expected %p\n",
> > +                  __FILE__, line,
> > +                  address, actual.dlfo_map_start, expected->dlfo_map_start);
> > +        }
> > +      if (actual.dlfo_map_end != expected->dlfo_map_end)
> > +        {
> > +          support_record_failure ();
> > +          printf ("%s:%d: error: %p: map end is %p, expected %p\n",
> > +                  __FILE__, line,
> > +                  address, actual.dlfo_map_end, expected->dlfo_map_end);
> > +        }
> >      }
> >    if (actual.dlfo_link_map != expected->dlfo_link_map)
> >      {
> > --
> > 2.33.1
> >
>
> I still see
>
> FAIL: elf/tst-dl_find_object
>
> even when using the new linker with the fix for
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=28743
>
> to remove the 1-page gap.  Which file doesn't have
> non-contiguous mapping?
>

The linker bug isn't really fixed:

  [21] .eh_frame         PROGBITS        0000000000005ef0 005ef0
000758 00   A  0   0  8
  [22] .init_array       INIT_ARRAY      0000000000007000 007000
000010 08  WA  0   0  8
  [23] .fini_array       FINI_ARRAY      0000000000007010 007010
000008 08  WA  0   0  8
  [24] .data.rel.ro      PROGBITS        0000000000007020 007020
0000a0 00  WA  0   0 32
  [25] .dynamic          DYNAMIC         00000000000070c0 0070c0
000200 10  WA  8   0  8
  [26] .got              PROGBITS        00000000000072c0 0072c0
000078 08  WA  0   0  8
  [27] .got.plt          PROGBITS        0000000000008000 008000
0001d0 08  WA  0   0  8
Florian Weimer Jan. 14, 2022, 3:10 p.m. UTC | #3 
* H. J. Lu:

> On Mon, Jan 3, 2022 at 9:13 AM Florian Weimer via Libc-alpha
> <libc-alpha@sourceware.org> wrote:
>>
>> ---
>>  elf/tst-dl_find_object.c | 29 +++++++++++++++++------------
>>  1 file changed, 17 insertions(+), 12 deletions(-)
>>
>> diff --git a/elf/tst-dl_find_object.c b/elf/tst-dl_find_object.c
>> index 21cdc0f848..2ad1924088 100644
>> --- a/elf/tst-dl_find_object.c
>> +++ b/elf/tst-dl_find_object.c
>> @@ -71,19 +71,24 @@ check (void *address,
>>                __FILE__, line, address,
>>                actual.dlfo_flags, expected->dlfo_flags);
>>      }
>> -  if (actual.dlfo_flags != expected->dlfo_flags)
>> +  if (expected->dlfo_link_map->l_contiguous)
>>      {
>> -      support_record_failure ();
>> -      printf ("%s:%d: error: %p: map start is %p, expected %p\n",
>> -              __FILE__, line,
>> -              address, actual.dlfo_map_start, expected->dlfo_map_start);
>> -    }
>> -  if (actual.dlfo_map_end != expected->dlfo_map_end)
>> -    {
>> -      support_record_failure ();
>> -      printf ("%s:%d: error: %p: map end is %p, expected %p\n",
>> -              __FILE__, line,
>> -              address, actual.dlfo_map_end, expected->dlfo_map_end);
>> +      /* If the mappings are not contiguous, the actual and execpted
>> +         mappings may differ, so this subtest will not work.  */
>> +      if (actual.dlfo_flags != expected->dlfo_flags)
>> +        {
>> +          support_record_failure ();
>> +          printf ("%s:%d: error: %p: map start is %p, expected %p\n",
>> +                  __FILE__, line,
>> +                  address, actual.dlfo_map_start, expected->dlfo_map_start);
>> +        }
>> +      if (actual.dlfo_map_end != expected->dlfo_map_end)
>> +        {
>> +          support_record_failure ();
>> +          printf ("%s:%d: error: %p: map end is %p, expected %p\n",
>> +                  __FILE__, line,
>> +                  address, actual.dlfo_map_end, expected->dlfo_map_end);
>> +        }
>>      }
>>    if (actual.dlfo_link_map != expected->dlfo_link_map)
>>      {
>> --
>> 2.33.1
>>
>
> I still see
>
> FAIL: elf/tst-dl_find_object
>
> even when using the new linker with the fix for
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=28743
>
> to remove the 1-page gap.  Which file doesn't have
> non-contiguous mapping?

We never set l_contiguous for the main executable, so it doesn't matter
what the link editor does.  And none of the glibc fixes went in so far.

Thanks,
Florian
H.J. Lu Jan. 14, 2022, 3:19 p.m. UTC | #4 
On Fri, Jan 14, 2022 at 7:10 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > On Mon, Jan 3, 2022 at 9:13 AM Florian Weimer via Libc-alpha
> > <libc-alpha@sourceware.org> wrote:
> >>
> >> ---
> >>  elf/tst-dl_find_object.c | 29 +++++++++++++++++------------
> >>  1 file changed, 17 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/elf/tst-dl_find_object.c b/elf/tst-dl_find_object.c
> >> index 21cdc0f848..2ad1924088 100644
> >> --- a/elf/tst-dl_find_object.c
> >> +++ b/elf/tst-dl_find_object.c
> >> @@ -71,19 +71,24 @@ check (void *address,
> >>                __FILE__, line, address,
> >>                actual.dlfo_flags, expected->dlfo_flags);
> >>      }
> >> -  if (actual.dlfo_flags != expected->dlfo_flags)
> >> +  if (expected->dlfo_link_map->l_contiguous)
> >>      {
> >> -      support_record_failure ();
> >> -      printf ("%s:%d: error: %p: map start is %p, expected %p\n",
> >> -              __FILE__, line,
> >> -              address, actual.dlfo_map_start, expected->dlfo_map_start);
> >> -    }
> >> -  if (actual.dlfo_map_end != expected->dlfo_map_end)
> >> -    {
> >> -      support_record_failure ();
> >> -      printf ("%s:%d: error: %p: map end is %p, expected %p\n",
> >> -              __FILE__, line,
> >> -              address, actual.dlfo_map_end, expected->dlfo_map_end);
> >> +      /* If the mappings are not contiguous, the actual and execpted
> >> +         mappings may differ, so this subtest will not work.  */
> >> +      if (actual.dlfo_flags != expected->dlfo_flags)
> >> +        {
> >> +          support_record_failure ();
> >> +          printf ("%s:%d: error: %p: map start is %p, expected %p\n",
> >> +                  __FILE__, line,
> >> +                  address, actual.dlfo_map_start, expected->dlfo_map_start);
> >> +        }
> >> +      if (actual.dlfo_map_end != expected->dlfo_map_end)
> >> +        {
> >> +          support_record_failure ();
> >> +          printf ("%s:%d: error: %p: map end is %p, expected %p\n",
> >> +                  __FILE__, line,
> >> +                  address, actual.dlfo_map_end, expected->dlfo_map_end);
> >> +        }
> >>      }
> >>    if (actual.dlfo_link_map != expected->dlfo_link_map)
> >>      {
> >> --
> >> 2.33.1
> >>
> >
> > I still see
> >
> > FAIL: elf/tst-dl_find_object
> >
> > even when using the new linker with the fix for
> >
> > https://sourceware.org/bugzilla/show_bug.cgi?id=28743
> >
> > to remove the 1-page gap.  Which file doesn't have
> > non-contiguous mapping?
>
> We never set l_contiguous for the main executable, so it doesn't matter
> what the link editor does.  And none of the glibc fixes went in so far.
>
> Thanks,
> Florian
>

Should l_contiguous be set on the main executable? If not, why?
Florian Weimer Jan. 14, 2022, 3:39 p.m. UTC | #5 
* H. J. Lu:

>> We never set l_contiguous for the main executable, so it doesn't matter
>> what the link editor does.  And none of the glibc fixes went in so far.
>>
>> Thanks,
>> Florian
>>
>
> Should l_contiguous be set on the main executable? If not, why?

I think it should be set even if the kernel loads the main executable.
See patch 2:

  [PATCH 2/3] elf: Set l_contiguous to 1 for the main map in more cases
  <https://sourceware.org/pipermail/libc-alpha/2022-January/134894.html>

Thanks,
Florian
H.J. Lu Jan. 14, 2022, 3:47 p.m. UTC | #6 
On Fri, Jan 14, 2022 at 7:39 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> >> We never set l_contiguous for the main executable, so it doesn't matter
> >> what the link editor does.  And none of the glibc fixes went in so far.
> >>
> >> Thanks,
> >> Florian
> >>
> >
> > Should l_contiguous be set on the main executable? If not, why?
>
> I think it should be set even if the kernel loads the main executable.
> See patch 2:
>
>   [PATCH 2/3] elf: Set l_contiguous to 1 for the main map in more cases
>   <https://sourceware.org/pipermail/libc-alpha/2022-January/134894.html>

If we have the second patch, do we still need the 3rd one?
Florian Weimer Jan. 14, 2022, 3:51 p.m. UTC | #7 
* H. J. Lu:

> On Fri, Jan 14, 2022 at 7:39 AM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * H. J. Lu:
>>
>> >> We never set l_contiguous for the main executable, so it doesn't matter
>> >> what the link editor does.  And none of the glibc fixes went in so far.
>> >>
>> >> Thanks,
>> >> Florian
>> >>
>> >
>> > Should l_contiguous be set on the main executable? If not, why?
>>
>> I think it should be set even if the kernel loads the main executable.
>> See patch 2:
>>
>>   [PATCH 2/3] elf: Set l_contiguous to 1 for the main map in more cases
>>   <https://sourceware.org/pipermail/libc-alpha/2022-January/134894.html>
>
> If we have the second patch, do we still need the 3rd one?

I think gaps are expected on some architectures for main executable
(ia64?).

Thanks,
Florian
H.J. Lu Jan. 14, 2022, 3:54 p.m. UTC | #8 
On Fri, Jan 14, 2022 at 7:51 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > On Fri, Jan 14, 2022 at 7:39 AM Florian Weimer <fweimer@redhat.com> wrote:
> >>
> >> * H. J. Lu:
> >>
> >> >> We never set l_contiguous for the main executable, so it doesn't matter
> >> >> what the link editor does.  And none of the glibc fixes went in so far.
> >> >>
> >> >> Thanks,
> >> >> Florian
> >> >>
> >> >
> >> > Should l_contiguous be set on the main executable? If not, why?
> >>
> >> I think it should be set even if the kernel loads the main executable.
> >> See patch 2:
> >>
> >>   [PATCH 2/3] elf: Set l_contiguous to 1 for the main map in more cases
> >>   <https://sourceware.org/pipermail/libc-alpha/2022-January/134894.html>
> >
> > If we have the second patch, do we still need the 3rd one?
>
> I think gaps are expected on some architectures for main executable
> (ia64?).
>

Please add a header to indicate that the gap is expected only on some
architectures.
Florian Weimer Jan. 14, 2022, 3:56 p.m. UTC | #9 
* H. J. Lu:

> On Fri, Jan 14, 2022 at 7:51 AM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * H. J. Lu:
>>
>> > On Fri, Jan 14, 2022 at 7:39 AM Florian Weimer <fweimer@redhat.com> wrote:
>> >>
>> >> * H. J. Lu:
>> >>
>> >> >> We never set l_contiguous for the main executable, so it doesn't matter
>> >> >> what the link editor does.  And none of the glibc fixes went in so far.
>> >> >>
>> >> >> Thanks,
>> >> >> Florian
>> >> >>
>> >> >
>> >> > Should l_contiguous be set on the main executable? If not, why?
>> >>
>> >> I think it should be set even if the kernel loads the main executable.
>> >> See patch 2:
>> >>
>> >>   [PATCH 2/3] elf: Set l_contiguous to 1 for the main map in more cases
>> >>   <https://sourceware.org/pipermail/libc-alpha/2022-January/134894.html>
>> >
>> > If we have the second patch, do we still need the 3rd one?
>>
>> I think gaps are expected on some architectures for main executable
>> (ia64?).
>>
>
> Please add a header to indicate that the gap is expected only on some
> architectures.

Sorry, do you mean there should be a .h file to indicate whether the
architecture expects a gap?

How do I tell whether the gap is expected or the result of binutils
bugs?

Thanks,
Florian
H.J. Lu Jan. 14, 2022, 4:06 p.m. UTC | #10 
On Fri, Jan 14, 2022 at 7:56 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > On Fri, Jan 14, 2022 at 7:51 AM Florian Weimer <fweimer@redhat.com> wrote:
> >>
> >> * H. J. Lu:
> >>
> >> > On Fri, Jan 14, 2022 at 7:39 AM Florian Weimer <fweimer@redhat.com> wrote:
> >> >>
> >> >> * H. J. Lu:
> >> >>
> >> >> >> We never set l_contiguous for the main executable, so it doesn't matter
> >> >> >> what the link editor does.  And none of the glibc fixes went in so far.
> >> >> >>
> >> >> >> Thanks,
> >> >> >> Florian
> >> >> >>
> >> >> >
> >> >> > Should l_contiguous be set on the main executable? If not, why?
> >> >>
> >> >> I think it should be set even if the kernel loads the main executable.
> >> >> See patch 2:
> >> >>
> >> >>   [PATCH 2/3] elf: Set l_contiguous to 1 for the main map in more cases
> >> >>   <https://sourceware.org/pipermail/libc-alpha/2022-January/134894.html>
> >> >
> >> > If we have the second patch, do we still need the 3rd one?
> >>
> >> I think gaps are expected on some architectures for main executable
> >> (ia64?).
> >>
> >
> > Please add a header to indicate that the gap is expected only on some
> > architectures.
>
> Sorry, do you mean there should be a .h file to indicate whether the
> architecture expects a gap?
>
> How do I tell whether the gap is expected or the result of binutils
> bugs?
>

You can add a linker test to check for the linker bug.
Florian Weimer Jan. 14, 2022, 4:12 p.m. UTC | #11 
* H. J. Lu:

> You can add a linker test to check for the linker bug.

The linker bug could appear randomly, depending on the input files and
their contents.

Thanks,
Florian
H.J. Lu Jan. 16, 2022, 2:05 p.m. UTC | #12 
On Mon, Jan 3, 2022 at 9:13 AM Florian Weimer via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> ---
>  elf/tst-dl_find_object.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/elf/tst-dl_find_object.c b/elf/tst-dl_find_object.c
> index 21cdc0f848..2ad1924088 100644
> --- a/elf/tst-dl_find_object.c
> +++ b/elf/tst-dl_find_object.c
> @@ -71,19 +71,24 @@ check (void *address,
>                __FILE__, line, address,
>                actual.dlfo_flags, expected->dlfo_flags);
>      }
> -  if (actual.dlfo_flags != expected->dlfo_flags)
> +  if (expected->dlfo_link_map->l_contiguous)
>      {
> -      support_record_failure ();
> -      printf ("%s:%d: error: %p: map start is %p, expected %p\n",
> -              __FILE__, line,
> -              address, actual.dlfo_map_start, expected->dlfo_map_start);
> -    }
> -  if (actual.dlfo_map_end != expected->dlfo_map_end)
> -    {
> -      support_record_failure ();
> -      printf ("%s:%d: error: %p: map end is %p, expected %p\n",
> -              __FILE__, line,
> -              address, actual.dlfo_map_end, expected->dlfo_map_end);
> +      /* If the mappings are not contiguous, the actual and execpted
> +         mappings may differ, so this subtest will not work.  */
> +      if (actual.dlfo_flags != expected->dlfo_flags)
> +        {
> +          support_record_failure ();
> +          printf ("%s:%d: error: %p: map start is %p, expected %p\n",
> +                  __FILE__, line,
> +                  address, actual.dlfo_map_start, expected->dlfo_map_start);
> +        }
> +      if (actual.dlfo_map_end != expected->dlfo_map_end)
> +        {
> +          support_record_failure ();
> +          printf ("%s:%d: error: %p: map end is %p, expected %p\n",
> +                  __FILE__, line,
> +                  address, actual.dlfo_map_end, expected->dlfo_map_end);
> +        }
>      }
>    if (actual.dlfo_link_map != expected->dlfo_link_map)
>      {
> --
> 2.33.1
>

LGTM.

Reviewed-by: H.J. Lu <hjl.tools@gmail.com>

Thanks.
diff mbox series

Patch

diff --git a/elf/tst-dl_find_object.c b/elf/tst-dl_find_object.c
index 21cdc0f848..2ad1924088 100644
--- a/elf/tst-dl_find_object.c
+++ b/elf/tst-dl_find_object.c
@@ -71,19 +71,24 @@  check (void *address,
               __FILE__, line, address,
               actual.dlfo_flags, expected->dlfo_flags);
     }
-  if (actual.dlfo_flags != expected->dlfo_flags)
+  if (expected->dlfo_link_map->l_contiguous)
     {
-      support_record_failure ();
-      printf ("%s:%d: error: %p: map start is %p, expected %p\n",
-              __FILE__, line,
-              address, actual.dlfo_map_start, expected->dlfo_map_start);
-    }
-  if (actual.dlfo_map_end != expected->dlfo_map_end)
-    {
-      support_record_failure ();
-      printf ("%s:%d: error: %p: map end is %p, expected %p\n",
-              __FILE__, line,
-              address, actual.dlfo_map_end, expected->dlfo_map_end);
+      /* If the mappings are not contiguous, the actual and execpted
+         mappings may differ, so this subtest will not work.  */
+      if (actual.dlfo_flags != expected->dlfo_flags)
+        {
+          support_record_failure ();
+          printf ("%s:%d: error: %p: map start is %p, expected %p\n",
+                  __FILE__, line,
+                  address, actual.dlfo_map_start, expected->dlfo_map_start);
+        }
+      if (actual.dlfo_map_end != expected->dlfo_map_end)
+        {
+          support_record_failure ();
+          printf ("%s:%d: error: %p: map end is %p, expected %p\n",
+                  __FILE__, line,
+                  address, actual.dlfo_map_end, expected->dlfo_map_end);
+        }
     }
   if (actual.dlfo_link_map != expected->dlfo_link_map)
     {