[PATCH] D45438: [CodeView] Enable debugging of captured variables within C++ lambdas

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/tools/clang/lib/CodeGen/CGDebugInfo.cpp:805
+static bool needsUniqueTagTypeName(const TagDecl *TD,
+		                   CodeGenModule& CGM,
+				   llvm::DICompileUnit *TheCU) {
----------------
The indentation seems off which suggests the patch may contain tabs. Please check that there are none before submitting.


================
Comment at: llvm/tools/clang/lib/CodeGen/CGDebugInfo.cpp:807
+				   llvm::DICompileUnit *TheCU) {
+  return hasCXXMangling(TD, TheCU) &&
+	  (TD->isExternallyVisible() ||
----------------
This boolean condition is complicated. Can you find a way to break it up and add comments to clarify the intent? Basically, externally visible types with C++ manglings always need unique names. When emitting codeview, non-externally visible unnamed types also need unique names.

However, doesn't this conflict with the goal of D32498? At this point, Clang will put unique names on every unnamed type in C++ mode. That's fine with me, FWIW.


================
Comment at: llvm/tools/clang/lib/CodeGen/CGDebugInfo.cpp:809
+	  (TD->isExternallyVisible() ||
+           (CGM.getCodeGenOpts().EmitCodeView && TD->getName().empty()));
+}
----------------
My final concern is that the C++ mangled names we generate in this case are not necessarily unique across the whole program, which is what we mean when we say "unique name" in this context. Fixing that is out of scope for this patch, but we should file a bug for it. I guess here is a case where we'll have two unrelated types with the same identifier:
```
// a.cpp
namespace { struct { int x; } x; }
int getx() { return x.x; }
// b.cpp
namespace { struct { float y; } x; }
float gety() { return x.y; }
```

I think we're safe for types inside non-inline functions, because we mangle in the parent function as a scope, so no other TU will collide with that mangling. Only anonymous namespaces have this problem. MSVC addresses this by including a hash (MD5?) of the main source file path in the mangled name for the anonymous namespace.


================
Comment at: llvm/tools/clang/test/CodeGenCXX/debug-info-codeview-unnamed.cpp:35
+  //
+  struct { int bar; } two = { 42 };
+  int decltype(two)::*ptr2unnamed = &decltype(two)::bar;
----------------
We should have a test that in C mode, anonymous structs do not get identifiers. If we already have it, that's fine.


================
Comment at: llvm/tools/clang/test/CodeGenCXX/debug-info-codeview-unnamed.cpp:57
+
+  // Named structures do not require an identifier in either Linux or CodeView.
+  //
----------------
Maybe "Non-externally visible named structures do not ..." to be specific.

Also, are you sure? I would expect the debugger to require a forward reference for these methods and data members, and how can that work without a unique name?


https://reviews.llvm.org/D45438