[llvm] [DebugInfo][RemoveDIs] Use autoupgrader to convert old debug-info (PR #143452)

[Orlando Cazalet-Hyams via llvm-commits]

================
@@ -4398,11 +4397,25 @@ static Value *upgradeAMDGCNIntrinsicCall(StringRef Name, CallBase *CI,
 /// Helper to unwrap intrinsic call MetadataAsValue operands.
 template <typename MDType>
 static MDType *unwrapMAVOp(CallBase *CI, unsigned Op) {
-  if (MetadataAsValue *MAV = dyn_cast<MetadataAsValue>(CI->getArgOperand(Op)))
-    return dyn_cast<MDType>(MAV->getMetadata());
+  if (Op < CI->arg_size()) {
+    if (MetadataAsValue *MAV = dyn_cast<MetadataAsValue>(CI->getArgOperand(Op)))
+      // Use a reinterpret cast rather than a safe default-to-null cast: the
+      // autoupgrade process happens before the verifier, and thus there might
+      // be some nonsense metadata in there.
+      return reinterpret_cast<MDType*>(MAV->getMetadata());
+  }
   return nullptr;
 }
 
+static const DILocation *getDebugLocSafe(const Instruction *I) {
+  MDNode *MD = I->getDebugLoc().getAsMDNode();
+  // Use a C-style cast here rather than cast<DILocation>. The autoupgrader
+  // runs before the verifier, so the Metadata could refer to anything. Allow
+  // the verifier to detect and produce an error message, which will be much
+  // more ergonomic to the user.
+  return (const DILocation*)MD;
----------------
OCHyams wrote:

Not 100% sure but could this be UB if MD is in fact not a DILocation? Or is that only if it's read through the ptr... I guess it's still not ideal to introduce the chance of UB...

Would it be more correct to return nullptr in that case, and allow nullptrs in DbgRecords which are checked against in the verifier (I don't like it, but not sure this is right either?). I guess the same comment applies to the reinterpret_cast above

https://github.com/llvm/llvm-project/pull/143452