[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.