[PATCH] D114355: [DebugInfo] ValueMapper impl for DIArgList should respect RF_IgnoreMissingLocals
Stephen Tozer via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Nov 24 07:17:28 PST 2021
StephenTozer added inline comments.
================
Comment at: llvm/unittests/Transforms/Utils/ValueMapperTest.cpp:323
+ EXPECT_EQ(N1AV, ValueMapper(VM).mapValue(*MAV1));
+ EXPECT_EQ(MAV1, ValueMapper(VM, RF_IgnoreMissingLocals).mapValue(*MAV1));
+ EXPECT_FALSE(VM.count(MAV0));
----------------
StephenTozer wrote:
> jmorse wrote:
> > (Line I'm referring to in main comment)
> I believe the "we shouldn't reference the unmapped local" comment is in reference to the non-IgnoreMissingLocals case specifically. The return value of nullptr for the IgnoreMissingLocals case is, as far as I can tell, equivalent to returning the unmapped local - this is the only site at which we may call this on a LocalAsMetadata/DIArgList:
>
> ```
> Value *V = mapValue(Op);
> // If we aren't ignoring missing entries, assert that something happened.
> if (V)
> Op = V;
> else
> assert((Flags & RF_IgnoreMissingLocals) &&
> "Referenced value not in value map!");
> ```
>
Just to expand on this answer - returning `nullptr` if no mapping can be found is meant to be the standard for mapValue, so that the caller can decide what to do about that; the specific difficulty with DIArgList is that there may be //some// elements with a mapping and some without. In this case, this patch makes the executive decision to keep the unmapped value in the returned DIArgList, rather than replacing it with a nullptr and expecting the caller to manually check for a DIArgList with nullptr elements. It may be worth adding a comment to this patch to explain that, however!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114355/new/
https://reviews.llvm.org/D114355
More information about the llvm-commits
mailing list