This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Fix PR37395.
ClosedPublic

Authored by HsiangKai on May 10 2018, 8:01 PM.

Details

Summary

Do not assume DbgInfoIntrinsic has an address as its first operand.

Fix https://bugs.llvm.org/show_bug.cgi?id=37395

Diff Detail

Repository
rL LLVM

Event Timeline

HsiangKai created this revision.May 10 2018, 8:01 PM
davide requested changes to this revision.May 10 2018, 8:33 PM
davide added inline comments.
test/DebugInfo/Generic/PR37395.ll
2

This is too fragile. The test should run a single pass.

This revision now requires changes to proceed.May 10 2018, 8:33 PM

The fix itself looks fine, but I'll defer to @aprantl.
Do you need anybody to commit this for you?

The fix itself looks fine, but I'll defer to @aprantl.
Do you need anybody to commit this for you?

@aprantl Do you have any comments?
@davide I could ask for Shiva Chen to commit this for me, thanks anyway.

test/DebugInfo/Generic/PR37395.ll
2

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?

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?

xbolva00 added inline comments.
test/DebugInfo/Generic/PR37395.ll
2

Seems like it crashed here:

  1. Running pass 'CallGraph Pass Manager' on module 'small.c'.
  2. Running pass 'Loop-Closed SSA Form Pass' on function '@f'

so try to run this pass only.

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?

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?

HsiangKai updated this revision to Diff 147728.May 20 2018, 7:13 PM

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.

HsiangKai added inline comments.May 20 2018, 7:15 PM
test/DebugInfo/Generic/PR37395.ll
2

I will try it. Thanks a lot. :)

HsiangKai updated this revision to Diff 147729.May 20 2018, 8:11 PM

Update test case.

HsiangKai updated this revision to Diff 147752.May 21 2018, 3:22 AM

Update test case.

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?

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?

Although the fix is for DILabel now, I think it is a more general fix for DbgInfoIntrinsic without address as its first operand.

HsiangKai marked 4 inline comments as done.May 21 2018, 7:00 PM

Update the test case according to comments is done.

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?

Although the fix is for DILabel now, I think it is a more general fix for DbgInfoIntrinsic without address as its first operand.

@aprantl: Do you agree my point?

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.

HsiangKai added a comment.EditedMay 31 2018, 12:28 AM

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

I got your point. I will update the patch as you mentioned. Thanks.

HsiangKai updated this revision to Diff 150258.Jun 6 2018, 11:31 PM
aprantl accepted this revision.Jun 7 2018, 11:16 AM
This revision was not accepted when it landed; it landed in state Needs Review.Jul 3 2018, 1:00 AM
Closed by commit rL336176: [DebugInfo] Fix PR37395. (authored by shiva). · Explain Why
This revision was automatically updated to reflect the committed changes.