This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Fix ICE in DwarfCompileUnit::constructSubprogramScopeDIE
Needs ReviewPublic

Authored by scott.linder on Oct 28 2020, 2:14 PM.

Details

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:

Diff Detail

Event Timeline

scott.linder created this revision.Oct 28 2020, 2:14 PM
scott.linder requested review of this revision.Oct 28 2020, 2:14 PM

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.

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.

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.

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 edited the summary of this revision. (Show Details)

Address feedback by requiring type:

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

aprantl accepted this revision.Dec 4 2020, 6:31 PM

I think this looks reasonable.

This revision is now accepted and ready to land.Dec 4 2020, 6:31 PM

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.

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?

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.

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?

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 requested review of this revision.Jul 19 2021, 10:29 AM

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;
}

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.

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.

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 .

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.