Do not assume DbgInfoIntrinsic has an address as its first operand.
Details
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
| test/DebugInfo/Generic/PR37395.ll | ||
|---|---|---|
| 1 ↗ | (On Diff #146271) | This is too fragile. The test should run a single pass. |
Comment Actions
The fix itself looks fine, but I'll defer to @aprantl.
Do you need anybody to commit this for you?
Comment Actions
| test/DebugInfo/Generic/PR37395.ll | ||
|---|---|---|
| 1 ↗ | (On Diff #146271) | I use the steps provided by Zhendong Su to reproduce the bug in https://bugs.llvm.org/show_bug.cgi?id=37395. I didn't get the idea that 'run a single pass'. Could you give more information or examples to improve the test case? |
Comment Actions
Neither the PR nor this review contain any information on why removing the assertion is the correct action here. Can you explain why this is the right fix, and what the MDNode is when the assertion fires?
| test/DebugInfo/Generic/PR37395.ll | ||
|---|---|---|
| 1 ↗ | (On Diff #146271) | Seems like it crashed here:
so try to run this pass only. |
Comment Actions
getVariableLocation() assumes all DbgInfoIntrinsic has the address value as its first operand. It is not the case for DILabel.
When the assertion is triggered, MDNode is DILabel. The first operand of DILabel is a metadata to keep label information, not address value. So, the second if contition, dyn_cast<ValueAsMetadata>, will return null.
It adds an assertion to ensure the number of operand must be zero before returning nullptr. I am not sure it is reasonable to check the condition before return. Is there any case it must ensure the number of operands be zero before returning nullptr?
Comment Actions
If getVariableLocation() allows to return nullptr and the first operand
of the intrinsic function is not address value, we just return nullptr
without checking the number of operands be zero.
| test/DebugInfo/Generic/PR37395.ll | ||
|---|---|---|
| 1 ↗ | (On Diff #146271) | I will try it. Thanks a lot. :) |
Comment Actions
getVariableLocation() assumes all DbgInfoIntrinsic has the address value as its first operand. It is not the case for DILabel.
When the assertion is triggered, MDNode is DILabel. The first operand of DILabel is a metadata to keep label information, not address value. So, the second if contition, dyn_cast<ValueAsMetadata>, will return null.
It adds an assertion to ensure the number of operand must be zero before returning nullptr. I am not sure it is reasonable to check the condition before return. Is there any case it must ensure the number of operands be zero before returning nullptr?
Why aren't you checking whether this is a DILabel instead?
Comment Actions
Although the fix is for DILabel now, I think it is a more general fix for DbgInfoIntrinsic without address as its first operand.
Comment Actions
The fix is also related to broken Chromium build due to D45045.
https://bugs.chromium.org/p/chromium/issues/detail?id=841170#c1
Comment Actions
My general problem with this is that a debug info intrinsic with a null operand is usually indicative of a bug in a transformation pass that forgot to update the debug intrinsic after modifying the value it was pointing to. If the value goes away then the debug intrinsic should use an undef value (to end the range started by an earlier dbg intrinsic describing the same variable) or be removed entirely (if all instructions dealing with that variable have been removed as dead code). I don't want to make it easier to for transformation passes to not update the debug info correctly. If all you need is that DILabels are handled correctly, then this patch should just specifically enable that case.
Comment Actions
Debug info intrinsic with a null operand could not indicate a bug in transformation passes, in getVariableLocation() at least. It will return nullptr in getVariableLocation() and the caller just skip the intrinsic.
Following is the code fragment from llvm::insertDebugValuesForPHIs()
ValueToValueMapTy DbgValueMap;
for (auto &I : *BB) {
if (auto DbgII = dyn_cast<DbgInfoIntrinsic>(&I)) {
if (auto *Loc = dyn_cast_or_null<PHINode>(DbgII->getVariableLocation()))
DbgValueMap.insert({Loc, DbgII});
}
}You could see that when getVariableLocation() return nullptr, it just skips the intrinsic.
The assertion is triggered when the intrinsic has operands and the first operand is not value, not has a null operand.
Comment Actions
Do you think you could post an example were this assertion incorrectly fires for a dbg,value intrinsic? I would really like to understand this case better.
Comment Actions
I think this assertion will not be incorrectly fired for dbg.value. dbg.value always has its location address as its first operand.
In addition, the argument of getVariableLocation(), AllowNullOp, has default value as true. So, it seems to allow null operand for dbg.value.
Value *Op = getArgOperand(0); if (AllowNullOp && !Op) return nullptr;
If the first operand of dbg.value is not location address and not nullptr, the assertion will be triggered.
Comment Actions
If that is the case then you should be able to write the assertion as
assert((isa<DILabelInst>() || !cast<MDNode>(MD)->getNumOperands()) && ...);
without guarding it in if (AllowNullOp). I'd be more comfortable this way since it is more explicit in that dbg.value's may not have null arguments. My point is that a dbg.value with a null argument is indicative of a compiler bug. We need to relax the assertion in order to support dbg.label (that's fine) but while doing that we shouldn't also relax it for dbg.value. Please let me know if I'm misunderstanding something.