[DebugInfo] Fix ICE in DwarfCompileUnit::constructSubprogramScopeDIE

[scott.linder]

Summary

Add a check to the verifier so we don't ICE when generating DWARF debug
information for a DISubprogram that is missing a type:

[dblaikie]

IF we've so far successfully made the assumption that all DISubprograms have types - I'd say updating the verifier would be the way to go here. But I defer to @aprantl on verifier things, if he's got an overriding/differing opinion.

[scott.linder]

In D90345#2360699, @dblaikie wrote:

IF we've so far successfully made the assumption that all DISubprograms have types - I'd say updating the verifier would be the way to go here. But I defer to @aprantl on verifier things, if he's got an overriding/differing opinion.

Thank you for the feedback, I wasn't sure which way to lean so I just flipped a coin, as it were :^)

I think ideally the types of these accessors would mirror the requirements and e.g. return an Option for anything not strictly required by the Verifier. I was also wondering if people would be amenable to changing the LangRef entries for the DI* metadata nodes to be explicit, for each field:

• When it is required
• When it is forbidden (including e.g. mutual exclusion with other fields)
• Which "types" of metadata it can point to

Currently it seems like we do this for some fields but not for others, and without anything in the type system to enforce things there isn't really any ground truth to work off of.

If nobody has major objections I'd like to try to work on a patch when I have time.

[scott.linder]

In D90345#2362055, @scott.linder wrote:

In D90345#2360699, @dblaikie wrote:

IF we've so far successfully made the assumption that all DISubprograms have types - I'd say updating the verifier would be the way to go here. But I defer to @aprantl on verifier things, if he's got an overriding/differing opinion.

Thank you for the feedback, I wasn't sure which way to lean so I just flipped a coin, as it were :^)

I think ideally the types of these accessors would mirror the requirements and e.g. return an Option for anything not strictly required by the Verifier. I was also wondering if people would be amenable to changing the LangRef entries for the DI* metadata nodes to be explicit, for each field:

• When it is required
• When it is forbidden (including e.g. mutual exclusion with other fields)
• Which "types" of metadata it can point to

Currently it seems like we do this for some fields but not for others, and without anything in the type system to enforce things there isn't really any ground truth to work off of.

If nobody has major objections I'd like to try to work on a patch when I have time.

Also, clangd seems to think this is the only use of DISubprogram::getType(), which doesn't seem quite right, but I went along with the idea that nowhere else is assuming type: is present.

[dblaikie]

In D90345#2362082, @scott.linder wrote:

In D90345#2362055, @scott.linder wrote:

In D90345#2360699, @dblaikie wrote:

IF we've so far successfully made the assumption that all DISubprograms have types - I'd say updating the verifier would be the way to go here. But I defer to @aprantl on verifier things, if he's got an overriding/differing opinion.

Thank you for the feedback, I wasn't sure which way to lean so I just flipped a coin, as it were :^)

I think ideally the types of these accessors would mirror the requirements and e.g. return an Option for anything not strictly required by the Verifier. I was also wondering if people would be amenable to changing the LangRef entries for the DI* metadata nodes to be explicit, for each field:

• When it is required
• When it is forbidden (including e.g. mutual exclusion with other fields)
• Which "types" of metadata it can point to

Currently it seems like we do this for some fields but not for others, and without anything in the type system to enforce things there isn't really any ground truth to work off of.

If nobody has major objections I'd like to try to work on a patch when I have time.

Sounds plausible to me.

Also, clangd seems to think this is the only use of DISubprogram::getType(), which doesn't seem quite right, but I went along with the idea that nowhere else is assuming type: is present.

Seems plausible to me - most of the DI info is only used once to generate the resulting DWARF or CodeView debug info at the end (yeah, I guess it's somewhat noteworthy that it's not used in the CodeView generation code).

[scott.linder]

Address feedback by requiring type:

[dblaikie]

@aprantl @JDevlieghere you two have more investment and familiarity with the verifier - mind checking this one over?

[aprantl]

I think this looks reasonable.

[scott.linder]

OK, thank you both for taking a look! I didn't go and update all of the failing test cases at first, because I wanted to avoid it if we decided to go the "type: is optional" route. I'll update them all and post to the review.

[scott.linder]

I finally got around to updating the tests, and ran into the issue of supporting old bitcode which lacks a type: for DISubprograms

The only way I can see of "upgrading" these bitcodes is to pick some arbitrary DISubroutineType to use in place of null, likely !DISubroutineType(types: !{null}). This isn't correct in any sense, but if we are going to require the type: field it seems like the best we can do.

Thoughts?

[dblaikie]

In D90345#2853542, @scott.linder wrote:

I finally got around to updating the tests, and ran into the issue of supporting old bitcode which lacks a type: for DISubprograms

The only way I can see of "upgrading" these bitcodes is to pick some arbitrary DISubroutineType to use in place of null, likely !DISubroutineType(types: !{null}). This isn't correct in any sense, but if we are going to require the type: field it seems like the best we can do.

Thoughts?

Perhaps an empty types list, rather than a list containing a single null element? I guess an empty list corresponds to the "void()" type & that seems OK-ish?

I guess if we've been crashing on it previously, maybe it's OK not to make this backwards compatible.

[scott.linder]

Update tests.

I went the route of "upgrading" bitcode which lacks the type: field on DISubprograms. It seems like there is currently no difference from LLVM's perspective between !DISubroutineType(types: !{}) and !DISubroutineType(types: !{null}), or at least the difference (or whether the first form should even be valid) is not clear to me.

As an example, DebugTypeInfoRemoval uses !DISubroutineType(types: !{}) to represent the C type (void)(), but the langref would lead one to believe this should be !DISubroutineType(types: !{null}).

Does anyone have any thoughts?

[scott.linder]

Fix up more of the test changes to use !DISubroutineType(!{}). I think I've
got them all now, I was looking at a partial diff before.

[scott.linder]

I also ended up writing a script to make updating the tests easier and I'm wondering if anyone sees merit in putting it in llvm/utils. It (crudely) inserts MDNodes at given indices in IR, updating all references to the old IDs:

#!/usr/bin/perl
use strict;
use warnings;

# Make room in LLVM IR for new MDNode IDs by incrementing references to MDNode
# IDs in stdin, writing the result to stdout. The list of new MDNode IDs is
# read from argv, one ID per argument.
#
# Note: Just uses a regex which matches on an exclamation point followed by a
# sequence of digits. This may be incorrect in some cases, so always make sure
# the result looks how you intend.

my @ids = sort grep /^\d+$/, @ARGV;

while (<STDIN>) {
    foreach my $id (@ids) {
        print "!$id = \n" if /^!$id = /;
        s/!(\d+)/if ($1 < $id) {"!" . $1} else {"!" . ($1 + 1)}/eg;
    }
    print;
}

[dblaikie]

In D90345#2887719, @scott.linder wrote:

Update tests.

I went the route of "upgrading" bitcode which lacks the type: field on DISubprograms. It seems like there is currently no difference from LLVM's perspective between !DISubroutineType(types: !{}) and !DISubroutineType(types: !{null}), or at least the difference (or whether the first form should even be valid) is not clear to me.

As an example, DebugTypeInfoRemoval uses !DISubroutineType(types: !{}) to represent the C type (void)(), but the langref would lead one to believe this should be !DISubroutineType(types: !{null}).

Does anyone have any thoughts?

I'd lean slightly towards !{null} being the representation for a known signature of void() - for consistency.

[scott.linder]

In D90345#2888514, @dblaikie wrote:

In D90345#2887719, @scott.linder wrote:

Update tests.

I went the route of "upgrading" bitcode which lacks the type: field on DISubprograms. It seems like there is currently no difference from LLVM's perspective between !DISubroutineType(types: !{}) and !DISubroutineType(types: !{null}), or at least the difference (or whether the first form should even be valid) is not clear to me.

As an example, DebugTypeInfoRemoval uses !DISubroutineType(types: !{}) to represent the C type (void)(), but the langref would lead one to believe this should be !DISubroutineType(types: !{null}).

Does anyone have any thoughts?

I'd lean slightly towards !{null} being the representation for a known signature of void() - for consistency.

Would it be worth preceding this patch with another that enforces this? It seems like a verifier check, doc update, and bitcode reader upgrade path from !{} to !{null} for DISubroutineType would clear things up. If so, I can do that and then update this patch.

[dblaikie]

In D90345#2888538, @scott.linder wrote:

In D90345#2888514, @dblaikie wrote:

In D90345#2887719, @scott.linder wrote:

Update tests.

I went the route of "upgrading" bitcode which lacks the type: field on DISubprograms. It seems like there is currently no difference from LLVM's perspective between !DISubroutineType(types: !{}) and !DISubroutineType(types: !{null}), or at least the difference (or whether the first form should even be valid) is not clear to me.

As an example, DebugTypeInfoRemoval uses !DISubroutineType(types: !{}) to represent the C type (void)(), but the langref would lead one to believe this should be !DISubroutineType(types: !{null}).

Does anyone have any thoughts?

I'd lean slightly towards !{null} being the representation for a known signature of void() - for consistency.

Would it be worth preceding this patch with another that enforces this? It seems like a verifier check, doc update, and bitcode reader upgrade path from !{} to !{null} for DISubroutineType would clear things up. If so, I can do that and then update this patch.

Maybe? Though might be more effort than its worth - it's probably pretty harmless for !{null} and !{} to be treated the same - especially if they are already supported? But if we're trying to pick a representation to use in new cases where it comes up I'd lean towards !{null}.

