Page MenuHomePhabricator

[ELF] Make the rule to create relative relocations in a writable section stricter
ClosedPublic

Authored by MaskRay on Jun 10 2019, 10:34 PM.

Details

Summary

The current rule is loose: !Sym.IsPreemptible || Expr == R_GOT.

When the symbol is non-preemptable, this allows absolute relocation
types with smaller numbers of bits, e.g. R_X86_64_{8,16,32}. They are
disallowed by ld.bfd and gold, e.g.

ld.bfd: a.o: relocation R_X86_64_8 against `.text' can not be used when making a shared object; recompile with -fPIC

This patch:

a) Add TargetInfo::SymbolicRel to represent relocation types that resolve to a
symbol value (e.g. R_AARCH_ABS64, R_386_32, R_X86_64_64).

As a side benefit, we currently (ab)use GotRel (R_*_GLOB_DAT) to resolve
GOT slots that are link-time constants. Since we now use Target->SymbolRel
to do the job, we can remove R_*_GLOB_DAT from relocateOne() for all targets.
R_*_GLOB_DAT cannot be used as static relocation types.

b) Change the condition to !Sym.IsPreemptible && Type != Target->SymbolicRel || Expr == R_GOT.

Some tests are caught by the improved error checking (ld.bfd/gold also
issue errors on them). Many misuse .long where .quad should be used
instead.

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Jun 10 2019, 10:34 PM
Herald added a project: Restricted Project. · View Herald Transcript
MaskRay updated this revision to Diff 203978.Jun 10 2019, 10:41 PM

Update a comment

This looks good to me. However, I think the name SymbolicRel does not make it immediately obvious to me it should be used for. Maybe add a comment to the member?

ELF/Target.h
98 ↗(On Diff #203978)

Maybe something like 'AbsPtrRel'? To me symbolic does not necessarily imply a elf_addr size relocation against a symbol.

grimar added inline comments.Jun 11 2019, 2:02 AM
ELF/Relocations.cpp
971 ↗(On Diff #203978)

This comment is a bit unclear now. It says "Copy relocations ... are only possible if we are creating an executable",
what made sence before when the condition was if (Config->Shared), but now it also allows
Config->Pie, which stands for position independent executable.

MaskRay marked an inline comment as done.Jun 11 2019, 3:24 AM
MaskRay added inline comments.
ELF/Target.h
98 ↗(On Diff #203978)

I'd like to stick with the current name. AbsPtrRel sounds to me it has something to do with pointers but it doesn't. I have to confess this name was inspired by musl defined general dynamic relocation types:

http://git.musl-libc.org/cgit/musl/tree/arch/x86_64/reloc.h#n7
#define REL_SYMBOLIC R_X86_64_64

Symbolic means it takes the address of a symbol at runtime.

		case REL_SYMBOLIC:
		case REL_GOT:
		case REL_PLT:
			*reloc_addr = sym_val + addend;
			break;
		case REL_RELATIVE:
			*reloc_addr = (size_t)base + addend;
			break;
MaskRay updated this revision to Diff 204004.Jun 11 2019, 3:29 AM

Add comment about

(Config->Pie && Expr == R_ABS && Type != Target->SymbolicRel)) {
grimar added a comment.EditedJun 11 2019, 3:45 AM

This looks good to me, I have no more comments.

ruiu accepted this revision.Jun 11 2019, 5:27 AM

LGTM

This revision is now accepted and ready to land.Jun 11 2019, 5:27 AM
This revision was automatically updated to reflect the committed changes.

This commit has caused the arm lld buildbot to fail. I can reproduce it on an Arm machine and a bisect found this one as the change. If I revert this change then the specific failure goes away. Unfortunately I can't tell you exactly what is wrong yet, the failure is in the libclang unit tests which are a shared object linked by LLD. I haven't yet had a chance to isolate the segfault and work out what is causing it. I'll keep looking to see if I can work out what specifically is wrong.

First failure: http://lab.llvm.org:8011/builders/clang-cmake-armv8-lld/builds/1558

A look at the differences between libclang.so with and without the patch is that what used to be a R_ARM_RELATIVE relocation is now a R_ARM_ABS32 relocation to an undefined symbol. The latter will likely resolve to 0. All of the difference is concentrated in the .init_array section which uses the R_ARM_TARGET1 relocation, which is treated as R_ARM_ABS32 by default but with an option be treated as R_ARM_REL32 (some embedded systems use offsets rather than addresses).
For example:
Good

Contents of section .init_array:
 2d21008 c980c300 c46fb301 f89ab301 acfbb601  .....o..........
 2d21018 8067b801 8040b901 403fbf01 00bec101  .g...@..@?......
 2d21028 c0bdc201 c873c301 8c52c501 7810c601  .....s...R..x...
...
02d21008  00000017 R_ARM_RELATIVE   
02d2100c  00000017 R_ARM_RELATIVE   
02d21010  00000017 R_ARM_RELATIVE   
02d21014  00000017 R_ARM_RELATIVE   
02d21018  00000017 R_ARM_RELATIVE   
02d2101c  00000017 R_ARM_RELATIVE   
02d21020  00000017 R_ARM_RELATIVE   
02d21024  00000017 R_ARM_RELATIVE   
02d21028  00000017 R_ARM_RELATIVE

Bad:

Contents of section .init_array:
 2d21008 c980c300 00000000 00000000 00000000  ................
 2d21018 00000000 00000000 00000000 00000000  ................
 2d21028 00000000 00000000 00000000 00000000  ................
...
02d21008  00000017 R_ARM_RELATIVE
02d2100c  00000002 R_ARM_ABS32      
02d21010  00000002 R_ARM_ABS32      
02d21014  00000002 R_ARM_ABS32      
02d21018  00000002 R_ARM_ABS32      
02d2101c  00000002 R_ARM_ABS32      
02d21020  00000002 R_ARM_ABS32      
02d21024  00000002 R_ARM_ABS32      
02d21028  00000002 R_ARM_ABS32

So far I've not been able to make a simpler reproducer, as can be seen from the first entry in the .init_array there must be some special case.

Reproducer:
cat init.cpp

template <class T>
struct Bar {    
    T X;
    Bar(T X) : X(X) {}
};

Foo<int> F(0);
Bar<int> B(1);

int func(void) {
    return F.X + B.X;
}
clang --target=arm-linux-gnueabihf init.cpp -fpic --shared -o init.so  -fuse-ld=lld
llvm-readobj --relocs init.so 

File: init.so
Format: ELF32-arm-little
Arch: arm
AddressSize: 32bit
LoadName: 
Relocations [
  Section (6) .rel.dyn {
    0x2000 R_ARM_RELATIVE - 0x0
    0x2008 R_ARM_RELATIVE - 0x0
    0x3000 R_ARM_RELATIVE - 0x0
    0x2004 R_ARM_ABS32 - 0x0
    0x20D8 R_ARM_GLOB_DAT __gmon_start__ 0x0
    0x20DC R_ARM_GLOB_DAT _ITM_deregisterTMCloneTable 0x0
    0x20E0 R_ARM_GLOB_DAT _ITM_registerTMCloneTable 0x0
    0x20E4 R_ARM_GLOB_DAT __cxa_finalize@GLIBC_2.4 0x0
    0x20E8 R_ARM_GLOB_DAT _Jv_RegisterClasses 0x0
    0x20EC R_ARM_GLOB_DAT B 0x0
    0x20F0 R_ARM_GLOB_DAT F 0x0
  }
  Section (8) .rel.plt {
    0x3010 R_ARM_JUMP_SLOT __gmon_start__ 0x0
    0x3014 R_ARM_JUMP_SLOT __cxa_finalize@GLIBC_2.4 0x0
    0x3018 R_ARM_JUMP_SLOT _ZN3FooIiEC2Ei 0x0
    0x301C R_ARM_JUMP_SLOT _ZN3BarIiEC2Ei 0x0
  }
]

Note the 0x2004 R_ARM_ABS32 - 0x0 which is is in the .init_array section.

peter.smith added inline comments.Jun 12 2019, 10:51 AM
lld/trunk/ELF/Relocations.cpp
922

I'm not sure that there is always just one relocation per Target that is a SymbolicRel. On Arm there are a few implementation defined relocations like R_ARM_TARGET1 that can be R_ARM_ABS32 or R_ARM_REL32 depending on the platform. I think that this needs to be a callback to Target so that more than one relocation can apply.

927

Can we guarantee that Sym is in the dynamic symbol table after we have narrowed the relocation types accepted?

@peter.smith Sorry I just noticed the build bot was failing. Just commited a workaround rL363236

ld.bfd's rules to resolve absolute relocation types to R_*_RELATIVE are scattered. I do want to find a unified approach to handle them.
I'll do some experiments with R_ARM_TARGET1 (I didn't know this relocation type). PPC64 may have a similar relocation type that I'm not aware of...

Was difficult to spot what was wrong with the bot as there were lots of commits and a non specific error message.

I'll have a think to see if there are any other relocation types besides R_ARM_TARGET1 that LLD supports that can be made into a R_ARM_RELATIVE. I don't know PPC very well unfortunately. The other case I thought of that could be difficult is ILP32, where even on something like X86_64, or AArch64 the symbolic rel for a pointer would be the 32-bit relocation. I know LLD doesn't support the AArch64 ILP32 so this isn't a problem right now.

In theory R_ARM_TARGET2 (references to TypeInfo in read-only exception handling tables) can be made into an alias for R_ARM_ABS32 with Target2Policy::Abs (--target2=abs). That option is intended for embedded systems without dynamic linking (the default is to treat it as R_ARM_GOT_PREL). So in theory it is possible, although unlikely and would arguably be better triggering an Error message if it were used.

The R_ARM_TARGET1 is a result of Arm's embedded past where there were several toolchains with subtly different ABIs, when it came to defining the official ABI there were some implementation defined points. In one environment the libraries static initialisation code used offsets to the initialization function, in another it used absolute addresses of the initialization function, the R_ARM_TARGET1 permitted a compiler to support both choices as the linker could choose depending . For Linux R_ARM_TARGET1 is essentially an alias for R_ARM_ABS32.

I'll have a think to see if there are any other relocation types besides R_ARM_TARGET1 that LLD supports that can be made into a R_ARM_RELATIVE. I don't know PPC very well unfortunately. The other case I thought of that could be difficult is ILP32, where even on something like X86_64, or AArch64 the symbolic rel for a pointer would be the 32-bit relocation. I know LLD doesn't support the AArch64 ILP32 so this isn't a problem right now.

Taken a look; on the Arm side I can't find anything other than R_ARM_TARGET1 and to a lesser extent R_ARM_TARGET2 that need to be considered as an alias for R_ARM_ABS32, when considering whether R_ARM_RELATIVE can be used.