[DebugInfo][OpenCL] Address post-commit review of D49930

[scott.linder]

Summary

Refactor collection of member debug info into helper functions and add separate debug-info tests.

The reason for -debug-info-kind=limited in the tests is because they invoke cc1. I am just following the existing tests, but I can change this to just clang with -g.

[scott.linder]

Add comments to explain OpenCL case

[echristo]

Looks pretty good. Could you pass CGM in and just make the functions static I couldn't see any other class variables, but might have missed something. One inline comment as well.

-eric

lib/CodeGen/CGDebugInfo.cpp

997 ↗

(On Diff #158532)

Just noticed that LineNo is 0... for the entire function.

[scott.linder]

When I went to mark these as static I noticed they use CGDebugInfo::CreateMemberType which uses a couple other non-static member functions, and it starts to become difficult to tease things out into nice clean static functions.

lib/CodeGen/CGDebugInfo.cpp

997 ↗

(On Diff #158532)

What should it be instead? My understanding is that LineNo=0 indicates there is no corresponding source, and these fields seem to be implementation details.

[echristo]

In D50099#1189667, @scott.linder wrote:

When I went to mark these as static I noticed they use CGDebugInfo::CreateMemberType which uses a couple other non-static member functions, and it starts to become difficult to tease things out into nice clean static functions.

Ah. Fair. No worries then.

lib/CodeGen/CGDebugInfo.cpp

997 ↗

(On Diff #158532)

Could probably just replace it with a constant 0 in the calls rather than having the local?

[scott.linder]

lib/CodeGen/CGDebugInfo.cpp

997 ↗

(On Diff #158532)

Oh, I see what you mean; I will make that change, and I am also working out how to enable ASan correctly to check the patch, then I will post a new diff.

[scott.linder]

Address feedback

[echristo]

LGTM. Thanks.

cfe/trunk/lib/CodeGen/CGDebugInfo.h

#include "clang/Basic/OpenCLImageTypes.def"
   /// DWARF address space, appends extended dereferencing mechanism to complex
   /// expression \p Expr. Otherwise, does nothing.
   ///
   /// Extended dereferencing mechanism is has the following format:
   ///     DW_OP_constu <DWARF Address Space> DW_OP_swap DW_OP_xderef
   void AppendAddressSpaceXDeref(unsigned AddressSpace,
                                 SmallVectorImpl<int64_t> &Expr) const;
 
+  /// A helper function to collect debug info for the default elements of a
+  /// block.
+  ///
+  /// \returns The next available field offset after the default elements.
+  uint64_t collectDefaultElementTypesForBlockPointer(
+      const BlockPointerType *Ty, llvm::DIFile *Unit,
+      llvm::DIDerivedType *DescTy, unsigned LineNo,
+      SmallVectorImpl<llvm::Metadata *> &EltTys);
+
+  /// A helper function to collect debug info for the default fields of a
+  /// block.
+  void collectDefaultFieldsForBlockLiteralDeclare(
+      const CGBlockInfo &Block, const ASTContext &Context, SourceLocation Loc,
+      const llvm::StructLayout &BlockLayout, llvm::DIFile *Unit,
+      SmallVectorImpl<llvm::Metadata *> &Fields);
+
 public:
   CGDebugInfo(CodeGenModule &CGM);
   ~CGDebugInfo();
 
   void finalize();
 
   /// Register VLA size expression debug node with the qualified type.
   void registerVLASizeExpression(QualType Ty, llvm::Metadata *SizeExpr) {

cfe/trunk/lib/CodeGen/CGDebugInfo.cpp

if (Cache)
     return Cache;
   Cache = DBuilder.createForwardDecl(llvm::dwarf::DW_TAG_structure_type, Name,
                                      TheCU, getOrCreateMainFile(), 0);
   unsigned Size = CGM.getContext().getTypeSize(CGM.getContext().VoidPtrTy);
   Cache = DBuilder.createPointerType(Cache, Size);
   return Cache;
 }
 
+uint64_t CGDebugInfo::collectDefaultElementTypesForBlockPointer(
+    const BlockPointerType *Ty, llvm::DIFile *Unit, llvm::DIDerivedType *DescTy,
+    unsigned LineNo, SmallVectorImpl<llvm::Metadata *> &EltTys) {
+  QualType FType;
+
+  // Advanced by calls to CreateMemberType in increments of FType, then
+  // returned as the overall size of the default elements.
+  uint64_t FieldOffset = 0;
+
+  // Blocks in OpenCL have unique constraints which make the standard fields
+  // redundant while requiring size and align fields for enqueue_kernel. See
+  // initializeForBlockHeader in CGBlocks.cpp
+  if (CGM.getLangOpts().OpenCL) {
+    FType = CGM.getContext().IntTy;
+    EltTys.push_back(CreateMemberType(Unit, FType, "__size", &FieldOffset));
+    EltTys.push_back(CreateMemberType(Unit, FType, "__align", &FieldOffset));
+  } else {
+    FType = CGM.getContext().getPointerType(CGM.getContext().VoidTy);
+    EltTys.push_back(CreateMemberType(Unit, FType, "__isa", &FieldOffset));
+    FType = CGM.getContext().IntTy;
+    EltTys.push_back(CreateMemberType(Unit, FType, "__flags", &FieldOffset));
+    EltTys.push_back(CreateMemberType(Unit, FType, "__reserved", &FieldOffset));
+    FType = CGM.getContext().getPointerType(Ty->getPointeeType());
+    EltTys.push_back(CreateMemberType(Unit, FType, "__FuncPtr", &FieldOffset));
+    FType = CGM.getContext().getPointerType(CGM.getContext().VoidTy);
+    uint64_t FieldSize = CGM.getContext().getTypeSize(Ty);
+    uint32_t FieldAlign = CGM.getContext().getTypeAlign(Ty);
+    EltTys.push_back(DBuilder.createMemberType(
+        Unit, "__descriptor", nullptr, LineNo, FieldSize, FieldAlign,
+        FieldOffset, llvm::DINode::FlagZero, DescTy));
+    FieldOffset += FieldSize;
+  }
+
+  return FieldOffset;
+}
+
 llvm::DIType *CGDebugInfo::CreateType(const BlockPointerType *Ty,
                                       llvm::DIFile *Unit) {
   SmallVector<llvm::Metadata *, 8> EltTys;
   QualType FType;
-  uint64_t FieldSize, FieldOffset;
-  uint32_t FieldAlign;
+  uint64_t FieldOffset;
   llvm::DINodeArray Elements;
 
   FieldOffset = 0;
   FType = CGM.getContext().UnsignedLongTy;
   EltTys.push_back(CreateMemberType(Unit, FType, "reserved", &FieldOffset));
   EltTys.push_back(CreateMemberType(Unit, FType, "Size", &FieldOffset));
 
   Elements = DBuilder.getOrCreateArray(EltTys);
   EltTys.clear();
 
   llvm::DINode::DIFlags Flags = llvm::DINode::FlagAppleBlock;
-  unsigned LineNo = 0;
 
   auto *EltTy =
-      DBuilder.createStructType(Unit, "__block_descriptor", nullptr, LineNo,
+      DBuilder.createStructType(Unit, "__block_descriptor", nullptr, 0,
                                 FieldOffset, 0, Flags, nullptr, Elements);
 
   // Bit size, align and offset of the type.
   uint64_t Size = CGM.getContext().getTypeSize(Ty);
 
   auto *DescTy = DBuilder.createPointerType(EltTy, Size);
 
-  FieldOffset = 0;
-  if (CGM.getLangOpts().OpenCL) {
-    FType = CGM.getContext().IntTy;
-    EltTys.push_back(CreateMemberType(Unit, FType, "__size", &FieldOffset));
-    EltTys.push_back(CreateMemberType(Unit, FType, "__align", &FieldOffset));
-  } else {
-    FType = CGM.getContext().getPointerType(CGM.getContext().VoidTy);
-    EltTys.push_back(CreateMemberType(Unit, FType, "__isa", &FieldOffset));
-    FType = CGM.getContext().IntTy;
-    EltTys.push_back(CreateMemberType(Unit, FType, "__flags", &FieldOffset));
-    EltTys.push_back(CreateMemberType(Unit, FType, "__reserved", &FieldOffset));
-    FType = CGM.getContext().getPointerType(Ty->getPointeeType());
-    EltTys.push_back(CreateMemberType(Unit, FType, "__FuncPtr", &FieldOffset));
-    FType = CGM.getContext().getPointerType(CGM.getContext().VoidTy);
-    FieldSize = CGM.getContext().getTypeSize(Ty);
-    FieldAlign = CGM.getContext().getTypeAlign(Ty);
-    EltTys.push_back(DBuilder.createMemberType(
-        Unit, "__descriptor", nullptr, LineNo, FieldSize, FieldAlign, FieldOffset,
-        llvm::DINode::FlagZero, DescTy));
-    FieldOffset += FieldSize;
-  }
+  FieldOffset = collectDefaultElementTypesForBlockPointer(Ty, Unit, DescTy,
+                                                          0, EltTys);
 
   Elements = DBuilder.getOrCreateArray(EltTys);
 
   // The __block_literal_generic structs are marked with a special
   // DW_AT_APPLE_BLOCK attribute and are an implementation detail only
   // the debugger needs to know about. To allow type uniquing, emit
   // them without a name or a location.
-  EltTy = DBuilder.createStructType(Unit, "", nullptr, LineNo, FieldOffset, 0,
+  EltTy = DBuilder.createStructType(Unit, "", nullptr, 0, FieldOffset, 0,
                                     Flags, nullptr, Elements);
 
   return DBuilder.createPointerType(EltTy, Size);
 }
 
 llvm::DIType *CGDebugInfo::CreateType(const TemplateSpecializationType *Ty,
                                       llvm::DIFile *Unit) {
   assert(Ty->isTypeAlias());
struct BlockLayoutChunk {
   uint64_t OffsetInBits;
   const BlockDecl::Capture *Capture;
 };
 bool operator<(const BlockLayoutChunk &l, const BlockLayoutChunk &r) {
   return l.OffsetInBits < r.OffsetInBits;
 }
 } // namespace
 
+void CGDebugInfo::collectDefaultFieldsForBlockLiteralDeclare(
+    const CGBlockInfo &Block, const ASTContext &Context, SourceLocation Loc,
+    const llvm::StructLayout &BlockLayout, llvm::DIFile *Unit,
+    SmallVectorImpl<llvm::Metadata *> &Fields) {
+  // Blocks in OpenCL have unique constraints which make the standard fields
+  // redundant while requiring size and align fields for enqueue_kernel. See
+  // initializeForBlockHeader in CGBlocks.cpp
+  if (CGM.getLangOpts().OpenCL) {
+    Fields.push_back(createFieldType("__size", Context.IntTy, Loc, AS_public,
+                                     BlockLayout.getElementOffsetInBits(0),
+                                     Unit, Unit));
+    Fields.push_back(createFieldType("__align", Context.IntTy, Loc, AS_public,
+                                     BlockLayout.getElementOffsetInBits(1),
+                                     Unit, Unit));
+  } else {
+    Fields.push_back(createFieldType("__isa", Context.VoidPtrTy, Loc, AS_public,
+                                     BlockLayout.getElementOffsetInBits(0),
+                                     Unit, Unit));
+    Fields.push_back(createFieldType("__flags", Context.IntTy, Loc, AS_public,
+                                     BlockLayout.getElementOffsetInBits(1),
+                                     Unit, Unit));
+    Fields.push_back(
+        createFieldType("__reserved", Context.IntTy, Loc, AS_public,
+                        BlockLayout.getElementOffsetInBits(2), Unit, Unit));
+    auto *FnTy = Block.getBlockExpr()->getFunctionType();
+    auto FnPtrType = CGM.getContext().getPointerType(FnTy->desugar());
+    Fields.push_back(createFieldType("__FuncPtr", FnPtrType, Loc, AS_public,
+                                     BlockLayout.getElementOffsetInBits(3),
+                                     Unit, Unit));
+    Fields.push_back(createFieldType(
+        "__descriptor",
+        Context.getPointerType(Block.NeedsCopyDispose
+                                   ? Context.getBlockDescriptorExtendedType()
+                                   : Context.getBlockDescriptorType()),
+        Loc, AS_public, BlockLayout.getElementOffsetInBits(4), Unit, Unit));
+  }
+}
+
 void CGDebugInfo::EmitDeclareOfBlockLiteralArgVariable(const CGBlockInfo &block,
                                                        StringRef Name,
                                                        unsigned ArgNo,
                                                        llvm::AllocaInst *Alloca,
                                                        CGBuilderTy &Builder) {
   assert(DebugKind >= codegenoptions::LimitedDebugInfo);
   ASTContext &C = CGM.getContext();
   const BlockDecl *blockDecl = block.getBlockDecl();
 
   // Collect some general information about the block's location.
   SourceLocation loc = blockDecl->getCaretLocation();
   llvm::DIFile *tunit = getOrCreateFile(loc);
   unsigned line = getLineNumber(loc);
   unsigned column = getColumnNumber(loc);
 
   // Build the debug-info type for the block literal.
   getDeclContextDescriptor(blockDecl);
 
   const llvm::StructLayout *blockLayout =
       CGM.getDataLayout().getStructLayout(block.StructureType);
 
   SmallVector<llvm::Metadata *, 16> fields;
-  if (CGM.getLangOpts().OpenCL) {
-    fields.push_back(createFieldType("__size", C.IntTy, loc, AS_public,
-                                     blockLayout->getElementOffsetInBits(0),
-                                     tunit, tunit));
-    fields.push_back(createFieldType("__align", C.IntTy, loc, AS_public,
-                                     blockLayout->getElementOffsetInBits(1),
-                                     tunit, tunit));
-  } else {
-    fields.push_back(createFieldType("__isa", C.VoidPtrTy, loc, AS_public,
-                                     blockLayout->getElementOffsetInBits(0),
-                                     tunit, tunit));
-    fields.push_back(createFieldType("__flags", C.IntTy, loc, AS_public,
-                                     blockLayout->getElementOffsetInBits(1),
-                                     tunit, tunit));
-    fields.push_back(createFieldType("__reserved", C.IntTy, loc, AS_public,
-                                     blockLayout->getElementOffsetInBits(2),
-                                     tunit, tunit));
-    auto *FnTy = block.getBlockExpr()->getFunctionType();
-    auto FnPtrType = CGM.getContext().getPointerType(FnTy->desugar());
-    fields.push_back(createFieldType("__FuncPtr", FnPtrType, loc, AS_public,
-                                     blockLayout->getElementOffsetInBits(3),
-                                     tunit, tunit));
-    fields.push_back(createFieldType(
-        "__descriptor",
-        C.getPointerType(block.NeedsCopyDispose
-                             ? C.getBlockDescriptorExtendedType()
-                             : C.getBlockDescriptorType()),
-        loc, AS_public, blockLayout->getElementOffsetInBits(4), tunit, tunit));
-  }
+  collectDefaultFieldsForBlockLiteralDeclare(block, C, loc, *blockLayout, tunit,
+                                             fields);
 
   // We want to sort the captures by offset, not because DWARF
   // requires this, but because we're paranoid about debuggers.
   SmallVector<BlockLayoutChunk, 8> chunks;
 
   // 'this' capture.
   if (blockDecl->capturesCXXThis()) {
     BlockLayoutChunk chunk;

cfe/trunk/test/CodeGenOpenCL/blocks.cl

-// RUN: %clang_cc1 %s -cl-std=CL2.0 -emit-llvm -o - -O0 -debug-info-kind=limited -triple spir-unknown-unknown | FileCheck -check-prefixes=COMMON,SPIR %s
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -emit-llvm -o - -O0 -debug-info-kind=limited -triple amdgcn-amd-amdhsa | FileCheck -check-prefixes=COMMON,AMDGCN %s
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -emit-llvm -o - -O0 -triple spir-unknown-unknown | FileCheck -check-prefixes=COMMON,SPIR %s
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -emit-llvm -o - -O0 -triple amdgcn-amd-amdhsa | FileCheck -check-prefixes=COMMON,AMDGCN %s
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -emit-llvm -o - -O0 -debug-info-kind=limited -triple spir-unknown-unknown | FileCheck -check-prefixes=CHECK-DEBUG %s
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -emit-llvm -o - -O0 -debug-info-kind=limited -triple amdgcn-amd-amdhsa | FileCheck -check-prefixes=CHECK-DEBUG %s
 
 // COMMON: @__block_literal_global = internal addrspace(1) constant { i32, i32 } { i32 8, i32 4 }
 // COMMON-NOT: .str
 
 // SPIR-LABEL: define internal {{.*}}void @block_A_block_invoke(i8 addrspace(4)* %.block_descriptor, i8 addrspace(3)* %a)
 // AMDGCN-LABEL: define internal {{.*}}void @block_A_block_invoke(i8* %.block_descriptor, i8 addrspace(3)* %a)
 void (^block_A)(local void *) = ^(local void *a) {
   return;

 // SPIR:  %[[block_capture:.*]] = load i32, i32 addrspace(4)* %[[block_capture_addr]]
 // AMDGCN-LABEL: define internal {{.*}}i32 @__foo_block_invoke(i8* %.block_descriptor)
 // AMDGCN:  %[[block:.*]] = bitcast i8* %.block_descriptor to <{ i32, i32, i32 }>*
 // AMDGCN:  %[[block_capture_addr:.*]] = getelementptr inbounds <{ i32, i32, i32 }>, <{ i32, i32, i32 }>* %[[block]], i32 0, i32 2
 // AMDGCN:  %[[block_capture:.*]] = load i32, i32* %[[block_capture_addr]]
 
 // COMMON-NOT: define{{.*}}@__foo_block_invoke_kernel
 
-// COMMON: !DIDerivedType(tag: DW_TAG_member, name: "__size"
-// COMMON: !DIDerivedType(tag: DW_TAG_member, name: "__align"
+// CHECK-DEBUG: !DIDerivedType(tag: DW_TAG_member, name: "__size"
+// CHECK-DEBUG: !DIDerivedType(tag: DW_TAG_member, name: "__align"
 
-// COMMON-NOT: !DIDerivedType(tag: DW_TAG_member, name: "__isa"
-// COMMON-NOT: !DIDerivedType(tag: DW_TAG_member, name: "__flags"
-// COMMON-NOT: !DIDerivedType(tag: DW_TAG_member, name: "__reserved"
-// COMMON-NOT: !DIDerivedType(tag: DW_TAG_member, name: "__FuncPtr"
+// CHECK-DEBUG-NOT: !DIDerivedType(tag: DW_TAG_member, name: "__isa"
+// CHECK-DEBUG-NOT: !DIDerivedType(tag: DW_TAG_member, name: "__flags"
+// CHECK-DEBUG-NOT: !DIDerivedType(tag: DW_TAG_member, name: "__reserved"
+// CHECK-DEBUG-NOT: !DIDerivedType(tag: DW_TAG_member, name: "__FuncPtr"