Page MenuHomePhabricator

[RTDYLD] support absolute relocations where needed
ClosedPublic

Authored by vtjnash on Nov 3 2020, 2:47 PM.

Details

Summary

These appear in some sections, such as DWARF tables, since
RuntimeDyldELF explicitly maps to this as a sentinel value:
https://github.com/llvm/llvm-project/blob/29d1fba7b5335d969e3e5daa84b7a25cd1fa75ef/llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.cpp#L1199

That could then be a source of problems if it tried to examine these
sections (for example, with either setProcessAllSections(true) or ORCv2 on i686).

Replaces https://reviews.llvm.org/D89241

Diff Detail

Event Timeline

vtjnash created this revision.Nov 3 2020, 2:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 3 2020, 2:47 PM
vtjnash requested review of this revision.Nov 3 2020, 2:47 PM
vtjnash edited the summary of this revision. (Show Details)
vtjnash added a subscriber: Restricted Project.
vchuravy accepted this revision.Nov 3 2020, 3:01 PM

Looks good from my side. @lhames should sign off though.

Is there a good standalone test that we could add?

This revision is now accepted and ready to land.Nov 3 2020, 3:01 PM
lhames accepted this revision.Nov 3 2020, 7:15 PM

Hi Jameson, Valentin,

Valentin -- Sorry I didn't get to your initial review earlier!

Jameson -- Thanks for catching the DWARF / sentinel value issue. I would have missed that.

This looks great to me. I think the original problem was misapplied relocations in the presence of empty symbols, right? In that case the correct way to test would be to write an assembly file that triggers generation of one empty symbol, plus a relocation that's expected to fail in the presence of that empty symbol. Then you can use llvm-rtdyld in -verify mode to verify that the relocation was handled correctly. See https://github.com/llvm/llvm-project/blob/master/llvm/test/ExecutionEngine/RuntimeDyld/X86/ELF-large-pic-relocations.s for an example of this style of test.

I don't know exactly which relocations were failing for you in the presence of empty symbols, but say it was plain R_X86_64_64s. You could write a program like this to generate one:

void bad_target(void) {}

void good_target(void) {}

void test_relocation(void) {
  good_target();
}

Convert it to assembly with:

% clang -mcmodel=large --target=x86_64-unknown-linux -fno-asynchronous-unwind-tables -S -o ELF_empty_symbol_name.s input.c

Then add an # rtdyld-check line to verify that the relocation applies correctly.

# RUN: rm -rf %t && mkdir -p %t
# RUN: llvm-mc -triple=x86_64-unknown-freebsd -filetype=obj -o %t/ELF_empty_symbol_name.o %s
# RUN: llvm-rtdyld -triple=x86_64-unknown-linux -verify -check=%s %t/ELF_empty_symbol_name.o

	.text
	.globl	bad_target
	.p2align	4, 0x90
	.type	bad_target,@function
bad_target:
	retq
.Lfunc_end0:
	.size	bad_target, .Lfunc_end0-bad_target

	.globl	good_target
	.p2align	4, 0x90
	.type	good_target,@function
good_target:
	retq
.Lfunc_end1:
	.size	good_target, .Lfunc_end1-good_target

	.globl	test_relocation
	.p2align	4, 0x90
	.type	test_relocation,@function

# rtdyld-check: decode-operand(test_relocation) = good_target
test_relocation:
	movabsq	$good_target, %rax
	callq	*%rax
	retq
.Lfunc_end2:
	.size	test_relocation, .Lfunc_end2-test_relocation

	.ident	"Apple clang version 12.0.0 (clang-1200.0.32.27)"
	.section	".note.GNU-stack","",@progbits
	.addrsig
	.addrsig_sym good_target

Then add whatever assembly / DWARF you need to trigger the bug and verify that the test fails. Then apply your patch and verify that it passes again.

If it's too fiddly I don't mind this going in without a test case right now -- I think extra cycles would be just as well spent bringing up / testing JITLink for ELF. We should make sure we don't replicate this issue by using empty strings as sentinels there.

I'll be on the road a lot the next few days: If you have any follow-up questions in the short term discord will be the best way to get hold of me. Otherwise I'll check back in here in a week or so.

If it's too fiddly I don't mind this going in without a test case right now -- I think extra cycles would be just as well spent bringing up / testing JITLink for ELF. We should make sure we don't replicate this issue by using empty strings as sentinels there.

Yeah, I'm thinking this is not worth chasing too much with tests, and focusing instead on JITLink. I was mostly interested in getting this working so I can start using ORCv2 and then incrementally testing the frontiers (ie JITLink). In this case, RTDyld seemed to be thinking it was a R_X86_64_32 relocation, so the main goal is to just avoid loading that section (which will abort) or doing anything too regrettable (like trying to record the address of it) as we examine the relocations list.

(Aside on this detail: I'm not sure precisely how this DWARF relocation is supposed to be represented in the struct here, but I vaguely know it should be a 32-bit section-relative symbol that's computed statically, so that no relocation needs to actually happen dynamically, regardless of how the sections eventually get laid out in memory. The debugger or unwinder is responsible for computing the real address on the fly.)

This revision was landed with ongoing or failed builds.Nov 6 2020, 11:24 AM
This revision was automatically updated to reflect the committed changes.