[DebugInfo] Prologue inserter need to insert DW_OP_deref_size

[markus]

Summary

The PrologEpilogInserter need to insert a DW_OP_deref_size before prepending a memory location expression to an already implicit expression to avoid having the existing expression act on the memory address instead of the value behind it.

The reason for using DW_OP_deref_size and not plain DW_OP_deref is that big-endian targets need to read the right size as simply truncating a larger read would yield the wrong result (LSB bytes are not at the lower address).

[aprantl]

lib/CodeGen/PrologEpilogInserter.cpp

1184

This would make more sense as a member function of DIExpression?

[markus]

I am a bit worried about the other DW_OP_deref used in the compiler and how they behave wrt big-endian targets when loading something that is smaller than the size of an address on the target machine. I do not have any concrete example of where it goes wrong but I have not actively looked for one either.

Do we all agree that using plain DW_OP_deref is potentially problematic?

lib/CodeGen/PrologEpilogInserter.cpp

1184

Agree. Will move it.

[probinson]

test/CodeGen/X86/prologepilog_deref_size.mir

59

Not sure I follow the purpose of these conversions. DW_OP_deref_size is specified to zero-extend the fetched bytes to the size of an address. The convert operators look like they are just doing the same thing over again?

[markus]

Again (and sorry for possibly over emphasizing this) the purpose of this patch is that a DW_OP_deref needs to be inserted before a memory location description can be prepended to an already existing implicit location description as otherwise the latter expression would act on the former address and not the actual value in memory.

The reasoning behind the DW_OP_deref_size is probably best explained with an example. Consider a machine with 32 bit addresses and assume that our location description referes to a 8 bit value (so that is really the size of the value we want to load).

• If our machine is little endian then a DW_OP_deref from address A will load (from most significant to least significant byte) {A[3],A[2],A[1],A[0]} onto the stack. If we truncate this 32 bit value to 8 bits then we are left with {A[0]} which exactly corresponds to a 8 bit value at address A, so no problem here.
• On the other hand if our machine is big endian then a DW_OP_deref from address A will load (from most significant to least significant byte) {A[0],A[1],A[2],A[3]} onto the stack. If we truncate this 32 bit value to 8 bits then we are left with {A[3]} which is clearly not the 8 bit value at address A.

So for big endian we need to load the right size from the start.

test/CodeGen/X86/prologepilog_deref_size.mir

59

Yes, perhaps this example is a bit ill formed in that sense and would be better of performing sext instead of zext so that the DW_OP_convert ops actually did something useful. I will change that.

[probinson]

Thanks for the detailed explanation. The deref_size is clearly necessary in this example.
Most DWARF expressions are location expressions, not value expressions, so most deref operations are actually fetching an address. As we start doing more with salvaging values from optimizations, we might find ourselves doing more value expressions, so it's worth keeping an eye out for those. The call-site parameter stuff might be a place to think about in those terms.

@aprantl @dblaikie it bugs me that MIParser and LLParser each have their own DIExpression parser. As this patch shows, they can get out of sync. Is it worth trying to do something about that?

[aprantl]

@aprantl @dblaikie it bugs me that MIParser and LLParser each have their own DIExpression parser. As this patch shows, they can get out of sync. Is it worth trying to do something about that?

We discussed this in the patch that introduced MIParser support for DIExpressions and concluded that it would be difficult to factor this out. I'm open to suggestions though.

[markus]

Moved isImplicit to DIExpression class and updated test to do sext instead of zext.

[probinson]

Aside from one style nit, I'm okay with this if Adrian is.

lib/CodeGen/PrologEpilogInserter.cpp

1177

This is the only use of MFI? I don't see any others...
If so, don't bother with a local variable MFI.

I know it's really picky... it's a fuzzy line, at what point do chains of calls like MF.getFrameInfo().getObjectSize(FrameIdx) become too unwieldy, but I think in this case it isn't.

[markus]

Rebased and fixed style nit.

[markus]

Ping @aprantl, just a friendly reminder.

[aprantl]

@aprantl @dblaikie it bugs me that MIParser and LLParser each have their own DIExpression parser. As this patch shows, they can get out of sync. Is it worth trying to do something about that?

