Fix faulty assertion for void type in debug info

[amccarth]

Summary

It appears the code uses nullptr to represent a void type in debug metadata, which led to an assertion failure when building DeltaAlgorithm.cpp with a self-hosted clang on Windows.

The problem occurs when selecting both Dwarf and CodeView. The CodeView causes some additional DI records (for nested typedefs) to be emitted. The Dwarf handler wasn't prepared for these records when they reference void type (which is represented internally by a null pointer).

Fixes bug https://bugs.llvm.org/show_bug.cgi?id=35543

[aprantl]

That seems suspicious. Do you have an llvm IR testcase and does the Verifier accept it?

llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp

167 ↗

(On Diff #127048)

That seems suspicious. Do you have an llvm IR testcase and does the Verifier accept it?

[amccarth]

In D41264#956840, @aprantl wrote:

That seems suspicious. Do you have an llvm IR testcase and does the Verifier accept it?

I'm not happy with it either. I put this up hoping to get some tips. I have a .cpp test (see the linked bug report). I'll reduce it to an IR test.

[amccarth]

Made part of the fix more localized, preserving one of the assertions.

Added an IR test that demonstrates the problem.

rnk succinctly described the problem in the bug report:

Right, this bug only occurs if the user asks for *both* DWARF and CodeView. We don't really support that configuration, but we didn't take any steps to disable it because there's nothing technically challenging about allowing it.

In this case, what's happening is we're emitting extra DI metadata (nested typedefs, in this case the one for 'void') to support CodeView, and the DWARF asm printer doesn't seem ready for it.

[aprantl]

Looks like CGDebugInfo::CreateType creates void as nullptr. If me modify the testcase to use the typedef

class A {
  typedef void _Nodeptr;
  _Nodeptr * n;
};

we can force it to be emitted even in DWARF:

0x00000068:     DW_TAG_typedef
                  DW_AT_name	("_Nodeptr")
                  DW_AT_decl_file	("/tmp/t.cpp")
                  DW_AT_decl_line	(2)

0x0000006f:     NULL

this also looks weird, but is apparently sort of legal DWARF:

For example, in C and C++ the language implementation can provide an unspecified type entry with the name “void” which can be referenced by the type attribute of pointer types and typedef declarations for ’void’.

although it would be nicer to use DW_TAG_unspecified_type instead.

Based on this it looks like we should allow this case.

llvm/test/DebugInfo/void-typedef.ll

34 ↗

(On Diff #127200)

We may not even need the function for the testcase to work, you might get away with removing the function, adding the type to the retained types in the CU and then running the file through llvm-as - | llvm-dis to get rid of all unreachable MDNodes.

[probinson]

In D41264#959575, @aprantl wrote:

0x00000068:     DW_TAG_typedef
                  DW_AT_name	("_Nodeptr")
                  DW_AT_decl_file	("/tmp/t.cpp")
                  DW_AT_decl_line	(2)

0x0000006f:     NULL

this also looks weird, but is apparently sort of legal DWARF:

For example, in C and C++ the language implementation can provide an unspecified type entry with the name “void” which can be referenced by the type attribute of pointer types and typedef declarations for ’void’.

although it would be nicer to use DW_TAG_unspecified_type instead.

Based on this it looks like we should allow this case.

I didn't think it was legal either, but the description of DW_TAG_typedef does say it "may" have a DW_AT_type, and even discusses when it's appropriate to leave it out.

Using void as the return type of a subprogram is explicitly represented as omitting DW_AT_type from the subprogram DIE, and that's probably where the nullptr idea came from.

A typedef of void * goes to a DW_TAG_pointer_type which is required to have at DW_AT_type, which I think we do emit as DW_TAG_unspecified_type. So, for consistency I'd think we might want typedef to do something similar. But, omitting is legal.

llvm/trunk/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp

uint64_t DebugHandlerBase::getBaseTypeSize(const DITypeRef TyRef) {
 
   if (Tag != dwarf::DW_TAG_member && Tag != dwarf::DW_TAG_typedef &&
       Tag != dwarf::DW_TAG_const_type && Tag != dwarf::DW_TAG_volatile_type &&
       Tag != dwarf::DW_TAG_restrict_type && Tag != dwarf::DW_TAG_atomic_type)
     return DDTy->getSizeInBits();
 
   DIType *BaseType = DDTy->getBaseType().resolve();
 
-  assert(BaseType && "Unexpected invalid base type");
+  if (!BaseType)
+    return 0;
 
   // If this is a derived type, go ahead and get the base type, unless it's a
   // reference then it's just the size of the field. Pointer types have no need
   // of this since they're a different type of qualification on the type.
   if (BaseType->getTag() == dwarf::DW_TAG_reference_type ||
       BaseType->getTag() == dwarf::DW_TAG_rvalue_reference_type)
     return Ty->getSizeInBits();
 

llvm/trunk/lib/CodeGen/AsmPrinter/DwarfUnit.cpp

 }
 
 void DwarfUnit::constructMemberDIE(DIE &Buffer, const DIDerivedType *DT) {
   DIE &MemberDie = createAndAddDIE(DT->getTag(), Buffer);
   StringRef Name = DT->getName();
   if (!Name.empty())
     addString(MemberDie, dwarf::DW_AT_name, Name);
 
-  addType(MemberDie, resolve(DT->getBaseType()));
+  if (DIType *Resolved = resolve(DT->getBaseType()))
+    addType(MemberDie, Resolved);
 
   addSourceLine(MemberDie, DT);
 
   if (DT->getTag() == dwarf::DW_TAG_inheritance && DT->isVirtual()) {
 
     // For C++, virtual base classes are not at fixed offset. Use following
     // expression to extract appropriate offset from vtable.
     // BaseAddr = ObAddr + *((*ObAddr) - Offset)

llvm/trunk/test/DebugInfo/void-typedef.ll

+; Choosing CodeView generates debug metadata for class-scope typedefs that
+; Dwarf would normally omit.  Choosing both CodeView and Dwarf triggered
+; assertion failures and crashes because the Dwarf handler wasn't prepared for
+; those records (in particular, ones with the void type represented by a
+; null pointer).
+;
+; This test was generated with:
+;    clang++ -cc1 -emit-llvm -debug-info-kind=limited -dwarf-version=4 -gcodeview -x c++
+; on the following source code:
+;
+;   class A {
+;     typedef void _Nodeptr;
+;   };
+;   class B {
+;     A FailedTestsCache;
+;     bool m_fn1();
+;   };
+;   bool B::m_fn1() {}
+;
+; CodeView generates a DIDerivedType for the _Nodeptr typedef.
+;
+; RUN: llc %s -o - 2>&1 | FileCheck %s
+; CHECK-NOT: Assertion failed
+
+; ModuleID = 'bug.cpp'
+source_filename = "bug.cpp"
+target datalayout = "e-m:x-p:32:32-i64:64-f80:32-n8:16:32-a:0:32-S32"
+target triple = "i686-pc-windows-msvc"
+
+%class.B = type { %class.A }
+%class.A = type { i8 }
+
+; Function Attrs: noinline nounwind optnone
+define x86_thiscallcc zeroext i1 @"\01?m_fn1@B@@AAE_NXZ"(%class.B* %this) #0 align 2 !dbg !9 {
+entry:
+  %retval = alloca i1, align 1
+  %this.addr = alloca %class.B*, align 4
+  store %class.B* %this, %class.B** %this.addr, align 4
+  call void @llvm.dbg.declare(metadata %class.B** %this.addr, metadata !22, metadata !DIExpression()), !dbg !24
+  %this1 = load %class.B*, %class.B** %this.addr, align 4
+  call void @llvm.trap(), !dbg !25
+  unreachable, !dbg !25
+
+return:                                           ; No predecessors!
+  %0 = load i1, i1* %retval, align 1, !dbg !25
+  ret i1 %0, !dbg !25
+}
+
+; Function Attrs: nounwind readnone speculatable
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+; Function Attrs: noreturn nounwind
+declare void @llvm.trap() #2
+
+attributes #0 = { noinline nounwind optnone "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-features"="+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { nounwind readnone speculatable }
+attributes #2 = { noreturn nounwind }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5, !6, !7}
+!llvm.ident = !{!8}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 6.0.0 ", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "<stdin>", directory: "D:\5Csrc\5Cbug", checksumkind: CSK_MD5, checksum: "2216f11c5ddda8c48a6f92a6079ad4b6")
+!2 = !{}
+!3 = !{i32 1, !"NumRegisterParameters", i32 0}
+!4 = !{i32 2, !"Dwarf Version", i32 4}
+!5 = !{i32 2, !"CodeView", i32 1}
+!6 = !{i32 2, !"Debug Info Version", i32 3}
+!7 = !{i32 1, !"wchar_size", i32 2}
+!8 = !{!"clang version 6.0.0 "}
+!9 = distinct !DISubprogram(name: "m_fn1", linkageName: "\01?m_fn1@B@@AAE_NXZ", scope: !11, file: !10, line: 8, type: !18, isLocal: false, isDefinition: true, scopeLine: 8, flags: DIFlagPrototyped, isOptimized: false, unit: !0, declaration: !17, variables: !2)
+!10 = !DIFile(filename: "bug.cpp", directory: "D:\5Csrc\5Cbug", checksumkind: CSK_MD5, checksum: "2216f11c5ddda8c48a6f92a6079ad4b6")
+!11 = distinct !DICompositeType(tag: DW_TAG_class_type, name: "B", file: !10, line: 4, size: 8, elements: !12, identifier: ".?AVB@@")
+!12 = !{!13, !17}
+!13 = !DIDerivedType(tag: DW_TAG_member, name: "FailedTestsCache", scope: !11, file: !10, line: 5, baseType: !14, size: 8)
+!14 = distinct !DICompositeType(tag: DW_TAG_class_type, name: "A", file: !10, line: 1, size: 8, elements: !15, identifier: ".?AVA@@")
+!15 = !{!16}
+!16 = !DIDerivedType(tag: DW_TAG_typedef, name: "_Nodeptr", scope: !14, file: !10, line: 2, baseType: null)
+!17 = !DISubprogram(name: "m_fn1", linkageName: "\01?m_fn1@B@@AAE_NXZ", scope: !11, file: !10, line: 6, type: !18, isLocal: false, isDefinition: false, scopeLine: 6, flags: DIFlagPrototyped, isOptimized: false)
+!18 = !DISubroutineType(cc: DW_CC_BORLAND_thiscall, types: !19)
+!19 = !{!20, !21}
+!20 = !DIBasicType(name: "bool", size: 8, encoding: DW_ATE_boolean)
+!21 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !11, size: 32, flags: DIFlagArtificial | DIFlagObjectPointer)
+!22 = !DILocalVariable(name: "this", arg: 1, scope: !9, type: !23, flags: DIFlagArtificial | DIFlagObjectPointer)
+!23 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !11, size: 32)
+!24 = !DILocation(line: 0, scope: !9)
+!25 = !DILocation(line: 8, scope: !9)