Page MenuHomePhabricator

[Linker] Fix crash handling appending linkage
ClosedPublic

Authored by rafauler on Mar 19 2019, 10:32 AM.

Details

Summary

When linking two llvm.used arrays, if the resulting merged
array ends up with duplicated elements (with the same name) but with
different types, the IRLinker was crashing. This was supposed to be
legal, as the IRLinker bitcasts elements to match types in these
situations.

This bug was exposed by D56928 in clang to support attribute used
in member functions of class templates. Crash happened when self-hosting
with LTO. Since LLVM depends on attribute used to generate code
for the dump() method, ubiquitous in the code base, many input bc
had a definition of this method referenced in their llvm.used array.
Some of these classes got optimized, changing the type of the first
parameter (this) in the dump method, leading to a scenario with a
pool of valid definitions but some with a different type, triggering
this bug.

This is a memory bug: ValueMapper depends on (calls) the materializer
provided by IRLinker, and this materializer was freely calling RAUW
methods whenever a global definition was updated in the temporary merged
output file. However, replaceAllUsesWith may or may not destroy
constants that use this global. If the linked definition has a type
mismatch regarding the new def and the old def, the materializer would
bitcast the old type to the new type and the elements of the llvm.used
array, which already uses bitcast to i8*, would end up with elements
cascading two bitcasts. RAUW would then indirectly call the
constantfolder to update the constant to the new ref, which would,
instead of updating the constant, destroy it to be able to create
a new constant that folds the two bitcasts into one. The problem is that
ValueMapper works with pointers to the same constants that may be
getting destroyed by RAUW. Obviously, RAUW can update references in the
Module to do not use the old destroyed constant, but it can't update
ValueMapper's internal pointers to these constants, which are now
invalid.

The approach here is to move the task of RAUWing old definitions
outside of the materializer.

Test Plan:
Added LIT test case, tested clang self-hosting with D56928 and
verified it works

Diff Detail

Repository
rL LLVM

Event Timeline

rafauler created this revision.Mar 19 2019, 10:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2019, 10:32 AM
Herald added a subscriber: steven_wu. · View Herald Transcript
efriedma added inline comments.Mar 19 2019, 12:44 PM
lib/Linker/IRMover.cpp
1059 ↗(On Diff #191346)

Why is the call to std::reverse() necessary? There shouldn't be any other code touching the worklist, so you can just iterate over it normally.

1067 ↗(On Diff #191346)

ValueMap and AliasValueMap use raw pointers for keys. Do we need to update them if one of those pointers is deleted?

1396 ↗(On Diff #191346)

I guess you can just flush the worklist here because the mapValue() invariant only applies inside a single call?

rafauler marked 3 inline comments as done.Mar 19 2019, 3:01 PM

Thanks for your feedback, Eli

lib/Linker/IRMover.cpp
1059 ↗(On Diff #191346)

Indeed, I'll fix this

1067 ↗(On Diff #191346)

This is handled in Value's destructor

1396 ↗(On Diff #191346)

That's correct

rafauler updated this revision to Diff 191404.Mar 19 2019, 3:02 PM

Eli's suggestions

efriedma added inline comments.Mar 19 2019, 3:21 PM
lib/Linker/IRMover.cpp
1067 ↗(On Diff #191346)

I don't follow; how could ~Value() possibly find the key of a ValueMap? (The values are ValueHandles, but that's separate.)

rafauler marked an inline comment as done.Mar 19 2019, 3:32 PM
rafauler added inline comments.
lib/Linker/IRMover.cpp
1067 ↗(On Diff #191346)

Isn't it the opposite: the values are arbitrary, and the keys are ValueHandles? From ValueMap.h:179:

auto MapResult = Map.insert(std::make_pair(Wrap(KV.first), KV.second));

Wrap() installs the callback paraphernalia that updates the map on deletions/ RAUWs at ValueMapCallbackVH::allUsesReplacedWith / deleted

efriedma accepted this revision.Mar 19 2019, 3:46 PM

LGTM

lib/Linker/IRMover.cpp
1067 ↗(On Diff #191346)

You're right, sorry, I misread the code.

This revision is now accepted and ready to land.Mar 19 2019, 3:46 PM
This revision was automatically updated to reflect the committed changes.