Page MenuHomePhabricator

[DebugInfo] When the enum type causes ODR violation, skip ODRUniquing
Needs ReviewPublic

Authored by ychen on Wed, Oct 13, 5:19 PM.

Details

Summary

Otherwise, ODRUniquing would map some member method/variable MDNodes
to have enum type DIScope, resulting in invalid debug info and bad
DWARF.

  • Add the check in Verifier that 'scope:' operand could not be enum type.
  • Exclude enum type from ODRUniquing when ODR is detected so that the debuginfo/DWARF is well-formed. The other option is to report the error to the user considering this may imply a correctness issue (https://crbug.com/630335) however ODRUniquing doesn't do this currently.

Diff Detail

Event Timeline

ychen created this revision.Wed, Oct 13, 5:19 PM
ychen requested review of this revision.Wed, Oct 13, 5:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Oct 13, 5:19 PM
ychen edited the summary of this revision. (Show Details)Wed, Oct 13, 5:25 PM
ychen edited the summary of this revision. (Show Details)

I think this has come up before in llvm-dev threads, etc - could you link to those in the patch description for further context?

For other folks, bit of a design question: Perhaps we shouldn't special case enums here, and just say that if the ODR uniquing triggers on nodes with different tags, we don't deduplicate in that case? Clearly this is best-effort, I don't think we should go and do full structural equivalence but checking the tag seems cheap and if someone's violated the ODR by accident maybe this'd make it easier on them?

ychen added a comment.Thu, Oct 14, 4:04 PM

I think this has come up before in llvm-dev threads, etc - could you link to those in the patch description for further context?

Searched llvm-dev with "odr"/"LTO"/"uniquing" didn't find anything closely related. However, https://bugs.llvm.org/show_bug.cgi?id=24923 seems pretty relevant although it is of structure type ODR violation and this patch is for enum/non-enum type ODR violation.

I think this has come up before in llvm-dev threads, etc - could you link to those in the patch description for further context?

Searched llvm-dev with "odr"/"LTO"/"uniquing" didn't find anything closely related. However, https://bugs.llvm.org/show_bug.cgi?id=24923 seems pretty relevant although it is of structure type ODR violation and this patch is for enum/non-enum type ODR violation.

Ah, yeah, that's a generalization of the issue - might be worth fixing the general issue rather than only addressing this (more severe) subset of it. (@aprantl - thoughts?) Guess the generalization is checking both the tag type matches and the size of the type matches too?

Seems the case I was remembering was something @mtrofin came across internally at Google a couple of years ago ( Google internal: b/129559805 ) - yeah, I can't find lots of info on it other than the fact that we fixed the ODR violation by renaming an enum type, but doesn't look like we looked further into fixing (Thin)LTO to not fail in this particular way. (maybe @mtrofin remembers something more of that situation)

llvm/lib/IR/Verifier.cpp
852–858

Looks like this could be a function template rather than a macro?