This is an archive of the discontinued LLVM Phabricator instance.

[DWARFv5] Verify all-or-nothing constraint on DIFile source
ClosedPublic

Authored by scott.linder on Nov 27 2018, 8:25 AM.

Details

Summary

Update IR verifier to check the constraint that DIFile source is present on all files or no files.

This may be the wrong approach, but it prevents a nasty cantFail during DWARF emission. After writing this patch I noticed MD5 checksums (which have the same restriction in DWARF) take a different approach of dropping the attribute and warning, see https://reviews.llvm.org/D48135.

Diff Detail

Event Timeline

scott.linder created this revision.Nov 27 2018, 8:25 AM

Actually, after thinking about this I agree more with the approach of dropping or somehow warning/failing in the DWARF backend, rather than verifying at the IR level. Conceivably another DI backend which implements embedded source might not have the same restriction as DWARF, so restricting the IR doesn't seem right.

In the case of https://reviews.llvm.org/D53329 the dropping might be surprising, however, as the MD5 handling only seems to warn in the AsmParser.

scott.linder edited the summary of this revision. (Show Details)Nov 28 2018, 2:25 PM

Actually, after thinking about this I agree more with the approach of dropping or somehow warning/failing in the DWARF backend, rather than verifying at the IR level. Conceivably another DI backend which implements embedded source might not have the same restriction as DWARF, so restricting the IR doesn't seem right.

Yeah, I'd rather keep the restriction until we have a use case that merits removing it.

I'll leave the sign-off here to @aprantl or @JDevlieghere who have more familiarity/investment in the verifier.

Is this constraint per-DICompileUnit or per llvm::Module? If it is per DICompileUnit then this implementation is too strict.

lib/IR/Verifier.cpp
1028

is there a less clever and more clear way to write this?

The constraint is only per-CU. I've attempted to capture this in the latest patch, although I don't quite understand the interaction of all of the DI* metadata, so I may have still missed cases.

aprantl accepted this revision.Nov 29 2018, 1:50 PM

I'm assuming that with "per-CU" you mean a DICompileUnit, this LGTM.

This revision is now accepted and ready to land.Nov 29 2018, 1:50 PM
This revision was automatically updated to reflect the committed changes.

I posted to the commit first by accident; copying here:

I realize now that this is insufficient. The other DI* metadata which have a file: (e.g. DILocalVariable) can point to an arbitrary DIFile, and I don't confirm that it matches the source: of all other files in the same DICompileUnit.