[PATCH] D41698: [DebugInfo] Enable debug information for C99 VLA types

[Adrian Prantl via Phabricator via cfe-commits]

aprantl added a comment.

It would be awesome if you could also add an end-to-end test to the debuginfo-tests repository so we can verify that this actually works in LLDB and GDB.



================
Comment at: lib/CodeGen/CGDebugInfo.cpp:2358
+    if (auto *SizeNode = getVLASizeExpressionForType(EltTy))
+      Subscripts.push_back(DBuilder.getOrCreateSubrange(0, SizeNode));
+    else
----------------
perhaps write sizeNode/Count to a variable, so you don't have to duplicate the expression?


================
Comment at: lib/CodeGen/CGDebugInfo.cpp:3473
                               CGBuilderTy &Builder) {
+  llvm::Optional<llvm::Metadata *> Optional;
+  EmitDeclare(VD, Storage, ArgNo, Optional, Builder);
----------------
Could this function be replaced by a default argument instead?


================
Comment at: lib/CodeGen/CGDebugInfo.h:86
+  /// represented by instantiated Metadata nodes.
+  llvm::SmallDenseMap<QualType, llvm::Metadata *> SizeExprCache;
+
----------------
I'm expecting VLA's to not be very common, should we use a regular DenseMap instead?


================
Comment at: lib/CodeGen/CGDebugInfo.h:317
+  llvm::Metadata *getVLASizeExpressionForType(QualType Ty) {
+    if (SizeExprCache.count(Ty))
+      return SizeExprCache[Ty];
----------------
This is performing the lookup twice. If you use .find() instead it will be more efficient. We also don't use accessor functions like this for any of the other caches. If you think that they help, perhaps make this more generic and useful for all caches?


================
Comment at: lib/CodeGen/CGDebugInfo.h:404
+  void EmitDeclareOfAutoVariable(const VarDecl *Decl, llvm::Value *AI,
+                                 llvm::Optional<llvm::Metadata *> &MetadataDecl,
+                                 CGBuilderTy &Builder);
----------------
do we need the qualifier on Optional?


================
Comment at: lib/CodeGen/CGDecl.cpp:1125
+    // If we have debug info enabled, describe the VLA dimensions properly.
+    if (EmitDebugInfo) {
+      QualType Type1D = Ty;
----------------
Please move this code into a member function of CGDebugInfo.


================
Comment at: lib/CodeGen/CGDecl.cpp:1137
+          // Allocate memory for the address of the vla expression
+          // We can use this for debugging purposes
+          auto SizeExprAddr = CreateDefaultAlignTempAlloca(
----------------
`.`


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:1963
+  assert(VlaSize->getType() == SizeTy);
+  return std::pair<llvm::Value *, QualType>(VlaSize, Vla->getElementType());
+}
----------------
`return {VlaSize, Vla->getElementType()};`


https://reviews.llvm.org/D41698