This is an archive of the discontinued LLVM Phabricator instance.

LLD: Avoid segfault with --emit-relocs
AbandonedPublic

Authored by dstolfa on Mar 17 2018, 12:44 PM.

Details

Summary

When passing an --emit-relocs flag to lld, there seems to be a possibility for the output section to be NULL. This causes lld to crash which results in the executable not being generated. This patch works around this issue by simply checking if the output section exists and returns "" as the name if it doesn't. The check of the return value then prevents propagation of "" in other bits of the code.

I've inspected a number of resulting ELF files generated with this and it looks correct and it breaks no additional tests.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

dstolfa created this revision.Mar 17 2018, 12:44 PM
dstolfa updated this revision to Diff 138827.EditedMar 17 2018, 1:53 PM

Generate a minimal test case for the breakage. It seems like the root cause of this are the cfi directives.

arichardson added inline comments.Mar 17 2018, 3:40 PM
ELF/Writer.cpp
101

It seems like the problem here is that when this is called with .rela.eh_frame getRelocatedSection() returns an EhInputSection where ->getParent() returns null.

If I link the test file with ld.bfd it contains relocations so we should probably be emitting the same output and not just omit the relocations:

Relocations [
  Section (3) .rela.eh_frame {
    0x4000A0 R_X86_64_PC32 .text 0x0
  }
]
test/ELF/emit-relocs-freebsd.s
2

Possibly add a llvm-readobj invocation here to show that the input file contains relocations:

Relocations [
  Section (4) .rela.eh_frame {
    0x20 R_X86_64_PC32 .text 0x0
  }
]
6

I don't there is much point in testing the aliases for --emit-relocs here. It is already done in emit-relocs.s

8

This line is not actually running llvm-readobj

24

I don't think we need to check the symbols in this test. This is about not crashing on relocations in .eh_frame and --emit-relocs

dstolfa retitled this revision from LLD: Avoid segfault on FreeBSD with --emit-relocs to LLD: Avoid segfault with --emit-relocs.EditedMar 18 2018, 6:00 PM
dstolfa edited the summary of this revision. (Show Details)

I've verified the breakage on Linux as well. It seems to be caused by InX::EhFrame->getParent() being nullptr when EmitRelocs is set. This is because near the start of the InputSection vector, there are additional input sections that end up pointing to InX::EhFrame as the parent and getOutputSectionName returns nullptr.

dstolfa updated this revision to Diff 138876.Mar 18 2018, 6:23 PM

Bump the diff to reflect things that @arichardson has helped me find.

The relocations are still not checked in the test itself because it's not clear what the section number should be yet (I'm getting different outputs from different linkers at the moment).

This diff removes the unnecessary duplicate runs that are checked by emit-relocs.s already and renames emit-relocs-freebsd.s to emit-relocs-eh.s as the breakage is not unique to FreeBSD. It does not provide a correct fix yet and still just works around the issue by simply ignoring the problematic entries.

dstolfa abandoned this revision.Mar 19 2018, 6:52 AM

Closing this due to a better patch in https://reviews.llvm.org/D44622.

dstolfa updated this revision to Diff 139034.EditedMar 19 2018, 5:35 PM

Fix the issue and add a sensible test.

EDIT: This breaks a couple of tests (my environment didn't update the first time around).

dstolfa updated this revision to Diff 139070.Mar 19 2018, 8:33 PM

I believe that this fully fixes the issue. It adds a check for Config->Relocatable and checks for OutsecName correctly.

All tests pass.

dstolfa marked an inline comment as done.Mar 19 2018, 8:38 PM
dstolfa updated this revision to Diff 139227.Mar 20 2018, 5:06 PM

Import the test case from @grimar's D44679 to verify that this patch works with the linkerscript test as well.

Thanks for thinking of this case!

This was fixed by r328299, right?

dstolfa abandoned this revision.Mar 23 2018, 7:06 PM

This was fixed by r328299, right?

Correct. I seem to have missed the commit when it happened so it remained open. Closing this diff because of the commit.