wolfgangp created this revision.
The problem in PRR33930 <https://bugs.llvm.org/show_bug.cgi?id=33930> 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 <https://reviews.llvm.org/D37038>
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.
https://reviews.llvm.org/D39396
Files:
include/llvm/IR/Metadata.h
lib/CodeGen/CGVTables.cpp
test/CodeGenCXX/tmp-md-nodes1.cpp
test/CodeGenCXX/tmp-md-nodes2.cpp
Index: test/CodeGenCXX/tmp-md-nodes2.cpp
===================================================================
--- test/CodeGenCXX/tmp-md-nodes2.cpp
+++ test/CodeGenCXX/tmp-md-nodes2.cpp
@@ -0,0 +1,33 @@
+// 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(
Index: test/CodeGenCXX/tmp-md-nodes1.cpp
===================================================================
--- test/CodeGenCXX/tmp-md-nodes1.cpp
+++ test/CodeGenCXX/tmp-md-nodes1.cpp
@@ -0,0 +1,18 @@
+// 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(
Index: lib/CodeGen/CGVTables.cpp
===================================================================
--- lib/CodeGen/CGVTables.cpp
+++ lib/CodeGen/CGVTables.cpp
@@ -14,11 +14,12 @@
#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>
@@ -122,6 +123,35 @@
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 debug.declare intrinsics and resolve the DILocalVariable nodes
+ // they are referencing.
+ for (llvm::Function::iterator BB = Fn->begin(), E = Fn->end(); BB != E;
+ ++BB) {
+ for (llvm::Instruction &I : *BB) {
+ auto *DII = dyn_cast<llvm::DbgInfoIntrinsic>(&I);
+ if (DII) {
+ 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
@@ -154,6 +184,10 @@
// 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);
Index: include/llvm/IR/Metadata.h
===================================================================
--- include/llvm/IR/Metadata.h
+++ include/llvm/IR/Metadata.h
@@ -958,6 +958,9 @@
/// \pre No operands (or operands' operands, etc.) have \a isTemporary().
void resolveCycles();
+ /// \brief Resolve a unique, unresolved node.
+ void resolve();
+
/// \brief Replace a temporary node with a permanent one.
///
/// Try to create a uniqued version of \c N -- in place, if possible -- and
@@ -1009,9 +1012,6 @@
private:
void handleChangedOperand(void *Ref, Metadata *New);
- /// Resolve a unique, unresolved node.
- void resolve();
-
/// Drop RAUW support, if any.
void dropReplaceableUses();
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
