Page MenuHomePhabricator

[DebugInfo] Fix ICE in DwarfCompileUnit::constructSubprogramScopeDIE
AcceptedPublic

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

Unit TestsFailed

TimeTest
410 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp

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.