We discussed this in the patch that introduced MIParser support for DIExpressions and concluded that it would be difficult to factor this out. I'm open to suggestions though.

include/llvm/IR/DebugInfoMetadata.h

2514

Wouldn't it be much faster to only check the last 2 operands? If it is a stack_value, we're good, if it is a DW_OP_LLVM_fragment, we need to check the one before it.

[markus]

Rebased and updated isImplicit() to only check last elements.

[aprantl]

include/llvm/IR/DebugInfoMetadata.h

2512

Thanks! I think the function is too long for an inline definition now, so let's move it into the .cpp file.

2517

the else is redundant

Is this better? I'm not actually sure...

if (N == 0)
  return false;
switch (getElement(N-1)) {
  case dwarf::DW_OP_stack_value: return true;
  case dwarf::DW_OP_LLVM_fragment:
     return N > 1 && getElement(N-2) == dwarf::DW_OP_stack_value;
  default: return true;
}

[markus]

Moved isImplicit() to .cpp file.

[markus]

lib/IR/DebugInfoMetadata.cpp

888

Like this maybe?

[aprantl]

Great, thanks!

[aprantl]

It looks like this causes clang to crash when compiling itself.

http://green.lab.llvm.org/green/job/clang-stage2-Rthinlto/16870/
http://green.lab.llvm.org/green/view/LLDB/job/clang-3.4-debuginfo-statistics/2489/

Could you please take a look / revert?

[markus]

Has been reverted in

commit 0366e3e18142466e4dd99d3109d8facd093cedc8 (llvm.org/master, master)
Author: Hans Wennborg <[e-mail]>
Date:   Fri Apr 12 12:54:52 2019 +0000

    Revert r358268 "[DebugInfo] DW_OP_deref_size in PrologEpilogInserter."
    
    It causes clang to crash while building Chromium. See https://crbug.com/952230
    for reproducer.

Will analyze the problem and put up a new revision for review shortly.

[markus]

The Chromium compilation segfault comes down to two issues:

1. DW_OP_deref_size is supposed to take a single byte argument (and not a ULEB128). If a size argument larger than 127 was used ULEB128 does not encode the same way as data1 so the extractor in DwarfDebug::emitDebugLocEntry looses track and that results in the crash.

2. When compiling the provoking file much larger than expected objects show up in PEI::replaceFrameIndices (I was expecting that it was just primitive parameter values passed on the stack) but instead we got

(gdb) p MI.getDebugVariable()->isParameter()
$20 = true
(gdb) p MI.dump()
DBG_VALUE $rbp, $noreg, !"this", !DIExpression(DW_OP_plus_uconst, 56, DW_OP_stack_value), debug-location !20746; ../../third_party/SPIRV-Tools/src/test/unit_spirv.h:0 @[ ../../third_party/SPIRV-Tools/src/test/text_to_binary.annotation_test.cpp:64:1 ] line no:0
(gdb) p Size
$23 = 2128

This patch addresses 1) but I am a bit confused about what to do with 2) so I will need to investigate that further.

[probinson]

In D59687#1468536, @markus wrote:

The Chromium compilation segfault comes down to two issues:

1. DW_OP_deref_size is supposed to take a single byte argument (and not a ULEB128). If a size argument larger than 127 was used ULEB128 does not encode the same way as data1 so the extractor in DwarfDebug::emitDebugLocEntry looses track and that results in the crash.

DWARF says that's illegal. The deref is fetching a value to push onto the expression stack. "This operand is a 1-byte unsigned integral constant whose value may not be larger than the size of the generic type."

It sounds like you're trying to do this with aggregate types, and that won't work. Only base types (for DWARF 5) or address-like types (for DWARF <= 4) should end up on the stack.

[markus]

! In D59687#1468571, @probinson wrote:
It sounds like you're trying to do this with aggregate types, and that won't work. Only base types (for DWARF 5) or address-like types (for DWARF <= 4) should end up on the stack.

Yes, but it is not intentional.

I guess that I did not consider that we could enter this place with a aggregate type object and an expression like !DIExpression(DW_OP_plus_uconst, 56, DW_OP_stack_value) ending with a DW_OP_stack_value.
I wonder what kind of source code that corresponds to, something that takes the address 56 bytes into a aggregate? So maybe something like this.

