Fix for PR33930. Short-circuit metadata mapping when cloning a varargs thunk.

[wolfgangp]

Summary

The problem in PRR33930 comes about because clang is cloning a function (to generate varargs thunks) before all the Metadata nodes are resolved. The value mapper, which is used by the cloner to deal with Medatdata nodes, asserts when it encounters temporary otherwise unresolved nodes.

Ideally we would improve varargs thunk generation to not use cloning, but in the meantime the fix I'm proposing here is to clone the function's DISubprogram node explicitly (the verifier insists on ever function having its own). The cloned node is then entered into the value map that is supplied to the cloner (which feeds it to the value mapper). This has the effect that the value mapper does not look any further when it's trying to map all the rest of the MD nodes and therefore does not run into any temporary MD nodes that still exist at than point (in the form of forward references to struct declarations).

Furthermore, there are unresolved DILocalVariable nodes referenced by dbg.value intrinsics, which causes the value mapper to assert when it is remapping the function's instructions' MDNodes. These nodes are normally resolved at the catch-all at the end of the module, but they can be safely resolved here, since they won't change any more. The value mapper will safely clone them (if necessary) and remap their operand nodes during the cloning process.

The only drawback is that the cloned function's (the thunk's) DILocalVariableNodes will not appear in the newly cloned DISubprogram's node variable list. Instead, the new node points to the original function's variable list. However, since the only difference between the original function and the thunk is the this-ptr adjustment, the resulting debug information is correct, at least in the test cases I have looked at.

Unfortunately this isn't the cleanest solution in the world. A different approach, deferring the creation of varargs thunks until the end of the module, is more invasive. The approach proposed here duplicates Metadata. If anybody has any better ideas, please let me know.

Note that there are two patches here. One is an llvm patch that is NFC (making resolve() public) while the main one is a clang patch.

[aprantl]

The only drawback is that the cloned function's (the thunk's) DILocalVariableNodes will not appear in the newly cloned DISubprogram's node variable list. Instead, the new node points to the original function's variable list. However, since the only difference between the original function and the thunk is the this-ptr adjustment, the resulting debug information is correct, at least in the test cases I have looked at.

Does the thunk contain any local variables other than function arguments?
What I'm getting at is: If this is only a thunk, do we need any local variables at all?

include/llvm/IR/Metadata.h

961 ↗

