[DWARFUnit] Don't assume basic types.

[davide]

+ llvm-commits.

[davide]

update tests

[JDevlieghere]

Can we further reduce this test case? If not this LGTM.

[aprantl]

Is the problem that it is a SubroutineType, but not a function *pointer*?
Generally, if the Verifier accepts it we ought to not crash, so this seems to be okay, but it might be worth asking whether this is really what you want your frontend to be creating since subroutinetypes don't have a size, etc.

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp

472

Why is false the right answer here?

[davide]

Yes, the problem is that this is a Subroutine type. We might as well reject this altogether in the verifier, what do you think?

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp

472

To be quite honest, I think it's irrelevant (in the sense that it doesn't matter what we return here).

[davide]

Actually, on a second thought, this seems perfectly legit (although not really something useful to emit), so I'm leaning towards leaving the patch as is.
We can discuss whether true or false is the right answer, I don't have a preference there. I simply have hard time imagining what's "right" to do for a subroutine.

[andrewrk]

In D56297#1345884, @aprantl wrote:

Is the problem that it is a SubroutineType, but not a function *pointer*?
Generally, if the Verifier accepts it we ought to not crash, so this seems to be okay, but it might be worth asking whether this is really what you want your frontend to be creating since subroutinetypes don't have a size, etc.

I'm the author of the frontend that is generating this code. Thanks for the explanation of the problem. Based on this I tried similar code with clang:

static void another(void) {}

int main() {
    void (*x)(void) = another;
    return 0;
}

To find out how it represented the debug info:

  call void @llvm.dbg.declare(metadata void ()** %2, metadata !11, metadata !DIExpression()), !dbg !15
!11 = !DILocalVariable(name: "x", scope: !7, file: !1, line: 4, type: !12)
!12 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !13, size: 64)
!13 = !DISubroutineType(types: !14)

So it looks like I should modify the frontend to represent this as a DerivedType with a baseType=SubroutineType. Is that right?

For what it's worth, I would consider this issue resolved if the verifier would reject the code. Although I will note that this assertion is not tripped in 7.0.0 for the same code.

[andrewrk]

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp

472

It seems to me that the question "is a subroutine an unsigned type" is a nonsensical question, and should be an assertion error (as it is in status quo trunk).

Or otherwise if a subroutine can be treated implicitly as a pointer, then it should have the same answer that a pointer would have, which is true.

[probinson]

In D56297#1346801, @andrewrk wrote:

So it looks like I should modify the frontend to represent this as a DerivedType with a baseType=SubroutineType. Is that right?

A DerivedType that's a pointer, yes. If that accurately describes your source.

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp

472

I agree that asking whether a subroutine type is an unsigned type makes no sense. All contexts where we call isUnsignedDIType() are about emitting constant values. Why are we asking about a subroutine type?

llvm/test/Assembler/DIAsmunsigned.ll

2

This looks like Darwin naming, you probably want an explicit triple on the RUN line.

[davide]

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp

472

Is there anything that makes you think we should treat this implicitly as a pointer? I think this a very strong assumption to make in general.

[davide]

llvm/test/Assembler/DIAsmunsigned.ll

2

Correct, I'll relax that.

[andrewrk]

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp

472

If you're asking for my opinion on this issue, what I think should happen is:

• No change to DwarfUnit.cpp
• Verifier gains support to reject the example IR as invalid

[aprantl]

In D56297#1346801, @andrewrk wrote:

In D56297#1345884, @aprantl wrote:

Is the problem that it is a SubroutineType, but not a function *pointer*?
Generally, if the Verifier accepts it we ought to not crash, so this seems to be okay, but it might be worth asking whether this is really what you want your frontend to be creating since subroutinetypes don't have a size, etc.

I'm the author of the frontend that is generating this code. Thanks for the explanation of the problem. Based on this I tried similar code with clang:

So it looks like I should modify the frontend to represent this as a DerivedType with a baseType=SubroutineType. Is that right?

Assuming that what your frontend produces is actually a function pointer or reference (and not the function *type*), yes.

For what it's worth, I would consider this issue resolved if the verifier would reject the code. Although I will note that this assertion is not tripped in 7.0.0 for the same code.

Correct. Any input that the Verifier accepts must be accepted by LLVM. We can fix this by either making the Verifier stricter (usually preferred), or by fixing LLVM.

[davide]

Superseded by: https://reviews.llvm.org/D56414

llvm/lib/CodeGen/AsmPrinter/DwarfUnit.cpp

if (auto *DTy = dyn_cast<DIDerivedType>(Ty)) {
     assert(T == dwarf::DW_TAG_typedef || T == dwarf::DW_TAG_const_type ||
            T == dwarf::DW_TAG_volatile_type ||
            T == dwarf::DW_TAG_restrict_type || T == dwarf::DW_TAG_atomic_type);
     DITypeRef Deriv = DTy->getBaseType();
     assert(Deriv && "Expected valid base type");
     return isUnsignedDIType(DD, DD->resolve(Deriv));
   }
 
