Page MenuHomePhabricator

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

Authored by scott.linder on Wed, Oct 28, 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
340 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
10 mslinux > LLVM-Unit.IR/_/IRTests::VerifierTest.DetectInvalidDebugInfo
Note: Google Test filter = VerifierTest.DetectInvalidDebugInfo [==========] Running 1 test from 1 test case. [----------] Global test environment set-up.
10 mslinux > LLVM-Unit.Transforms/Utils/_/UtilsTests::CloneFunction.CloneFunctionToDifferentModule
Note: Google Test filter = CloneFunction.CloneFunctionToDifferentModule [==========] Running 1 test from 1 test case. [----------] Global test environment set-up.
20 mslinux > LLVM.Assembler::debug-info-source-invalid.ll
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llvm-as < /mnt/disks/ssd0/agent/llvm-project/llvm/test/Assembler/debug-info-source-invalid.ll 2>&1 >/dev/null | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/Assembler/debug-info-source-invalid.ll
20 mslinux > LLVM.Assembler::debug-info-source.ll
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llvm-as < /mnt/disks/ssd0/agent/llvm-project/llvm/test/Assembler/debug-info-source.ll | /mnt/disks/ssd0/agent/llvm-project/build/bin/llvm-dis | /mnt/disks/ssd0/agent/llvm-project/build/bin/llvm-as | /mnt/disks/ssd0/agent/llvm-project/build/bin/llvm-dis | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/Assembler/debug-info-source.ll
View Full Test Results (59 Failed)

Event Timeline

scott.linder created this revision.Wed, Oct 28, 2:14 PM
scott.linder requested review of this revision.Wed, Oct 28, 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?