This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Add more tests for DISubprogram verifier
ClosedPublic

Authored by scott.linder on Oct 28 2020, 1:54 PM.

Details

Summary

Minimum amount of tests to cover (most) of the DISubprogram verifier
checks.

I noticed while writing these that there seem to be some dead code paths
in the Verifier. For example, the check for distinct when
isDefinition: true seems to be preempted by either the equivalent
check in the IR parser or the Function verifier. I didn't do anything
about these.

I'm also not certain if allowing isDefinition to be inconsistent with
the IR Function is intended?

Diff Detail

Event Timeline

scott.linder created this revision.Oct 28 2020, 1:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 28 2020, 1:54 PM
scott.linder requested review of this revision.Oct 28 2020, 1:54 PM
vsk accepted this revision.EditedOct 28 2020, 3:29 PM

Thanks, lgtm.

It looks like verifyFunction asserts that the DISubprogram attached to any function, declaration or definition, is distinct:

  } else if (F.isDeclaration()) {
    for (const auto &I : MDs) {
      // This is used for call site debug information.
      AssertDI(I.first != LLVMContext::MD_dbg ||
                   !cast<DISubprogram>(I.second)->isDistinct(),
               "function declaration may only have a unique !dbg attachment",
...
        AssertDI(cast<DISubprogram>(I.second)->isDistinct(),
                 "function definition may only have a distinct !dbg attachment",
                 &F);

It should be possible to simplify that. I don't think checking isDistinct() in verifyDISubprogram is redundant though. You might have DISubprogram metadata that's not attached to a function.

This revision is now accepted and ready to land.Oct 28 2020, 3:29 PM
In D90340#2360585, @vsk wrote:

Thanks, lgtm.

It looks like verifyFunction asserts that the DISubprogram attached to any function, declaration or definition, is distinct:

  } else if (F.isDeclaration()) {
    for (const auto &I : MDs) {
      // This is used for call site debug information.
      AssertDI(I.first != LLVMContext::MD_dbg ||
                   !cast<DISubprogram>(I.second)->isDistinct(),
               "function declaration may only have a unique !dbg attachment",
...
        AssertDI(cast<DISubprogram>(I.second)->isDistinct(),
                 "function definition may only have a distinct !dbg attachment",
                 &F);

It should be possible to simplify that. I don't think checking isDistinct() in verifyDISubprogram is redundant though. You might have DISubprogram metadata that's not attached to a function.

Yeah, there are several layers where this is enforced, but I didn't get to the bottom of where exactly each applies. I tried to test with "free standing" DISubprogram metadata like that, but it appeared to just be dropped before the verifier would see it.

This revision was landed with ongoing or failed builds.Oct 29 2020, 8:41 AM
This revision was automatically updated to reflect the committed changes.

I made a couple small edits before committing:

  • I used --match-full-lines to simplify the patterns a bit
  • I removed the no-op test at the end