[CloneModule] Cloned module retains some pointers to original module's objects #47769
Comments
ilovepi
added a commit
to ilovepi/llvm-project
that referenced
this issue
Nov 30, 2023
llvm#70703 pointed out that cloning LLVM modules could lead to miscompiles when using FatLTO. This is due to an existing issue when cloning modules with labels (see llvm#55991 and llvm#47769). Since this can lead to miscompilation, we can avoid cloning the LLVM modules, which was desirable anyway. This patch modifies the EmbedBitcodePass to no longer clone the module or run an input pipeline over it. Further, it make FatLTO always perform UnifiedLTO, so we can still defer the Thin/Full LTO decision to link-time. Lastly, it removes dead/obsolete code related to now defunct options that do not work with the EmbedBitcodePass implementation any longer.
ilovepi
added a commit
that referenced
this issue
Dec 1, 2023
#70703 pointed out that cloning LLVM modules could lead to miscompiles when using FatLTO. This is due to an existing issue when cloning modules with labels (see #55991 and #47769). Since this can lead to miscompilation, we can avoid cloning the LLVM modules, which was desirable anyway. This patch modifies the EmbedBitcodePass to no longer clone the module or run an input pipeline over it. Further, it make FatLTO always perform UnifiedLTO, so we can still defer the Thin/Full LTO decision to link-time. Lastly, it removes dead/obsolete code related to now defunct options that do not work with the EmbedBitcodePass implementation any longer.
|
This has been resolved. See #139223 which introduces a regression test to cover this error case. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Extended Description
When BlockAddresses are used as initializers of GlobalVariables,
and the module is cloned, the BlockAddress constants used as
initializers in the cloned module contain pointers to the
BasicBlocks of the old module rather than the new one.
As far as I can tell, in ValueMapper.cpp, Mapper::flush() attempts
to RAUW the temporary basic block it created when attempting to map
the BlockAddress constant which it encountered as the initializer of
a GlobalVariable, and it passes the OldBB when failing to find a
target. To my limited understanding, it sounds risky/incorrect to
fall-back to the OldBB there. The BasicBlocks themselves are of
course properly mapped/cloned into the new module in CloneFunction.cpp,
in function llvm::CloneFunctionInto, but a comment there seems to
talk exactly about it not being legal to clone a function that has
an external BlockAddress referencing it.
The text was updated successfully, but these errors were encountered: