This is an archive of the discontinued LLVM Phabricator instance.

Fix -emit-reloc against local symbols.
ClosedPublic

Authored by ruiu on Apr 4 2019, 10:18 PM.

Details

Summary

Previously, we drop symbols starting with .L from the symbol table, so
if there is a relocation that refers a .L symbol, it ended up
referencing a null -- which happened to be interpreted as an absolute
symbol.

This patch copies all symbols including local ones if -emit-reloc is
given.

Fixes https://bugs.llvm.org/show_bug.cgi?id=41385

Event Timeline

ruiu created this revision.Apr 4 2019, 10:18 PM
Herald added a project: Restricted Project. · View Herald Transcript
E5ten added a subscriber: E5ten.Apr 5 2019, 5:18 AM
kees accepted this revision.Apr 5 2019, 8:53 AM

I can confirm this fixes the Linux kernel relocation visibility problem I saw. Thank you!

This revision is now accepted and ready to land.Apr 5 2019, 8:53 AM
This revision was automatically updated to reflect the committed changes.
grimar added inline comments.Apr 15 2019, 2:00 AM
lld/trunk/test/ELF/emit-relocs-mergeable2.s
6 ↗(On Diff #194095)

I would suggest adding the header for these values and a short description of what this patch does.
(I know we are missing the descriptions often, but ideally, I think we might want to add a description for each of
our test. At least that is something I have in my TODO list for LLD and it is really useful sometimes).

grimar added inline comments.Apr 15 2019, 2:05 AM
lld/trunk/test/ELF/emit-relocs-mergeable2.s
6 ↗(On Diff #194095)

what this patch does -> what this test checks

ruiu marked an inline comment as done.Apr 15 2019, 2:19 AM
ruiu added inline comments.
lld/trunk/test/ELF/emit-relocs-mergeable2.s
6 ↗(On Diff #194095)

Honestly, unlike code, I don't think we need to explain the test in details. If you will need to change this test in the future, for example, it is very likely that you are changing the code and already understand what is expected.

grimar added inline comments.Apr 15 2019, 5:38 AM
lld/trunk/test/ELF/emit-relocs-mergeable2.s
6 ↗(On Diff #194095)

I disagree here. It is sometimes hard to realize what the test does. I do not see any reason why we can't add a short description for each.
Perhaps the problem is that I still do not understand what this test do? And then this is what I am talking about: each test should have a clear description.

MaskRay added inline comments.Apr 16 2019, 7:44 AM
lld/trunk/test/ELF/emit-relocs-mergeable2.s
6 ↗(On Diff #194095)

I agree with George that a short description will be helpful.

  1. help understand the code
  2. convenience for people who make layout changes and have to adjust many tests