[PATCH] D45438: [CodeView] Enable debugging of captured variables within C++ lambdas
This revision was automatically updated to reflect the committed changes.
Closed by commit rC332975 (authored by bwyma, committed by ).
Herald added a subscriber: cfe-commits.
Changed prior to commit:
https://reviews.llvm.org/D45438?vs=146612&id=147988#toc
Repository:
rC Clang
https://reviews.llvm.org/D45438
Files:
lib/CodeGen/CGDebugInfo.cpp
test/CodeGen/debug-info-codeview-unnamed.c
test/CodeGenCXX/debug-info-codeview-unnamed.cpp
Index: lib/CodeGen/CGDebugInfo.cpp
===
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -801,22 +801,46 @@
}
}
-/// In C++ mode, types have linkage, so we can rely on the ODR and
-/// on their mangled names, if they're external.
-static SmallString<256> getUniqueTagTypeName(const TagType *Ty,
- CodeGenModule &CGM,
- llvm::DICompileUnit *TheCU) {
- SmallString<256> FullName;
+// Determines if the tag declaration will require a type identifier.
+static bool needsTypeIdentifier(const TagDecl *TD,
+CodeGenModule& CGM,
+llvm::DICompileUnit *TheCU) {
+ // We only add a type identifier for types with C++ name mangling.
+ if (!hasCXXMangling(TD, TheCU))
+return false;
+
+ // CodeView types with C++ mangling need a type identifier.
+ if (CGM.getCodeGenOpts().EmitCodeView)
+return true;
+
+ // Externally visible types with C++ mangling need a type identifier.
+ if (TD->isExternallyVisible())
+return true;
+
+ return false;
+}
+
+// When emitting CodeView debug information we need to produce a type
+// identifier for all types which have a C++ mangling. Until a GUID is added
+// to the identifier (not currently implemented) the result will not be unique
+// across compilation units.
+// When emitting DWARF debug information, we need to produce a type identifier
+// for all externally visible types with C++ name mangling. This identifier
+// should be unique across ODR-compliant compilation units.
+static SmallString<256> getTypeIdentifier(const TagType *Ty,
+ CodeGenModule &CGM,
+ llvm::DICompileUnit *TheCU) {
+ SmallString<256> Identifier;
const TagDecl *TD = Ty->getDecl();
- if (!hasCXXMangling(TD, TheCU) || !TD->isExternallyVisible())
-return FullName;
+ if (!needsTypeIdentifier(TD, CGM, TheCU))
+return Identifier;
// TODO: This is using the RTTI name. Is there a better way to get
// a unique string for a type?
- llvm::raw_svector_ostream Out(FullName);
+ llvm::raw_svector_ostream Out(Identifier);
CGM.getCXXABI().getMangleContext().mangleCXXRTTIName(QualType(Ty, 0), Out);
- return FullName;
+ return Identifier;
}
/// \return the appropriate DWARF tag for a composite type.
@@ -849,10 +873,10 @@
uint32_t Align = 0;
// Create the type.
- SmallString<256> FullName = getUniqueTagTypeName(Ty, CGM, TheCU);
+ SmallString<256> Identifier = getTypeIdentifier(Ty, CGM, TheCU);
llvm::DICompositeType *RetTy = DBuilder.createReplaceableCompositeType(
getTagForRecord(RD), RDName, Ctx, DefUnit, Line, 0, Size, Align,
- llvm::DINode::FlagFwdDecl, FullName);
+ llvm::DINode::FlagFwdDecl, Identifier);
if (CGM.getCodeGenOpts().DebugFwdTemplateParams)
if (auto *TSpecial = dyn_cast(RD))
DBuilder.replaceArrays(RetTy, llvm::DINodeArray(),
@@ -2477,7 +2501,7 @@
Align = getDeclAlignIfRequired(ED, CGM.getContext());
}
- SmallString<256> FullName = getUniqueTagTypeName(Ty, CGM, TheCU);
+ SmallString<256> Identifier = getTypeIdentifier(Ty, CGM, TheCU);
bool isImportedFromModule =
DebugTypeExtRefs && ED->isFromASTFile() && ED->getDefinition();
@@ -2500,7 +2524,7 @@
StringRef EDName = ED->getName();
llvm::DIType *RetTy = DBuilder.createReplaceableCompositeType(
llvm::dwarf::DW_TAG_enumeration_type, EDName, EDContext, DefUnit, Line,
-0, Size, Align, llvm::DINode::FlagFwdDecl, FullName);
+0, Size, Align, llvm::DINode::FlagFwdDecl, Identifier);
ReplaceMap.emplace_back(
std::piecewise_construct, std::make_tuple(Ty),
@@ -2520,7 +2544,7 @@
Align = getDeclAlignIfRequired(ED, CGM.getContext());
}
- SmallString<256> FullName = getUniqueTagTypeName(Ty, CGM, TheCU);
+ SmallString<256> Identifier = getTypeIdentifier(Ty, CGM, TheCU);
// Create elements for each enumerator.
SmallVector Enumerators;
@@ -2542,7 +2566,7 @@
llvm::DIType *ClassTy = getOrCreateType(ED->getIntegerType(), DefUnit);
return DBuilder.createEnumerationType(EnumContext, ED->getName(), DefUnit,
Line, Size, Align, EltArray, ClassTy,
-FullName, ED->isFixed());
+Identifier, ED->isFixed());
}
llvm::DIMacro *CGDebugIn
[PATCH] D124982: [clang][OpenMP][DebugInfo] Debug support for variables in containing scope of OMP constructs
bwyma added a comment. If the parent function is inlined into multiple callers, is the outlined subprogram scope treated like any other nested lexical scope and duplicated? If the outlined subprogram scope is duplicated when inlined, how will retained nodes on the outlined subprogram be handled in this case? Does CodeViewDebug handle the nested subprogram scoping? Can you add a test which validates the nested scoping is correct in the DWARF emission (covering the non-clang part of this patch)? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124982/new/ https://reviews.llvm.org/D124982 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45438: [CodeView] Enable debugging of captured variables within C++ lambdas
bwyma added inline comments.
Comment at: lib/CodeGen/CGDebugInfo.cpp:812-814
+ // CodeView types with C++ mangling need a type identifier.
+ if (CGM.getCodeGenOpts().EmitCodeView)
+return true;
dblaikie wrote:
> rnk wrote:
> > rnk wrote:
> > > dblaikie wrote:
> > > > Just came across this code today - it's /probably/ a problem for LTO.
> > > > LLVM IR linking depends on the identifier to determine if two structs
> > > > are the same for linkage purposes - so if an identifier is added for a
> > > > non-linkage (local/not externally visible) type, LLVM will consider
> > > > them to be the same even though they're unrelated:
> > > > ```
> > > > namespace { struct t1 { int i; int j; }; }
> > > > t1 v1;
> > > > void *v3 = &v1;
> > > > ```
> > > > ```
> > > > namespace { struct t1 { int i; }; }
> > > > t1 v1;
> > > > void *v2 = &v1;
> > > > ```
> > > > ```
> > > > $ clang++-tot -emit-llvm -S a.cpp b.cpp -g -gcodeview &&
> > > > ~/dev/llvm/build/default/bin/llvm-link -o ab.bc a.ll b.ll &&
> > > > ~/dev/llvm/build/default/bin/llvm-dis ab.bc && cat ab.ll | grep "\"t1\""
> > > > !8 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "t1",
> > > > scope: !9, file: !3, line: 1, size: 64, flags: DIFlagTypePassByValue,
> > > > elements: !10, identifier: "_ZTSN12_GLOBAL__N_12t1E")
> > > > $ clang++-tot -emit-llvm -S a.cpp b.cpp -g &&
> > > > ~/dev/llvm/build/default/bin/llvm-link -o ab.bc a.ll b.ll &&
> > > > ~/dev/llvm/build/default/bin/llvm-dis ab.bc && cat ab.ll | grep "\"t1\""
> > > > !8 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "t1",
> > > > scope: !9, file: !3, line: 1, size: 64, flags: DIFlagTypePassByValue,
> > > > elements: !10)
> > > > !21 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "t1",
> > > > scope: !9, file: !17, line: 1, size: 32, flags: DIFlagTypePassByValue,
> > > > elements: !22)
> > > > ```
> > > >
> > > > So in linking, now both `a.cpp` and `b.cpp` refer to a single `t1` type
> > > > (in this case, it looks like the first one - the 64 bit wide one).
> > > >
> > > > If CodeView actually can't represent these two distinct types with the
> > > > same name in the same object file, so be it? But this looks like it's
> > > > likely to cause problems for consumers/users.
> > > If you use the MSVC ABI, we will assign unique identifiers to these two
> > > structs:
> > > ```
> > > !9 = distinct !DICompositeType(tag: DW_TAG_structure_type, name: "t1",
> > > scope: !10, file: !7, line: 1, size: 64, flags: DIFlagTypePassByValue,
> > > elements: !11, identifier: ".?AUt1@?A0xF964240C@@")
> > > ```
> > >
> > > The `A0xF964240C` is set up here, and it is based on the source file path
> > > (the hash will only be unique when source file paths are unique across
> > > the build):
> > > https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/MicrosoftMangle.cpp#L464
> > > ```
> > > // It's important to make the mangled names unique because, when
> > > CodeView
> > > // debug info is in use, the debugger uses mangled type names to
> > > distinguish
> > > // between otherwise identically named types in anonymous namespaces.
> > > ```
> > >
> > > Maybe this should use a "is MSVC ABI" check instead, since that is what
> > > controls whether the identifier will be unique, and the unique-ness is
> > > what matters for LTO and PDBs.
> > After thinking about it some more, see the comment I added here:
> > rG59320fc9d78237a5f9fdd6a5030d6554bb6976ce
> >
> > ```
> > // If the type is not externally visible, it should be unique to the
> > current TU,
> > // and should not need an identifier to participate in type deduplication.
> > // However, when emitting CodeView, the format internally uses these
> > // unique type name identifers for references between debug info. For
> > example,
> > // the method of a class in an anonymous namespace uses the identifer to
> > refer
> > // to its parent class. The Microsoft C++ ABI attempts to provide unique
> > names
> > // for such types, so when emitting CodeView, always use identifiers for C++
> > // types. This may create problems when attempting to emit CodeView when
> > the MS
> > // C++ ABI is not in use.
> > ```
> >
> > I think type identifiers are pretty crucial for CodeView functionality. The
> > debugger won't be able to link a method to its class without them.
> > Therefore, I think it's better to leave this as it is. The bad experience
> > of duplicate type identifiers is better than the lack of functionality from
> > not emitting identifiers at all for non-externally visible types.
> Yeah, thanks for explaining/adding the comment - that about covers it.
>
>
> Hmm, do these identifiers just need to only be unique within a single object
> file to provide the name referencing mechanics? Perhaps we could generate
> them later to ensure they're unique - rather than risk collapsing them while
> linking?
>
> (at least here's an obscure case
