Re: PR28827 testcase

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

On Sun, Feb 6, 2022 at 7:15 PM Alan Modra <amodra@gmail.com> wrote:
>
> On Sun, Feb 06, 2022 at 03:38:41PM -0800, H.J. Lu wrote:
> > On Sun, Feb 6, 2022 at 2:54 PM Alan Modra <amodra@gmail.com> wrote:
> > >
> > > On Sat, Feb 05, 2022 at 08:26:55PM -0800, H.J. Lu wrote:
> > > > On Sun, Feb 06, 2022 at 02:47:19PM +1030, Alan Modra via Binutils wrote:
> > > > > On Sat, Feb 05, 2022 at 10:39:50AM +0000, Nick Clifton wrote:
> > > > > > Hi Alan, Hi Fangrui,
> > > > > >
> > > > > > > This testcase triggers a stub sizing error with the patches applied
> > > > > > > for PR28743 (commit 2f83249c13d8 and c804c6f98d34).
> > > > > > >
> > > > > > >         PR 28827
> > > > > > >         * testsuite/ld-powerpc/pr28827-1.s,
> > > > > > >         * testsuite/ld-powerpc/pr28827-1.d: New test.
> > > > > > >         * testsuite/ld-powerpc/powerpc.exp: Run it.
> > > > > >
> > > > > > Given the importance of the PowerPC target, I am going to hold
> > > > > > off from creating the 2.38 release until this issue is fixed.
> > > > >
> > > > > Thanks, I appreciate it.
> > > > >
> > > > > >  I do hope however that it can be resolved soon....
> > > > >
> > > > > The solution is to revert HJ's two relro patches on the branch.  That
> > > > > will let you immediately make a release.  Despite being raised by
> > > > > Florian, I don't believe PR28743 is an important bug to fix just
> > > > > before a release.  Our relro support has sometimes created a hole for
> > > > > *years*.
> > > > >
> > > > > Of course, the patches ought to be reverted on mainline too,
> > > > > separately from whatever solution we finally adopt for PR28743.
> > > > >
> > > >
> > > > Why make removing the 1-page gap before the PT_GNU_RELRO segment opt-in?
> > > >
> > > > https://sourceware.org/pipermail/binutils/2022-February/119625.html
> > >
> > > I happen to think your changes to lang_size_relro_segment_1 are wrong.
> > > Making them optional doesn't fix that.
> >
> > So far, it has been working for x86.   Do you have a testcase to show
> > otherwise?
>
> Just put an ASSERT(0) on the do_reset code path.

On Linux/x86-64, I tried this with binutils-2_37-branch:

diff --git a/ld/ldlang.c b/ld/ldlang.c
index 042b492d52b..4760d45721a 100644
--- a/ld/ldlang.c
+++ b/ld/ldlang.c
@@ -6509,13 +6509,18 @@ lang_size_sections (bool *relax, bool check_regions)

   if (expld.dataseg.phase == exp_seg_end_seen)
     {
+      static int reset_counter;
+
       bool do_reset
  = lang_size_relro_segment (relax, check_regions);

       if (do_reset)
  {
+   reset_counter++;
    lang_reset_memory_regions ();
    one_lang_size_sections_pass (relax, check_regions);
+   if (reset_counter > 2)
+     abort ();
  }

       if (link_info.relro && expld.dataseg.relro_end)

It triggered:

Running /export/gnu/import/git/gitlab/x86-binutils-release/ld/testsuite/ld-elf/elf.exp
...
FAIL: ld-elf/eh5

Change it to

+   if (reset_counter > 3)
+     abort ();

avoided it.  The same also happens with my patches.

> > > The major reason is that I question the premise behind the patch.  Is
> > > it really worth wasting up to maxpagesize-1 in memory at the end of
> >
> > My patches shouldn't create a gap of more than 1-page at the end.
> > If it isn't the case, do you have a testcase for x86?  I don't see the
> > value of a 1-page gap before RELRO segment on x86.  I'd like to
> > avoid it on x86 if possible.
> >
> > > the relro segment in order to remove a page gap at the beginning of
> > > the relro segment?  That seems a dubious trade-off to me.  If process
> > > memory is an issue then it would be better to increase disk image size
> > > for reduced memory size.
> > >
> > > There is also the fact that we now hit the do_reset code path in
> > > lang_size_relro_segment when running multiple ld tests.  I think that
> > > says the current implementation is broken.
>
> I'm tired of arguing.  Your brokem patches have been reverted.
>

Is there a way to enable my patches just for x86?


-- 
H.J.