[IR] AutoUpgrade ModuleFlagBehavior for PIC and PIE level

[steven_wu]

Summary

From r303590, ModuleFlagBehavior for PIC and PIE level is changed from
Error to Max. This will cause bitcode compatibility issue when linking
against a bitcode static archive built with old compiler.
Add an auto-ugprade path to upgrade the the ModuleFlagBehavior in the
old bitcode to match the new one so IRLinker can link them.

[mehdi_amini]

Is this something that is affecting clang-5.0 backward compatibility with clang-4.0?
(adding @hansw for visibility)

[steven_wu]

Yes, this will affect clang-5.0 backward compatibility. Thanks for bringing this up. I was planning to nominate to merge into release branch after this gets reviewed.

[hans]

Thanks! It's on my radar now.

[steven_wu]

Ping.

[dexonsmith]

Who changed the behaviour? Can we get them to review the behaviour of the upgrade?

I don't see a general test for PIE level. Can we add one of those as well?

lib/IR/AutoUpgrade.cpp

2269–2270 ↗

(On Diff #110489)

This would read more nicely as auto *Behavior, since you've already been explicit about the ConstantInt.

2273–2274 ↗

(On Diff #110489)

It's not generally sound to call MDNode::replaceOperandWith, unless the nodes are "distinct". I strongly suspect these nodes are "uniqued", which means that they are semantically constants (with far-too-lenient an API).

Instead, you should construct a new node to attach to the NamedMDNode.

[tejohnson]

Sorry for the delay, I was out all last week and missed this one. LGTM but with one question below about a test change.

test/Linker/module-flags-pic-1-a.ll

5 ↗

(On Diff #110489)

Does this need to change - i.e. with this patch shouldn't it auto-upgrade?

[tejohnson]

Duncan has a good point about replaceOperandWith, please address that (updating to remove patch accept).

[steven_wu]

In D36556#844596, @dexonsmith wrote:

Who changed the behaviour? Can we get them to review the behaviour of the upgrade?

I don't see a general test for PIE level. Can we add one of those as well?

The PIE test is in the same module.

test/Linker/module-flags-pic-1-a.ll needs to updated because the auto upgrade only happens when you load from bitcode. The texture format is not auto upgradeable and the old value still has valid syntax.

lib/IR/AutoUpgrade.cpp

2273–2274 ↗

(On Diff #110489)

I am pretty sure the MDNode added to ModuleFlags are all uniqued.

I am trying to modify the ModuleFlag here. Attaching new node to module flag is easy but removing is not. Sure, I can reconstruct the entire module flag NamedMDNode, but I am not sure if that is worthwhile if the MDNode is ensured to be uniqued.

[steven_wu]

Here is a version that rebuilds the module flags to avoid replaceOperandsWith.

[dexonsmith]

lib/IR/AutoUpgrade.cpp

2305–2311 ↗

(On Diff #111754)

I think you should just change ModFlags->getOperand(I) in place. I think you'll need to add some API to NamedMDNode, but that should be straightforward.

[steven_wu]

Thanks for the feedback. Here is another attempt to modify the module flag in place.

[steven_wu]

Missing one fixup from the previous update. Try again.

[dexonsmith]

LGTM! Thanks for reworking this.

llvm/trunk/lib/IR/AutoUpgrade.cpp

bool llvm::UpgradeDebugInfo(Module &M) {
   if (RetCode) {
     DiagnosticInfoDebugMetadataVersion DiagVersion(M, Version);
     M.getContext().diagnose(DiagVersion);
   }
   return RetCode;
 }
 
 bool llvm::UpgradeModuleFlags(Module &M) {
-  const NamedMDNode *ModFlags = M.getModuleFlagsMetadata();
+  NamedMDNode *ModFlags = M.getModuleFlagsMetadata();
   if (!ModFlags)
     return false;
 
-  bool HasObjCFlag = false, HasClassProperties = false;
+  bool HasObjCFlag = false, HasClassProperties = false, Changed = false;
   for (unsigned I = 0, E = ModFlags->getNumOperands(); I != E; ++I) {
     MDNode *Op = ModFlags->getOperand(I);
-    if (Op->getNumOperands() < 2)
+    if (Op->getNumOperands() != 3)
       continue;
     MDString *ID = dyn_cast_or_null<MDString>(Op->getOperand(1));
     if (!ID)
       continue;
     if (ID->getString() == "Objective-C Image Info Version")
       HasObjCFlag = true;
     if (ID->getString() == "Objective-C Class Properties")
       HasClassProperties = true;
+    // Upgrade PIC/PIE Module Flags. The module flag behavior for these two
+    // field was Error and now they are Max.
+    if (ID->getString() == "PIC Level" || ID->getString() == "PIE Level") {
+      if (auto *Behavior =
+              mdconst::dyn_extract_or_null<ConstantInt>(Op->getOperand(0))) {
+        if (Behavior->getLimitedValue() == Module::Error) {
+          Type *Int32Ty = Type::getInt32Ty(M.getContext());
+          Metadata *Ops[3] = {
+              ConstantAsMetadata::get(ConstantInt::get(Int32Ty, Module::Max)),
+              MDString::get(M.getContext(), ID->getString()),
+              Op->getOperand(2)};
+          ModFlags->setOperand(I, MDNode::get(M.getContext(), Ops));
+          Changed = true;
         }
+      }
+    }
+  }
+
   // "Objective-C Class Properties" is recently added for Objective-C. We
   // upgrade ObjC bitcodes to contain a "Objective-C Class Properties" module
   // flag of value 0, so we can correclty downgrade this flag when trying to
   // link an ObjC bitcode without this module flag with an ObjC bitcode with
   // this module flag.
   if (HasObjCFlag && !HasClassProperties) {
     M.addModuleFlag(llvm::Module::Override, "Objective-C Class Properties",
                     (uint32_t)0);
-    return true;
+    Changed = true;
   }
-  return false;
+
+  return Changed;
 }
 
 static bool isOldLoopArgument(Metadata *MD) {
   auto *T = dyn_cast_or_null<MDTuple>(MD);
   if (!T)
     return false;
   if (T->getNumOperands() < 1)
     return false;

llvm/trunk/test/Bitcode/upgrade-module-flag.ll

 ; RUN: llvm-as < %s | llvm-dis | FileCheck %s
 ; RUN: verify-uselistorder < %s
 
-!llvm.module.flags = !{!0}
+!llvm.module.flags = !{!0, !1, !2}
 
-!0 = !{i32 1, !"Objective-C Image Info Version", i32 0}
+!0 = !{i32 1, !"PIC Level", i32 1}
+!1 = !{i32 1, !"PIE Level", i32 1}
+!2 = !{i32 1, !"Objective-C Image Info Version", i32 0}
 
-; CHECK: !0 = !{i32 1, !"Objective-C Image Info Version", i32 0}
-; CHECK: !1 = !{i32 4, !"Objective-C Class Properties", i32 0}
+; CHECK: !0 = !{i32 7, !"PIC Level", i32 1}
+; CHECK: !1 = !{i32 7, !"PIE Level", i32 1}
+; CHECK: !2 = !{i32 1, !"Objective-C Image Info Version", i32 0}
+; CHECK: !3 = !{i32 4, !"Objective-C Class Properties", i32 0}

llvm/trunk/test/Linker/module-flags-pic-1-a.ll

 ; RUN: llvm-link %s %p/Inputs/module-flags-pic-1-b.ll -S -o - | FileCheck %s
 
 ; test linking modules with specified and default PIC levels
 
-!0 = !{ i32 1, !"PIC Level", i32 1 }
+!0 = !{ i32 7, !"PIC Level", i32 1 }
 
 !llvm.module.flags = !{!0}
 ; CHECK: !llvm.module.flags = !{!0}
-; CHECK: !0 = !{i32 1, !"PIC Level", i32 1}
+; CHECK: !0 = !{i32 7, !"PIC Level", i32 1}