[PATCH] D53234: Don't remove COMDATs when internalizing global objects

[Aaron Hill via Phabricator via llvm-commits]

Aaron1011 created this revision.
Aaron1011 added reviewers: pcc, tejohnson.
Herald added a subscriber: llvm-commits.

**Background:**

The LLVM Language Reference <https://llvm.org/docs/LangRef.html#comdats> states that "All global objects that specify this key will only end up in the final object file if the linker chooses that key over some other key" - e.g. it shouldn't be possible for only some, but not all, of the objects in a COMDAT to be present in the final object.

However, Internalizer currently removes non-externally-visible COMDATs when internalizing global objects. The commit which added this behavior <https://github.com/llvm-mirror/llvm/commit/4a6d99b0a96d4eb27d89c22e33981ff0344c5737> states that passes like GlobalDCE can legally remove individual COMDAT members. However, this is no longer correct - GlobalDCE always marks other COMDAT members as live <https://github.com/llvm-mirror/llvm/blob/d069d45aa888c5b8a44521d4cade32767c09b35e/lib/Transforms/IPO/GlobalDCE.cpp#L131> when examining a module.

This behavior can end up causing issues involving the BFD linker and MD_Associated metadata, since the removed COMDAT allows GlobalDCE to delete the target of the MD_Associated metadata. Keeping the COMDAT causes the MD_Associated-annotated object and its target to be kept or removed as a unit, which is the correct behavior.

**Changes:**

This patch stops Internalizer from removing COMDATs from global objects, and updates the associated test to reflect this new behavior.


Repository:
  rL LLVM

https://reviews.llvm.org/D53234

Files:
  lib/Transforms/IPO/Internalize.cpp
  test/Transforms/Internalize/comdat.ll


Index: test/Transforms/Internalize/comdat.ll
===================================================================
--- test/Transforms/Internalize/comdat.ll
+++ test/Transforms/Internalize/comdat.ll
@@ -8,7 +8,7 @@
 ; CHECK: @c1_c = global i32 0, comdat($c1)
 @c1_c = global i32 0, comdat($c1)
 
-; CHECK: @c2_b = internal global i32 0{{$}}
+; CHECK: @c2_b = internal global i32 0, comdat($c2)
 @c2_b = global i32 0, comdat($c2)
 
 ; CHECK: @c3 = global i32 0, comdat{{$}}
@@ -36,12 +36,12 @@
   ret void
 }
 
-; CHECK: define internal void @c2() {
+; CHECK: define internal void @c2() comdat {
 define internal void @c2() comdat {
   ret void
 }
 
-; CHECK: define internal void @c2_a() {
+; CHECK: define internal void @c2_a() comdat($c2) {
 define void @c2_a() comdat($c2) {
   ret void
 }
Index: lib/Transforms/IPO/Internalize.cpp
===================================================================
--- lib/Transforms/IPO/Internalize.cpp
+++ lib/Transforms/IPO/Internalize.cpp
@@ -119,10 +119,6 @@
     if (ExternalComdats.count(C))
       return false;
 
-    // If a comdat is not externally visible we can drop it.
-    if (auto GO = dyn_cast<GlobalObject>(&GV))
-      GO->setComdat(nullptr);
-
     if (GV.hasLocalLinkage())
       return false;
   } else {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D53234.169530.patch
Type: text/x-patch
Size: 1265 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20181013/1ed8758f/attachment.bin>