Open to other opinions - @aprantl and co have more context/care more about the verifier .

[aprantl]

In D90345#2888746, @dblaikie wrote:

In D90345#2888538, @scott.linder wrote:

In D90345#2888514, @dblaikie wrote:

In D90345#2887719, @scott.linder wrote:

Update tests.

I went the route of "upgrading" bitcode which lacks the type: field on DISubprograms. It seems like there is currently no difference from LLVM's perspective between !DISubroutineType(types: !{}) and !DISubroutineType(types: !{null}), or at least the difference (or whether the first form should even be valid) is not clear to me.

As an example, DebugTypeInfoRemoval uses !DISubroutineType(types: !{}) to represent the C type (void)(), but the langref would lead one to believe this should be !DISubroutineType(types: !{null}).

Does anyone have any thoughts?

I'd lean slightly towards !{null} being the representation for a known signature of void() - for consistency.

Would it be worth preceding this patch with another that enforces this? It seems like a verifier check, doc update, and bitcode reader upgrade path from !{} to !{null} for DISubroutineType would clear things up. If so, I can do that and then update this patch.

Maybe? Though might be more effort than its worth - it's probably pretty harmless for !{null} and !{} to be treated the same - especially if they are already supported? But if we're trying to pick a representation to use in new cases where it comes up I'd lean towards !{null}.

Open to other opinions - @aprantl and co have more context/care more about the verifier .

I also think we should go with LangRef here. Clang currently also emits void f() {} as !DISubroutineType(types: !{null}). If have the time, you could write a separate patch to add a Verifier pass that rejects an empty list and then fix up all testcases, but as @dblaikie it's pretty harmless.

llvm/docs/LangRef.rst

 """"""""""""
 
 ``DISubprogram`` nodes represent functions from the source language. A distinct
 ``DISubprogram`` may be attached to a function definition using ``!dbg``
 metadata. A unique ``DISubprogram`` may be attached to a function declaration
 used for call site debug info. The ``retainedNodes:`` field is a list of
 :ref:`variables <DILocalVariable>` and :ref:`labels <DILabel>` that must be
 retained, even if their IR counterparts are optimized out of the IR. The
-``type:`` field must point at an :ref:`DISubroutineType`.
+``type:`` field is optional, but if present it must point at a
+:ref:`DISubroutineType`.
 
 .. _DISubprogramDeclaration:
 
 When ``isDefinition: false``, subprograms describe a declaration in the type
 tree as opposed to a definition of a function.  If the scope is a composite
 type with an ODR ``identifier:`` and that does not set ``flags: DIFwdDecl``,
 then the subprogram declaration is uniqued based only on its ``linkageName:``
 and ``scope:``.

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

if (Scope) {
     // Collect lexical scope children first.
     // ObjectPointer might be a local (non-argument) local variable if it's a
     // block's synthetic this pointer.
     if (DIE *ObjectPointer = createAndAddScopeChildren(Scope, ScopeDIE))
       addDIEEntry(ScopeDIE, dwarf::DW_AT_object_pointer, *ObjectPointer);
   }
 
   // If this is a variadic function, add an unspecified parameter.
-  DITypeRefArray FnArgs = Sub->getType()->getTypeArray();
+  if (DISubroutineType *FnType = Sub->getType()) {
+    DITypeRefArray FnArgs = FnType->getTypeArray();
 
     // If we have a single element of null, it is a function that returns void.
     // If we have more than one elements and the last one is null, it is a
     // variadic function.
     if (FnArgs.size() > 1 && !FnArgs[FnArgs.size() - 1] &&
         !includeMinimalInlineScopes())
       ScopeDIE.addChild(
           DIE::get(DIEValueAllocator, dwarf::DW_TAG_unspecified_parameters));
+  }
 
   return ScopeDIE;
 }
 
 DIE *DwarfCompileUnit::createAndAddScopeChildren(LexicalScope *Scope,
                                                  DIE &ScopeDIE) {
   // We create children when the scope DIE is not null.
   SmallVector<DIE *, 8> Children;

llvm/test/CodeGen/Generic/disubprogram-no-type.ll

+; RUN: llc %s -filetype=obj -o - | llvm-dwarfdump - | FileCheck %s
+
+; Confirm we don't ICE when generating DWARF debug information for a
+; DISubprogram that is missing a `type:`
+
+; CHECK-NOT: DW_AT_type
+
+define i32 @f(i32 %0) !dbg !3 { ret i32 %0 }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2}
+
+!0 = distinct !DICompileUnit(language: 0, file: !1, emissionKind: FullDebug)
+!1 = !DIFile(filename: "-", directory: "")
+!2 = !{i32 2, !"Debug Info Version", i32 3}
+!3 = distinct !DISubprogram(file: !1, unit: !0)