RISC-V: Fix TLS and --gc-sections conflict.

Message ID 20180802231457.12493-1-jimw@sifive.com
State New
Headers show
Series
  • RISC-V: Fix TLS and --gc-sections conflict.
Related show

Commit Message

Jim Wilson Aug. 2, 2018, 11:14 p.m.
This fixes a problem reported as a gcc bug.
    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86770
The libstdc++ testsuite fails when linking with --gc-sections, generating an
error that says we have a reference to a discarded section .tdata.dyn.

The problem here is that .tdata.dyn is a linker created section for copy
relocs, that doesn't get populated until after sections are garbage collected.
So we have to mark it to keep it from being deleted, and a good way to do that
is to add the SEC_LINKER_CREATED flag.  I didn't see any bad effects from this.
It gets merged into .tdata, which still gets optimized away if empty at the
end.

This was tested with cross binutils and gcc builds and checks, with the gcc
testing hacked to force use of --gc-sections.  The patch fixes about 59
libstdc++ failures.

Committed.

Jim

	bfd/
	* elfnn-riscv.c (riscv_elf_create_dynamic_sections): For .tdata.dyn,
	add SEC_LINKER_CREATED flag.
---
 bfd/elfnn-riscv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
2.17.1

Comments

Andreas Schwab Oct. 2, 2018, 9:51 a.m. | #1
On Aug 02 2018, Jim Wilson <jimw@sifive.com> wrote:

> It gets merged into .tdata, which still gets optimized away if empty at the

> end.


Does it?  I don't see that happening.  The linker now always generates a
.tdata section, moreover with the wrong type NOBITS if empty.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Jim Wilson Oct. 3, 2018, 12:39 a.m. | #2
On Tue, Oct 2, 2018 at 2:51 AM Andreas Schwab <schwab@suse.de> wrote:
> On Aug 02 2018, Jim Wilson <jimw@sifive.com> wrote:

> > It gets merged into .tdata, which still gets optimized away if empty at the

> > end.

>

> Does it?  I don't see that happening.  The linker now always generates a

> .tdata section, moreover with the wrong type NOBITS if empty.


Apparently not, as I can see it in a testcase now.  Not sure where I
made the mistake.  But does this cause any actual problems?  I didn't
see any problems with my gcc testsuite run.  It is target independent
code that is setting the .tdata section to NOBITS when empty.  So that
part doesn't seem to be a problem with the RISC-V code.  Is there some
reason why this is wrong?  I do see that we have an empty PT_TLS
segment.  Maybe there is a performance issue here with doing extra
unnecessary work at load time?

As far as I know, RISC-V is the only target with copy relocs against
thread data which is the ultimate cause of this problem.   This means
tdata sections may not work exactly the same as other targets, and we
might need changes to target independent code in bfd to improve this.
But I don't think that my earlier patch is wrong, as it is fixing gcc
testsuite failures with --gc-sections.  I probably need to spend some
time studying bfd and see if I can find a way to delete an empty tdata
section when there are no copy relocs.  Maybe I can strip the
tdata.dyn section in elfnn-riscv.c when it is zero size.  Or maybe I
can change where it is created, so that it is only created when
non-zero.  I will take a look at this.

Still, it would be nice to know exactly why this is wrong.

Jim
Andreas Schwab Oct. 4, 2018, 7:38 a.m. | #3
On Okt 02 2018, Jim Wilson <jimw@sifive.com> wrote:

> On Tue, Oct 2, 2018 at 2:51 AM Andreas Schwab <schwab@suse.de> wrote:

>> On Aug 02 2018, Jim Wilson <jimw@sifive.com> wrote:

>> > It gets merged into .tdata, which still gets optimized away if empty at the

>> > end.

>>

>> Does it?  I don't see that happening.  The linker now always generates a

>> .tdata section, moreover with the wrong type NOBITS if empty.

>

> Apparently not, as I can see it in a testcase now.  Not sure where I

> made the mistake.  But does this cause any actual problems?  I didn't

> see any problems with my gcc testsuite run.


elflint is complaining, and that breaks the elfutils testsuite.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Jim Wilson Oct. 4, 2018, 8:34 p.m. | #4
On Thu, Oct 4, 2018 at 12:38 AM Andreas Schwab <schwab@suse.de> wrote:
> On Okt 02 2018, Jim Wilson <jimw@sifive.com> wrote:

> > Apparently not, as I can see it in a testcase now.  Not sure where I

> > made the mistake.  But does this cause any actual problems?  I didn't

> > see any problems with my gcc testsuite run.

>

> elflint is complaining, and that breaks the elfutils testsuite.


I checked in a fix.  It turned out to be pretty simple, and looks
oddly familiar.  I think I might have accidentally checked in the
wrong version of the patch before.
https://sourceware.org/ml/binutils/2018-10/msg00100.html

I didn't check against elfutils, but for a simple hello world program
I get an empty .tdata section before the patch, and none after the
patch.

Jim

Patch

diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
index 934704a87e..4be3eceda8 100644
--- a/bfd/elfnn-riscv.c
+++ b/bfd/elfnn-riscv.c
@@ -353,7 +353,8 @@  riscv_elf_create_dynamic_sections (bfd *dynobj,
     {
       htab->sdyntdata =
 	bfd_make_section_anyway_with_flags (dynobj, ".tdata.dyn",
-					    SEC_ALLOC | SEC_THREAD_LOCAL);
+					    (SEC_ALLOC | SEC_THREAD_LOCAL
+					     | SEC_LINKER_CREATED));
     }
 
   if (!htab->elf.splt || !htab->elf.srelplt || !htab->elf.sdynbss