[HWASAN] NFC? Remove DW_OP_LLVM_tag_offset from DIExpression::isImplicit

[OCHyams]

According to its doc-comment isImplicit is meant to return true if the expression is an implicit location description (describes an object or part of an object which has no location by computing the value from available program state).

There's a brief entry for DW_OP_LLVM_tag_offset in the LangRef and there's some info in the original commit fb9ce100d19be130d004d03088ccd4af295f3435.

From what I can tell it doesn't look like DW_OP_LLVM_tag_offset affects whether or not the location is implicit; the opcode doesn't get included in the final location description but instead is added as a attribute to the variable.

I'm not 100% confident as I'm not very familiar with DW_OP_LLVM_tag_offset. However, applying this change builds (with assertions enabled) an identical clang self hosted binary built with -O2 -g and hwasan, which adds confidence.

This was tripping an assertion (code) in the latest application of the fix to #76545, #78606, where an expression containing a DW_OP_LLVM_tag_offset is split into a fragment (i.e., describe a part of the whole variable).

[llvmbot]

@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-llvm-ir

Author: Orlando Cazalet-Hyams (OCHyams)

Changes

According to its doc-comment isImplicit is meant to return true if the expression is an implicit location description (describes an object or part of an object which has no location by computing the value from available program state).

There's a brief entry for DW_OP_LLVM_tag_offset in the LangRef and there's some info in the original commit fb9ce100d19be130d004d03088ccd4af295f3435.

From what I can tell it doesn't look like DW_OP_LLVM_tag_offset affects whether or not the location is implicit; the opcode doesn't get included in the final location description but instead is added as a attribute to the variable.

I'm not 100% confident as I'm not very familiar with DW_OP_LLVM_tag_offset. However, applying this change builds (with assertions enabled) an identical clang self hosted binary built with -O2 -g and hwasan, which adds confidence.

This was tripping an assertion (code) in the latest application of the fix to #76545, #78606, where an expression containing a DW_OP_LLVM_tag_offset is split into a fragment (i.e., describe a part of the whole variable).

---

Full diff: https://github.com/llvm/llvm-project/pull/79816.diff

1 Files Affected:

• (modified) llvm/lib/IR/DebugInfoMetadata.cpp (-1)

diff --git a/llvm/lib/IR/DebugInfoMetadata.cpp b/llvm/lib/IR/DebugInfoMetadata.cpp
index 51950fc937f0ab..28f96653d815b5 100644
--- a/llvm/lib/IR/DebugInfoMetadata.cpp
+++ b/llvm/lib/IR/DebugInfoMetadata.cpp
@@ -1512,7 +1512,6 @@ bool DIExpression::isImplicit() const {
     default:
       break;
     case dwarf::DW_OP_stack_value:
-    case dwarf::DW_OP_LLVM_tag_offset:
       return true;
     }
   }

[vitalybuka]

Would this make all expressions with DW_OP_LLVM_tag_offset non implicit?
I don't know implications of this.

[OCHyams]

Thanks for the review. I'm replying in the main comments so that GitHub doesn't swallow my reply:

Would this make all expressions with DW_OP_LLVM_tag_offset non implicit?
I don't know implications of this.

Yeah it would. I don't think the presence of a DW_OP_LLVM_tag_offset should qualify an expression as implicit (basically I am thinking the current code is a "bug").

As for the implications, looking at current behaviour at the callsites of isImplicit (there aren't many):

In combineDIExpressions in DwarfDebug.cpp (code). The function combines expressions; if isImplicit is true for both expressions it avoids applying a DW_OP_stack_value. Here you can see that an expression containing a DW_OP_LLVM_tag_offset but no DW_OP_stack_value could create unexpected behaviour.

In MLocTracker::emitLoc in InstrRefBasedImpl.cpp (code). There's an assert checking a DBG_VALUE is not both indirect (describing a memory location) and has an expression that isImplicit. Here having a DW_OP_LLVM_tag_offset but no DW_OP_stack_value would trigger the assertion, which seems like unexpected behaviour again.

In PEI::replaceFrameIndexDebugInstr in PrologEpilogInserter.cpp (code), if the DBG_VALUE is indirect (describes a memory location) and isImplicit, a DW_OP_deref_size is prepended.

Finally, the assert in DIExpression::createFragmentExpression in DebugInfoMetadata.cpp (code), which is the one that tripped after applying #78606. This assert is confirming the DIExpression is suitable for splitting into fragments. It's not suitable to split some expressions that compute a value (using DW_OP_stack_value). To quote a comment there "We can't safely split arithmetic or shift operations into multiple fragments because we can't express carry-over between fragments.". It's completely fine to split a memory location into fragments. A DIExpression in a dbg.declare that contains a DW_OP_LLVM_tag_offset and an offset, e.g. DW_OP_plus_uconst, would currently trip this assertion, because currently that would mean the expression isImplicit.

I think in all these cases it is a mistake to treat an expression containing only a DW_OP_LLVM_tag_offset opcode as "implicit (which this patch fixes).

IMO the implication of the function name isImplicit in the context of DWARF, its doc-comment, and these call sites back up this claim. But I would definitely appreciate someone checking my working here.

[jryans]

I think in all these cases it is a mistake to treat an expression containing only a DW_OP_LLVM_tag_offset opcode as "implicit (which this patch fixes).

I agree with your analysis above. DW_OP_LLVM_tag_offset does not make the expression become a value instead of location, which seems to be what isImplicit is testing. So, this change seems correct to me.

[vitalybuka]

Thanks for the explanation!

[OCHyams]

Thanks for the reviews.

(The force push was just a fix for a typo in my email address in the patch.)