(On Diff #120710)

The \brief is redundant and can be dropped.

lib/CodeGen/CGVTables.cpp

130 ↗

(On Diff #120710)

///

140 ↗

(On Diff #120710)

debug.declare -> (llvm.)dbg.declare.

143 ↗

(On Diff #120710)

does this work?
for (auto &BB : Fn->getBasicBlockList())

145 ↗

(On Diff #120710)

if (auto *DII ...

[wolfgangp]

Incorporated review comments.

[wolfgangp]

lib/CodeGen/CGVTables.cpp

143 ↗

(On Diff #120710)

Yep, thanks. I wasn't aware of this.

[aprantl]

This works for me, but as I said previously, perhaps we can get by with just not having any variables described in the thunk to further simplify the code.

[wolfgangp]

In D39396#911306, @aprantl wrote:

This works for me, but as I said previously, perhaps we can get by with just not having any variables described in the thunk to further simplify the code.

I remember, but this "thunk" isn't really a thunk in the sense that it makes some adjustments and then branches to (or calls) the real function. It effectively becomes the real function (hence the cloning), so if you drop the variables you lose the ability to debug it.

[aprantl]

I see. Then it makes sense to do it this way. Thanks for clarifying!

cfe/trunk/lib/CodeGen/CGVTables.cpp

 //===--- CGVTables.cpp - Emit LLVM Code for C++ vtables -------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This contains code dealing with C++ code generation of virtual tables.
 //
 //===----------------------------------------------------------------------===//
 
 #include "CGCXXABI.h"
 #include "CodeGenFunction.h"
 #include "CodeGenModule.h"
-#include "clang/CodeGen/ConstantInitBuilder.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/RecordLayout.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
+#include "clang/CodeGen/ConstantInitBuilder.h"
 #include "clang/Frontend/CodeGenOptions.h"
+#include "llvm/IR/IntrinsicInst.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Transforms/Utils/Cloning.h"
 #include <algorithm>
 #include <cstdio>
 
 using namespace clang;
 using namespace CodeGen;
 
if (NullCheckValue) {
     PHI->addIncoming(llvm::Constant::getNullValue(ReturnValue->getType()),
                      AdjustNull);
     ReturnValue = PHI;
   }
 
   return RValue::get(ReturnValue);
 }
 
+/// This function clones a function's DISubprogram node and enters it into 
+/// a value map with the intent that the map can be utilized by the cloner
+/// to short-circuit Metadata node mapping.
+/// Furthermore, the function resolves any DILocalVariable nodes referenced
+/// by dbg.value intrinsics so they can be properly mapped during cloning.
+static void resolveTopLevelMetadata(llvm::Function *Fn,
+                                    llvm::ValueToValueMapTy &VMap) {
+  // Clone the DISubprogram node and put it into the Value map.
+  auto *DIS = Fn->getSubprogram();
+  if (!DIS)
+    return;
+  auto *NewDIS = DIS->replaceWithDistinct(DIS->clone());
+  VMap.MD()[DIS].reset(NewDIS);
+
+  // Find all llvm.dbg.declare intrinsics and resolve the DILocalVariable nodes
+  // they are referencing.
+  for (auto &BB : Fn->getBasicBlockList()) {
+    for (auto &I : BB) {
+      if (auto *DII = dyn_cast<llvm::DbgInfoIntrinsic>(&I)) {
+        auto *DILocal = DII->getVariable();
+        if (!DILocal->isResolved())
+          DILocal->resolve();
+      }
+    }
+  }
+}
+
 // This function does roughly the same thing as GenerateThunk, but in a
 // very different way, so that va_start and va_end work correctly.
 // FIXME: This function assumes "this" is the first non-sret LLVM argument of
 //        a function, and that there is an alloca built in the entry block
 //        for all accesses to "this".
 // FIXME: This function assumes there is only one "ret" statement per function.
 // FIXME: Cloning isn't correct in the presence of indirect goto!
 // FIXME: This implementation of thunks bloats codesize by duplicating the
CodeGenFunction::GenerateVarArgsThunk(llvm::Function *Fn,
   // Get the original function
   assert(FnInfo.isVariadic());
   llvm::Type *Ty = CGM.getTypes().GetFunctionType(FnInfo);
   llvm::Value *Callee = CGM.GetAddrOfFunction(GD, Ty, /*ForVTable=*/true);
   llvm::Function *BaseFn = cast<llvm::Function>(Callee);
 
   // Clone to thunk.
   llvm::ValueToValueMapTy VMap;
+
+  // We are cloning a function while some Metadata nodes are still unresolved.
+  // Ensure that the value mapper does not encounter any of them.
+  resolveTopLevelMetadata(BaseFn, VMap);
   llvm::Function *NewFn = llvm::CloneFunction(BaseFn, VMap);
   Fn->replaceAllUsesWith(NewFn);
   NewFn->takeName(Fn);
   Fn->eraseFromParent();
   Fn = NewFn;
 
   // "Initialize" CGF (minimally).
   CurFn = Fn;

cfe/trunk/test/CodeGenCXX/tmp-md-nodes1.cpp

+// REQUIRES: asserts
+// RUN: %clang_cc1 -O0 -triple %itanium_abi_triple -debug-info-kind=limited -S -emit-llvm %s -o - | \
+// RUN: FileCheck %s
+
+// This test simply checks that the varargs thunk is created. The failing test
+// case asserts.
+
+struct Alpha {
+  virtual void bravo(...);
+};
+struct Charlie {
+  virtual ~Charlie() {}
+};
+struct CharlieImpl : Charlie, Alpha {
+  void bravo(...) {}
+} delta;
+
+// CHECK: define {{.*}} void @_ZThn8_N11CharlieImpl5bravoEz(

cfe/trunk/test/CodeGenCXX/tmp-md-nodes2.cpp

+// REQUIRES: asserts
+// RUN: %clang_cc1 -O0 -triple %itanium_abi_triple -debug-info-kind=limited -S -emit-llvm %s -o - | \
+// RUN: FileCheck %s
+
+// This test simply checks that the varargs thunk is created. The failing test
+// case asserts.
+
+typedef signed char __int8_t;
+typedef int BOOL;
+class CMsgAgent;
+
+class CFs {
+public:
+  typedef enum {} CACHE_HINT;
+  virtual BOOL ReqCacheHint( CMsgAgent* p_ma, CACHE_HINT hint, ... ) ;
+};
+
+typedef struct {} _Lldiv_t;
+
+class CBdVfs {
+public:
+  virtual ~CBdVfs( ) {}
+};
+
+class CBdVfsImpl : public CBdVfs, public CFs {
+  BOOL ReqCacheHint( CMsgAgent* p_ma, CACHE_HINT hint, ... );
+};
+
+BOOL CBdVfsImpl::ReqCacheHint( CMsgAgent* p_ma, CACHE_HINT hint, ... ) {
+  return true;
+}
+
+// CHECK: define {{.*}} @_ZThn8_N10CBdVfsImpl12ReqCacheHintEP9CMsgAgentN3CFs10CACHE_HINTEz(