This is an archive of the discontinued LLVM Phabricator instance.

[ELF][RISCV] Create dummy .sdata for __global_pointer$ if .sdata does not exist
ClosedPublic

Authored by MaskRay on Jun 11 2019, 4:41 AM.

Details

Summary

If .sdata is absent, linker synthesized __global_pointer$ gets a section index of SHN_ABS.

In -pie/-shared mode, lld complains if a PC relative relocation references an absolute symbol (SHN_ABS) but ld.bfd doesn't:

ld.lld: error: relocation R_RISCV_PCREL_HI20 cannot refer to lute symbol: __global_pointer$

To allow lla gp, __global_pointer$ when .sdata is absent, create a dummy .sdata .

Also, change the visibility from STV_HIDDEN to STV_DEFAULT and don't define the symbol for -shared.
This is what ld.bfd does, though I don't understand why it does so.

Event Timeline

MaskRay created this revision.Jun 11 2019, 4:41 AM
Herald added a project: Restricted Project. · View Herald Transcript
ruiu accepted this revision.Jun 11 2019, 5:26 AM

LGTM

This revision is now accepted and ready to land.Jun 11 2019, 5:26 AM
jrtc27 requested changes to this revision.Jun 11 2019, 6:52 AM

GNU ld treats it as undefined:

10: 0000000000000000     0 NOTYPE  GLOBAL DEFAULT  UND __global_pointer$
test/ELF/riscv-gp.s
14

Should be GLOBAL DEFAULT to match GNU ld.

32

GLOBAL DEFAULT again.

This revision now requires changes to proceed.Jun 11 2019, 6:52 AM
MaskRay marked an inline comment as done.Jun 11 2019, 7:27 AM
MaskRay added inline comments.
test/ELF/riscv-gp.s
14

glibc/musl Scrt1.o:

.option push
.option norelax
	lla   gp, __global_pointer$ # R_RISCV_PCREL_HI20+R_RISCV_PCREL_LO12_I

.option pop

Do you know why __global_pointer$ is STB_GLOBAL STV_DEFAULT in ld.bfd?

If a shared object can have .sdata and __global_pointer$, and it does the similar lla (pc relative) to load gp, I think a hidden symbol makes more sense.

In a shared object, STB_GLOBAL STV_DEFAULT symbols are preemptable by default. The usage of lla will imply the symbols are non-preemptable, thus hidden visibility will make more sense.

jrtc27 added inline comments.Jun 11 2019, 7:30 AM
test/ELF/riscv-gp.s
14

The gp register is global for the entire application; the GPREL relaxations only occur for executables. Shared libraries have no __global_pointer$.

GNU ld treats it as undefined:

Yes. That was why I created the patch to fix the musl -fpie -pie link error: ld.lld: error: relocation R_RISCV_PCREL_HI20 cannot refer to absolute symbol: __global_pointer$. This error is lld specific: in -pie/-shared mode, a PC relative relocation to an absolute symbol is not allowed.

In https://groups.google.com/forum/#!searchin/generic-abi/SHN_ABS|sort:date/generic-abi/MC4b4sqnKC0/oQgZxxwMmJAJ, Cary's interpretation of gABI is:

On the contrary, the only interpretation that makes sense to me is that it will not change because of relocation at link time or at load time.

If lld resolves the PC relative relocation statically, the value will be incorrect when the pie or shared object is loaded at runtime because the SHN_ABS symbol's value does not change. The best lld can do is to issue an error.

An alternative is to create a dummy .sdata in musl's Scrt1.o. I don't know how reasonable it is to have lla gp,__global_pointer$ in other code that might not define .sdata.

GNU ld treats it as undefined:

Yes. That was why I created the patch to fix the musl -fpie -pie link error: ld.lld: error: relocation R_RISCV_PCREL_HI20 cannot refer to absolute symbol: __global_pointer$. This error is lld specific: in -pie/-shared mode, a PC relative relocation to an absolute symbol is not allowed.

In https://groups.google.com/forum/#!searchin/generic-abi/SHN_ABS|sort:date/generic-abi/MC4b4sqnKC0/oQgZxxwMmJAJ, Cary's interpretation of gABI is:

On the contrary, the only interpretation that makes sense to me is that it will not change because of relocation at link time or at load time.

If lld resolves the PC relative relocation statically, the value will be incorrect when the pie or shared object is loaded at runtime because the SHN_ABS symbol's value does not change. The best lld can do is to issue an error.

An alternative is to create a dummy .sdata in musl's Scrt1.o. I don't know how reasonable it is to have lla gp,__global_pointer$ in other code that might not define .sdata.

Ah, so GNU ld sets it to 0/UND for -shared, but for -pie it puts it 0x800 past the end of .got:

[ 7] .got              PROGBITS        0000000000002000 001000 000008 08  WA  0   0  8
  11: 0000000000002808     0 NOTYPE  GLOBAL DEFAULT    7 __global_pointer$

You can see the logic BFD is using in ld/emulparams/elf32lriscv-defs.sh.

dalias added a subscriber: dalias.Jun 11 2019, 7:57 AM

I don't immediately see any problem with adding a dummy .sdata, though I think the linker should probably also be able to do this. defining a dummy __global_pointer with SHN_ABS makes no sense. The bfd linker doing this looks like a bug to me, and the bfd linker failing to produce an error when linking it looks like another bug.

MaskRay updated this revision to Diff 204076.Jun 11 2019, 8:13 AM
MaskRay retitled this revision from [ELF][RISCV] Set st_shndx of __global_pointer$ to 1 if .sdata does not exist to [ELF][RISCV] Change __global_pointer$ from STV_HIDDEN to STV_DEFAULT and add test.
MaskRay edited the summary of this revision. (Show Details)
MaskRay removed a subscriber: dalias.

Repurpose this patch since musl will add a dummy .sdata to its Scrt1.o

MaskRay updated this revision to Diff 204089.Jun 11 2019, 9:34 AM
MaskRay retitled this revision from [ELF][RISCV] Change __global_pointer$ from STV_HIDDEN to STV_DEFAULT and add test to [ELF][RISCV] Create dummy .sdata for __global_pointer$ if .sdata does not exist.
MaskRay edited the summary of this revision. (Show Details)

Synthesize .sdata

_GLOBAL_OFFSET_TABLE_ has a similar behavior: it forces the creation of .got.plt or .got

MaskRay added a subscriber: dalias.Jun 11 2019, 9:36 AM
MaskRay updated this revision to Diff 204092.Jun 11 2019, 9:50 AM

Check if __global_pointer$ is referenced.

MaskRay marked 3 inline comments as done.Jun 11 2019, 7:13 PM
MaskRay updated this revision to Diff 204426.Jun 12 2019, 10:56 PM
MaskRay edited the summary of this revision. (Show Details)

Don't define __global_pointer$ for -shared

@jrtc27 Do this patch and D63076 look good now?

jrtc27 accepted this revision.Jun 13 2019, 1:52 PM

@jrtc27 Do this patch and D63076 look good now?

I think this patch is fine from my perspective now.

This revision is now accepted and ready to land.Jun 13 2019, 1:52 PM
This revision was automatically updated to reflect the committed changes.