This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Ensure DIArgList in bitcode has no null or forward metadata refs
ClosedPublic

Authored by StephenTozer on Apr 15 2021, 8:53 AM.

Details

Summary

This solves an issue seen in a Chromium build, which resulted from incorrect bitcode read/write handling for DIArgList. The specific cause of the issue is that previously, when writing DIArgList to bitcode, there was no guarantee that all of its arguments had been written to bitcode beforehand. DIArgList is strict about its arguments, and will only accept ValueAsMetadata*, so forward MD references (temporary MDNode*s) are not valid when constructing a DIArgList object. It also meant that if a ConstantAsMetadata was only referenced in a DIArgList, it would not be written at all, resulting in a null record.

This issue was further obfuscated by the fact that DIArgList was not being stringent enough when reading and writing bitcode records; although DIArgList does not support null values for its arguments, it was allowing the argument records to be null.

This has been fixed by adding a new method to the ValueEnumerator to handle DIArgList that enumerates any ConstantAsMetadata arguments and contains a number of DIArgList-specific assertions. The read/write code has also been modified to appropriately assert that its arguments are non-null.

Diff Detail

Event Timeline

StephenTozer created this revision.Apr 15 2021, 8:53 AM
StephenTozer requested review of this revision.Apr 15 2021, 8:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2021, 8:53 AM

In the previous version of the patch, the actual Constant values used by the ConstantAsMetadata arguments to DIArgList were not being enumerated until the function that used them was incorporated, which resulted in incorrect bitcode output (all constant values in the bitcode are expected to appear at the module level).

jmorse accepted this revision.Apr 21 2021, 6:31 AM
jmorse added a subscriber: jmorse.

LGTM, I enjoy the minimal extension of a test to improve coverage too.

I'm a little twitchy about whether this will affect performance in what's an important path for LTO builds: on balance, I don't think it will to any significant extent.

llvm/lib/Bitcode/Writer/ValueEnumerator.cpp
84–94

I worry about putting some extra loops and bigger return value in what seems to be a hot path (touches all Values once). On the other hand, this should be trivially inlined and DIArgLists won't be common.

This revision is now accepted and ready to land.Apr 21 2021, 6:31 AM
StephenTozer added inline comments.Apr 21 2021, 8:14 AM
llvm/lib/Bitcode/Writer/ValueEnumerator.cpp
84–94

I agree actually; I folded it this way for simplicity's sake, but we can really handle ValueAsMetadata and DIArgList as separate cases. Even with inlining, I worry that the use of SmallVector here might obstruct potential optimizations for the non-DIArgList case. It might only have a small impact, but it's easy to fix nonetheless.

Reduce possible performance issues in constant value ordering.

jmorse accepted this revision.Apr 21 2021, 8:54 AM

LGTM

This revision was landed with ongoing or failed builds.Apr 22 2021, 4:05 AM
This revision was automatically updated to reflect the committed changes.