This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Fix wrapping symbols produced during LTO codegen
ClosedPublic

Authored by smeenai on Apr 19 2022, 6:26 PM.

Details

Summary

We were previously not correctly wrapping symbols that were only
produced during LTO codegen and unreferenced before then, or symbols
only referenced from such symbols. The root cause was that we weren't
marking the wrapped symbol as used if we only saw the use after LTO
codegen, leading to the failed wrapping.

Fix this by explicitly tracking whether a symbol will become referenced
after wrapping is done. We can use this property to tell LTO to preserve
such symbols, instead of overload isUsedInRegularObj for this purpose.
Since we're no longer setting isUsedInRegularObj for all symbols which
will be wrapped, its value at the time of performing the wrapping in the
symbol table will accurately reflect whether the symbol was actually
used in an object (including in an LTO-generated object), and we can
propagate that value to the wrapped symbol and thereby ensure we wrap
correctly.

This incorrect wrapping was the only scenario I was aware of where we
produced an invalid PLT relocation, which D123985 started diagnosing,
and with it fixed, we lose the test for that diagnosis. I think it's
worth keeping the diagnosis though, in case we run into other issues in
the future which would be caught by it.

Fixes PR50675.

Diff Detail

Event Timeline

smeenai created this revision.Apr 19 2022, 6:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2022, 6:26 PM
smeenai requested review of this revision.Apr 19 2022, 6:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2022, 6:26 PM
smeenai updated this revision to Diff 423781.Apr 19 2022, 6:30 PM

Add comment in SymbolTable::wrap

smeenai updated this revision to Diff 423811.Apr 19 2022, 10:28 PM

Make git-clang-format happy

smeenai edited the summary of this revision. (Show Details)Apr 19 2022, 10:58 PM
smeenai updated this revision to Diff 423828.Apr 20 2022, 12:10 AM

Rebase on top of test removal from previous diff

Thanks for the patch. I think it is an important improvement.

Since we're no longer setting isUsedInRegularObj for all symbols which will be wrapped, its value at the time of performing the wrapping in the symbol table will accurately reflect whether the symbol was actually ...

I always need to wrap my head around when thinking about --wrap :-) And I did not find a good way to fix the inelegant condition

if (!real->isUsedInRegularObj && sym->isUndefined())
    sym->isUsedInRegularObj = false;

The new one is better! Let me play with this a bit...

I am also curious whether you want to do anything with D33621: the chunk of code is a bit of a hack

if (Res.LinkerRedefined)
  GV->setLinkage(GlobalValue::WeakAnyLinkage);
MaskRay added inline comments.Apr 22 2022, 2:01 PM
lld/ELF/Symbols.h
137

The name may be too long. We don't need to pursue perfect grammar, we can just call it referencedAfterWrap

MaskRay added inline comments.Apr 22 2022, 2:11 PM
lld/ELF/Symbols.h
131

Is this sentence stale now?

MaskRay added inline comments.Apr 22 2022, 2:15 PM
lld/ELF/SymbolTable.cpp
47

It will be good to have a comment for the if (sym->isUndefined()) condition.

Thanks for the patch. I think it is an important improvement.

Thanks for the review!

I am also curious whether you want to do anything with D33621: the chunk of code is a bit of a hack

if (Res.LinkerRedefined)
  GV->setLinkage(GlobalValue::WeakAnyLinkage);

Yeah, I've been meaning to look into that as well ... there's at least one issue where the logic to restore the original binding of the symbol in the linker got dropped (https://github.com/llvm/llvm-project/issues/51346). I need to understand the LTO logic better first though.

lld/ELF/SymbolTable.cpp
47

Note that I'm tightening the condition further in D124065, which is a follow-up to this diff. Would you prefer adding a comment here and then changing it in D124065, or just adding the comment in D124065? (I think it'll be easier to explain the logic of the conditional with the change in D124065.)

lld/ELF/Symbols.h
131

Hmm. In a way it is because referencedAfterWrap will be used for the retaining now, but referenced on the pre-wrapped symbol is still used to set referencedAfterWrap on the wrapped symbol. I think it's fine to remove the sentence though, since the referencedAfterWrap comment also mentions that property being set based on if the original symbol is referenced.

137

Sure, I'll change it.

MaskRay added inline comments.Apr 22 2022, 3:14 PM
lld/ELF/SymbolTable.cpp
47

Ah adding the comment to D124065 will be good.

Are you splitting the two patches for easy bisection? I.e. in case the second patch breaks something?

smeenai added inline comments.Apr 22 2022, 3:32 PM
lld/ELF/SymbolTable.cpp
47

Yup, I wanted to keep the changes isolated so that each patch was an individual behavior change (i.e. this one only fixes the wrapping behavior and doesn't change any existing tests, and the next one changes symbol retention and needs an existing test to be modified).

smeenai updated this revision to Diff 424640.Apr 22 2022, 3:39 PM

Address review comments

smeenai marked 4 inline comments as done.Apr 22 2022, 3:39 PM
MaskRay accepted this revision.Apr 22 2022, 3:45 PM
MaskRay added inline comments.
lld/test/ELF/lto/wrap-unreferenced-before-codegen.test
19

Prefer to add a space after callq

This revision is now accepted and ready to land.Apr 22 2022, 3:45 PM
smeenai updated this revision to Diff 424649.Apr 22 2022, 3:59 PM
smeenai marked an inline comment as done.

Add space after callq

This revision was automatically updated to reflect the committed changes.
lld/ELF/Symbols.h