[PATCH] D59552: [Linker] Fix crash handling appending linkage

[Rafael Auler via Phabricator via llvm-commits]

rafauler created this revision.
rafauler added reviewers: pcc, efriedma, mehdi_amini, dexonsmith, tejohnson.
Herald added a subscriber: steven_wu.
Herald added a project: LLVM.

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 <https://reviews.llvm.org/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 <https://reviews.llvm.org/D56928> and
verified it works


Repository:
  rL LLVM

https://reviews.llvm.org/D59552

Files:
  lib/Linker/IRMover.cpp
  test/LTO/Resolution/X86/Inputs/appending-var-2.ll
  test/LTO/Resolution/X86/appending-var.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D59552.191346.patch
Type: text/x-patch
Size: 4369 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190319/c8f414e2/attachment.bin>