struct S {
  char bar[1024];
}
void foo(S s) {
  char *p = &s.bar[56];
  ...
}

Having a bit of hard time minimizing the Chromium failure to something useful.

[aprantl]

In D59687#1471389, @markus wrote:

Having a bit of hard time minimizing the Chromium failure to something useful.

You may already know this, but just in case: I'm using results delta (http://delta.tigris.org) for these kind of jobs with great results. There is also creduce, but in my personal experience it is slower than multidelta and more easily tripped by unusual inputs.

[markus]

In D59687#1471912, @aprantl wrote:

You may already know this, but just in case: I'm using results delta (http://delta.tigris.org) for these kind of jobs with great results. There is also creduce, but in my personal experience it is slower than multidelta and more easily tripped by unusual inputs.

I did try but I had no luck with multidelta but anyway I realized much later that @hans had already been helpful and minimized the reproducer in https://crbug.com/952230 so I managed to do some further analysis using that.

[markus]

After studying the reproducer and reading some in https://llvm.org/docs/SourceLevelDebugging.html I realize that maybe isImplicit() is not a sufficient condition and that we also need to include the second operand of the DBG_VALUE in the test.

Cant say that I am very sure about this though but at least it does solve the reported problem..

Any feedback is welcome!

[bjope]

lib/CodeGen/PrologEpilogInserter.cpp

1194

MI.getOperand(1).isImm()

[aprantl]

lib/CodeGen/PrologEpilogInserter.cpp

1194

Perhaps MachineInstr::isIndirectDebugValue() ?

[markus]

Use MI.isIndirectDebugValue() and improved the test a bit.

[markus]

Can (or should) I proceed to push this again? I am a bit confused since the accepted status remains even though I upload new patches.

[aprantl]

Just land the patch and watch the bots :-)

[aprantl]

There's no hard and fast rules about this, but usually, tiny adjustments to fix issues found by bots can be landed without requiring a separate review.

include/llvm/IR/DebugInfoMetadata.h

public:
   /// Retrieve the details of this fragment expression.
   Optional<FragmentInfo> getFragmentInfo() const {
     return getFragmentInfo(expr_op_begin(), expr_op_end());
   }
 
   /// Return whether this is a piece of an aggregate variable.
   bool isFragment() const { return getFragmentInfo().hasValue(); }
 
+  /// Return whether this is an implicit location description.
+  bool isImplicit() const;
+
   /// Append \p Ops with operations to apply the \p Offset.
   static void appendOffset(SmallVectorImpl<uint64_t> &Ops, int64_t Offset);
 
   /// If this is a constant offset, extract it. If there is no expression,
   /// return true with an offset of zero.
   bool extractIfOffset(int64_t &Offset) const;
 
   /// Checks if the last 4 elements of the expression are DW_OP_constu <DWARF
   /// Address Space> DW_OP_swap DW_OP_xderef and extracts the <DWARF Address
   /// Space>.
   static const DIExpression *extractAddressClass(const DIExpression *Expr,
                                                  unsigned &AddrClass);

lib/CodeGen/AsmPrinter/DwarfExpression.cpp

while (ExprCursor) {
     case dwarf::DW_OP_swap:
       assert(LocationKind != Register);
       emitOp(dwarf::DW_OP_swap);
       break;
     case dwarf::DW_OP_xderef:
       assert(LocationKind != Register);
       emitOp(dwarf::DW_OP_xderef);
       break;
+    case dwarf::DW_OP_deref_size:
+      emitOp(dwarf::DW_OP_deref_size);
+      emitUnsigned(Op->getArg(0));
+      break;
     default:
       llvm_unreachable("unhandled opcode found in expression");
     }
   }
 
   if (LocationKind == Implicit)
     // Turn this into an implicit location description.
     addStackValue();

lib/CodeGen/MIRParser/MIParser.cpp

