[PATCH] D114355: [DebugInfo] ValueMapper impl for DIArgList should respect RF_IgnoreMissingLocals

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/ValueMapper.cpp:411
+        } else if ((Flags & RF_IgnoreMissingLocals) && isa<LocalAsMetadata>(VAM)) {
+            MappedArgs.push_back(VAM);
         } else {
----------------
dexonsmith wrote:
> It'd be great to have a comment here explaining why `undef` isn't correct, and what's happening instead. It's not obvious to me how identity could be valid, given that the local doesn't exist on the other side... to me it looks like this could cause a `DIArgList` from one llvm::Function to point at a local llvm::Value owned by a different function (or module). Is that possibility excluded somehow? How? (the test suggests this results in `metadata !{}`; should there be a comment explaining the mechanism for that here?)
> 
> From the commit message, the goal is to change LocalAsMetadata that's referenced from a DIArgList, but it looks like it changes behaviour for all LocalAsMetadata. Can you explain why that's okay/safe?
I might misunderstand the meaning of the RF_IgnoreMissingLocals flag, but I believe that the intention of the flag is to ensure that we don't delete any LocalAsMetadata that appear prior to their referenced local value or otherwise have no existing mapping in the ValueMap, by using an identity mapping; this is the existing behaviour of the flag for direct LocalAsMetadata, and this patch is updating the DIArgList case to match.

For a direct LocalAsMetadata, this function returns nullptr if a set of conditions hold: if `mapValue(LAM->getValue())` returns nullptr and the RF_IgnoreMissingLocals flag is set. The only caller of this function then interprets the nullptr return value as an identity mapping if the RF_IgnoreMissingLocals flag is set:

```
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!");
```

This patch updates DIArgList uses of a LocalAsMetadata to maintain an identity mapping iff the same conditions hold. Although in the direct case it is the caller that makes the decision to maintain an identity mapping, the DIArgList case would be more complicated to handle from the caller side, and the end result is the same. Given this, it was my assumption that we only set the RF_IgnoreMissingLocals flag when we know that it won't result in an invalid Value reference, e.g. when we're cloning basic blocks within a function.

> From the commit message, the goal is to change LocalAsMetadata that's referenced from a DIArgList, but it looks like it changes behaviour for all LocalAsMetadata. Can you explain why that's okay/safe?

I'm fairly sure that this should only affect LocalAsMetadata that's referenced from a DIArgList - the code for handling direct LocalAsMetadata is above (lines 382-396), all the changes of this patch are within the handler for DIArgLists, which may contain LocalAsMetadata and ConstantAsMetadata.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114355/new/

https://reviews.llvm.org/D114355