-  auto *BTy = cast<DIBasicType>(Ty);
+  auto *BTy = dyn_cast<DIBasicType>(Ty);
+  if (!BTy)
+    return false;
   unsigned Encoding = BTy->getEncoding();
   assert((Encoding == dwarf::DW_ATE_unsigned ||
           Encoding == dwarf::DW_ATE_unsigned_char ||
           Encoding == dwarf::DW_ATE_signed ||
           Encoding == dwarf::DW_ATE_signed_char ||
           Encoding == dwarf::DW_ATE_float || Encoding == dwarf::DW_ATE_UTF ||
           Encoding == dwarf::DW_ATE_boolean ||
           (Ty->getTag() == dwarf::DW_TAG_unspecified_type &&

llvm/test/Assembler/DIAsmunsigned.ll

+; RUN: llc < %s | FileCheck %s
+; CHECK:        .section        __DWARF,__debug_str,regular,debug
+; CHECK-NEXT: Linfo_string:
+; CHECK_NEXT:        .asciz  "zig 0.3.0"             ## string offset=0
+
+
+%timespec.0.1.2.3.0.1.2 = type { i64, i64 }
+define internal i64 @init_vdso_clock_gettime(i32, %timespec.0.1.2.3.0.1.2* nonnull) unnamed_addr !dbg !142 {
+  call void @llvm.dbg.value(metadata i64 (i32, %timespec.0.1.2.3.0.1.2*)* null, metadata !162, metadata !DIExpression()), !dbg !167
+  ret i64 -38, !dbg !168
+}
+declare void @llvm.dbg.value(metadata, metadata, metadata) #0
+!llvm.module.flags = !{!0}
+!llvm.dbg.cu = !{!1}
+!0 = !{i32 2, !"Debug Info Version", i32 3}
+!1 = distinct !DICompileUnit(language: DW_LANG_C99, file: !2, producer: "zig 0.3.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !3, globals: !76)
+!2 = !DIFile(filename: "test", directory: ".")
+!3 = !{!4}
+!4 = !DICompositeType(tag: DW_TAG_enumeration_type, name: "Arch", scope: !5, file: !5, line: 44, baseType: !6, size: 8, align: 8, elements: !7)
+!5 = !DIFile(filename: "builtin.zig", directory: "/home/andy/.local/share/zig/stage1/builtin/ugMGxVES9OkDAffv3xhJS3KQVy0Wm1xPM3Bc6x4MBuup5aetdi5pVTrGRG2aDAn0")
+!6 = !DIBasicType(name: "u7", size: 8, encoding: DW_ATE_unsigned)
+!7 = !{!8}
+!8 = !DIEnumerator(name: "armv8_5a", value: 0)
+!76 = !{!77}
+!77 = !DIGlobalVariableExpression(var: !78, expr: !DIExpression())
+!78 = distinct !DIGlobalVariable(name: "arch", linkageName: "arch", scope: !5, file: !5, line: 437, type: !4, isLocal: true, isDefinition: true)
+!81 = !DIFile(filename: "index.zig", directory: "/store/dev/zig/build-llvm8-debug/lib/zig/std/os/linux")
+!142 = distinct !DISubprogram(name: "init_vdso_clock_gettime", scope: !81, file: !81, line: 968, type: !143, scopeLine: 968, flags: DIFlagStaticMember, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition | DISPFlagOptimized, unit: !1, retainedNodes: !153)
+!143 = !DISubroutineType(types: !144)
+!144 = !{!145}
+!145 = !DIBasicType(name: "usize", size: 64, encoding: DW_ATE_unsigned)
+!146 = !DIBasicType(name: "i32", size: 32, encoding: DW_ATE_signed)
+!153 = !{!154}
+!154 = !DILocalVariable(name: "clk", arg: 1, scope: !142, file: !81, line: 968, type: !146)
+!162 = !DILocalVariable(name: "f", scope: !163, file: !81, line: 970, type: !143)
+!163 = distinct !DILexicalBlock(scope: !164, file: !81, line: 969, column: 5)
+!164 = distinct !DILexicalBlock(scope: !165, file: !81, line: 968, column: 66)
+!165 = distinct !DILexicalBlock(scope: !166, file: !81, line: 968, column: 45)
+!166 = distinct !DILexicalBlock(scope: !142, file: !81, line: 968, column: 35)
+!167 = !DILocation(line: 970, column: 5, scope: !163)
+!168 = !DILocation(line: 972, column: 28, scope: !169)
+!169 = distinct !DILexicalBlock(scope: !163, file: !81, line: 970, column: 5)