bool MIParser::parseDIExpression(MDNode *&Expr) {
   if (Token.isNot(MIToken::rparen)) {
     do {
       if (Token.is(MIToken::Identifier)) {
         if (unsigned Op = dwarf::getOperationEncoding(Token.stringValue())) {
           lex();
           Elements.push_back(Op);
           continue;
         }
+        if (unsigned Enc = dwarf::getAttributeEncoding(Token.stringValue())) {
+          lex();
+          Elements.push_back(Enc);
+          continue;
+        }
         return error(Twine("invalid DWARF op '") + Token.stringValue() + "'");
       }
 
       if (Token.isNot(MIToken::IntegerLiteral) ||
           Token.integerValue().isSigned())
         return error("expected unsigned integer");
 
       auto &U = Token.integerValue();

lib/CodeGen/PrologEpilogInserter.cpp

for (unsigned i = 0, e = MI.getNumOperands(); i != e; ++i) {
 
       // Frame indices in debug values are encoded in a target independent
       // way with simply the frame index and offset rather than any
       // target-specific addressing mode.
       if (MI.isDebugValue()) {
         assert(i == 0 && "Frame indices can only appear as the first "
                          "operand of a DBG_VALUE machine instruction");
         unsigned Reg;
+        unsigned FrameIdx = MI.getOperand(0).getIndex();
+        unsigned Size = MF.getFrameInfo().getObjectSize(FrameIdx);
+
         int64_t Offset =
-            TFI->getFrameIndexReference(MF, MI.getOperand(0).getIndex(), Reg);
+            TFI->getFrameIndexReference(MF, FrameIdx, Reg);
         MI.getOperand(0).ChangeToRegister(Reg, false /*isDef*/);
         MI.getOperand(0).setIsDebug();
-        auto *DIExpr = DIExpression::prepend(MI.getDebugExpression(),
-                                             DIExpression::NoDeref, Offset);
+
+        const DIExpression *DIExpr = MI.getDebugExpression();
+        // If we already have a Implicit location expression we need to insert
+        // a deref before prepending a Memory location expression.
+        if (DIExpr->isImplicit()) {
+          SmallVector<uint64_t, 2> Ops = {dwarf::DW_OP_deref_size, Size};
+          DIExpr = DIExpression::prependOpcodes(DIExpr, Ops, DIExpression::WithStackValue);
+        }
+        DIExpr = DIExpression::prepend(DIExpr, DIExpression::NoDeref, Offset);
         MI.getOperand(3).setMetadata(DIExpr);
         continue;
       }
 
       // TODO: This code should be commoned with the code for
       // PATCHPOINT. There's no good reason for the difference in
       // implementation other than historical accident.  The only
       // remaining difference is the unconditional use of the stack
       // pointer as the base register.
       if (MI.getOpcode() == TargetOpcode::STATEPOINT) {
         assert((!MI.isDebugValue() || i == 0) &&
                "Frame indicies can only appear as the first operand of a "

lib/IR/DebugInfoMetadata.cpp

 }
 
 unsigned DIExpression::ExprOperand::getSize() const {
   switch (getOp()) {
   case dwarf::DW_OP_LLVM_convert:
   case dwarf::DW_OP_LLVM_fragment:
     return 3;
   case dwarf::DW_OP_constu:
+  case dwarf::DW_OP_deref_size:
   case dwarf::DW_OP_plus_uconst:
     return 2;
   default:
     return 1;
   }
 }
 
 bool DIExpression::isValid() const {
for (auto I = expr_op_begin(), E = expr_op_end(); I != E; ++I) {
     case dwarf::DW_OP_mod:
     case dwarf::DW_OP_or:
     case dwarf::DW_OP_and:
     case dwarf::DW_OP_xor:
     case dwarf::DW_OP_shl:
     case dwarf::DW_OP_shr:
     case dwarf::DW_OP_shra:
     case dwarf::DW_OP_deref:
+    case dwarf::DW_OP_deref_size:
     case dwarf::DW_OP_xderef:
     case dwarf::DW_OP_lit0:
     case dwarf::DW_OP_not:
     case dwarf::DW_OP_dup:
       break;
     }
   }
   return true;
 }
 
+bool DIExpression::isImplicit() const {
+  unsigned N = getNumElements();
+  if (isValid() && N > 0) {
+    switch (getElement(N-1)) {
+      case dwarf::DW_OP_stack_value: return true;
+      case dwarf::DW_OP_LLVM_fragment:
+        return N > 1 && getElement(N-2) == dwarf::DW_OP_stack_value;
+      default: break;
+    }
+  }
+  return false;
+}
+
 Optional<DIExpression::FragmentInfo>
 DIExpression::getFragmentInfo(expr_op_iterator Start, expr_op_iterator End) {
   for (auto I = Start; I != End; ++I)
     if (I->getOp() == dwarf::DW_OP_LLVM_fragment) {
       DIExpression::FragmentInfo Info = {I->getArg(1), I->getArg(0)};
       return Info;
     }
   return None;

test/CodeGen/X86/prologepilog_deref_size.mir

+# RUN: llc -run-pass=prologepilog -o - %s | FileCheck %s
+--- |
+  ; ModuleID = 'dbg.opt.ll'
+  source_filename = "dbg.c"
+  target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+  target triple = "x86_64-unknown-linux-gnu"
+
+  ; Function Attrs: noinline nounwind uwtable
+  define dso_local zeroext i16 @foo(i16 zeroext %d0, i16 zeroext %d1, i16 zeroext %d2, i16 zeroext %d3, i16 zeroext %d4, i16 zeroext %d5, i16 zeroext %d6, i16 zeroext %d7, i16 zeroext %d8, i16 signext %x) #0 !dbg !7 {
+  entry:
+    call void @llvm.dbg.value(metadata i16 %x, metadata !32, metadata !DIExpression(DW_OP_LLVM_convert, 16, DW_ATE_signed, DW_OP_LLVM_convert, 32, DW_ATE_signed, DW_OP_stack_value)), !dbg !34
+    ret i16 %x
+  }
+
+  ; Function Attrs: nounwind readnone speculatable
+  declare void @llvm.dbg.value(metadata, metadata, metadata) #1
+
+  attributes #0 = { noinline nounwind uwtable }
+  attributes #1 = { nounwind readnone speculatable }
+
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!3, !4, !5}
+  !llvm.ident = !{!6}
+
+  !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 9.0.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, nameTableKind: None)
+  !1 = !DIFile(filename: "dbg.c", directory: "/tmp")
+  !2 = !{}
+  !3 = !{i32 2, !"Dwarf Version", i32 4}
+  !4 = !{i32 2, !"Debug Info Version", i32 3}
+  !5 = !{i32 1, !"wchar_size", i32 4}
+  !6 = !{!"clang version 9.0.0"}
+  !7 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 2, type: !8, scopeLine: 2, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+  !8 = !DISubroutineType(types: !9)
+  !9 = !{!10, !10, !10, !10, !10, !10, !10, !10, !10, !10, !10}
+  !10 = !DIDerivedType(tag: DW_TAG_typedef, name: "T", file: !1, line: 1, baseType: !11)
+  !11 = !DIBasicType(name: "signed short", size: 16, encoding: DW_ATE_signed)
+  !32 = !DILocalVariable(name: "y", scope: !7, file: !1, line: 3, type: !33)
+  !33 = !DIBasicType(name: "signed int", size: 32, encoding: DW_ATE_signed)
+  !34 = !DILocation(line: 3, column: 20, scope: !7)
+
+...
+---
+name:            foo
+tracksRegLiveness: true
+fixedStack:
+  - { id: 0, type: default, offset: 24, size: 2, alignment: 8, stack-id: 0,
+      isImmutable: true, isAliased: false, callee-saved-register: '', callee-saved-restored: true,
+      debug-info-variable: '', debug-info-expression: '', debug-info-location: '' }
+stack:           []
+constants:       []
+machineFunctionInfo: {}
+body:             |
+  bb.0.entry:
+    renamable $ax = MOV16rm %fixed-stack.0, 1, $noreg, 0, $noreg :: (load 2 from %fixed-stack.0, align 8)
+    DBG_VALUE %fixed-stack.0, 0, !32, !DIExpression(DW_OP_LLVM_convert, 16, DW_ATE_signed, DW_OP_LLVM_convert, 32, DW_ATE_signed, DW_OP_stack_value), debug-location !34
+    RET 0, $ax
+...
+# CHECK: machineFunctionInfo
+# CHECK: DIExpression(DW_OP_plus_uconst, {{[0-9]+}}, DW_OP_deref_size, 2, DW_OP_LLVM_convert, 16, DW_ATE_signed, DW_OP_LLVM_convert, 32, DW_ATE_signed, DW_OP_stack_value)