Bug 24673 - [RISCV] -fPIC -pie and -fPIC -no-pie create unexpected R_RISCV_NONE R_RISCV_DTPMOD64 relocations
Summary: [RISCV] -fPIC -pie and -fPIC -no-pie create unexpected R_RISCV_NONE R_RISCV_D...
Status: NEW
Alias: None
Product: binutils
Classification: Unclassified
Component: ld (show other bugs)
Version: 2.33
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks: 24676
  Show dependency treegraph
 
Reported: 2019-06-12 15:36 UTC by Fangrui Song
Modified: 2020-04-20 21:19 UTC (History)
3 users (show)

See Also:
Host:
Target: riscv64-*-*
Build:
Last reconfirmed: 2019-06-12 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Fangrui Song 2019-06-12 15:36:47 UTC
static __thread int a;
int foo() {return a;}
int main() {}

riscv-gcc -fuse-ld=bfd -fPIC -no-pie d.c -o d
readelf -r d

Relocation section '.rela.dyn' at offset 0x378 contains 1 entry:
    Offset             Info             Type               Symbol's Value  Symbol's Name + Addend
0000000000000000  0000000000000000 R_RISCV_NONE                              0

riscv-gcc -fuse-ld=bfd -fPIC -pie d.c -o d
readelf -r d

Relocation section '.rela.dyn' at offset 0x458 contains 9 entries:
...
0000000000001978  0000000000000007 R_RISCV_TLS_DTPMOD64                      0


There is no correctness issue.
1) In both cases, the DTPREL GOT slot is correctly pre-populated.
2) In the -fPIC -no-pie case, the DTPMOD GOT slot is pre-populated as 1.

But the dynamic relocations are unexpected and made me wonder if there is a bug :)
Comment 1 Jim Wilson 2019-06-12 21:21:51 UTC
The DTPMOD64 reloc is required for a shared library, and a PIE program is basically a shared library with a few differences.  So this is probably a failure to distinguish between pie and shared, and optimize them differently.

With this completely untested patch
diff --git a/bfd/elfnn-riscv.c b/bfd/elfnn-riscv.c
index 69cadaa..ed8b9f1 100644
--- a/bfd/elfnn-riscv.c
+++ b/bfd/elfnn-riscv.c
@@ -2182,7 +2182,7 @@ riscv_elf_relocate_section (bfd *output_bfd,
 
              /* The GOT entries have not been initialized yet.  Do it
                 now, and emit any relocations.  */
-             if ((bfd_link_pic (info) || indx != 0)
+             if ((bfd_link_dll (info) || indx != 0)
                  && (h == NULL
                      || ELF_ST_VISIBILITY (h->other) == STV_DEFAULT
                      || h->root.type != bfd_link_hash_undefweak))

I now get a R_RISCV_NONE for both the no-pie and pie cases, and still get a DTPMOD64 reloc for the shared case.  This also makes the RISC-V code look more like the MIPS code, so is probably correct.

I see that there are no uses of bfd_link_dll in elfnn-riscv.c currently, so there are probably other similar cases to fix.

I haven't looked at why we get the R_RISCV_NONE yet.
Comment 2 Jim Wilson 2019-06-12 23:40:14 UTC
The issue with the R_RISCV_NONE appears to be that we are pre-allocating space for dynamic relocs, and accidentally allocating one more than we need.  This space is apparently cleared someplace.  So it ends up looking like we have a R_RISCV_NONE reloc in the extra unused reloc space.

The exact sequence appears to be we have a R_RISCV_TLS_GD_HI20 reloc, which
generates a call to riscv_elf_record_got_reference, which increments
elf_local_got_refcounts.  It also calls riscv_elf_record_tls_type which sets the tls type to GOT_TLS_TD.  Then in riscv_elf_size_dynamic_sections, the fact that local_got was set for a symbol, and the fact that the tls type is GOT_TLS_TD, causes us to add one reloc to the srelgot size.  But we never write a reloc into that space, so that ends up being the mysterious R_RISCV_NONE at the end.

Fixing this looks complicated, and may need a rewrite to allocate space for relocs when we actually generate relocs.  It looks like the MIPS port works this way with the mips_elf_allocate_dynamic_relocations calls scattered around the code.
Comment 3 Jim Wilson 2019-06-13 17:03:27 UTC
Via IRC, elfnn-riscv.c circa line 563 has
        case R_RISCV_TLS_GOT_HI20:
          if (bfd_link_pic (info))
            info->flags |= DF_STATIC_TLS;
where this should be bfd_link_dll instead of bfd_link_pic.

The mips port uses bfd_link_pic.  The ppc64 port uses bfd_link_dll.  The x86_64 port uses !bfd_link_executable.  Maybe this is ABI dependent, or maybe lots of ports are getting it wrong.  Anyways, I will have to look at this later.
Comment 4 Alan Modra 2019-06-14 02:20:31 UTC
bfd_link_dll and !bfd_link_executable are equivalent when used to set DF_STATIC_TLS in ppc64 and x86 check_relocs.  bfd_link_relocatable is excluded earlier.

DF_STATIC_TLS is really only relevant for dlopen'd binaries, and so for glibc systems the flag need only be set for shared libraries.  (I can't see that it would be wrong to always set the flag on detecting thread pointer relative accesses as used by initial exec or local exec tls models.)