[PATCH] D31439: PR32382: Emit complex DWARF expressions with the correct location description kind

Adrian Prantl via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Mar 29 11:17:48 PDT 2017


aprantl added a comment.

In https://reviews.llvm.org/D31439#713317, @dblaikie wrote:

> There are a lot of small, careful changes in here - I might need a bit of a high level walkthrough of what the changes are and how they achieve the goal. More detail on how this changes behavior would be good. "makes LLVM aware of these DWARF restrictions" is a bit too high level.


This was unfortunately the smallest I could break the patch up into.
The observable behavior changes only for DIExpressions that have a DW_OP_deref operator *and* additional operators. The primary change is that the old code treated DW_OP_breg as if it where dereferencing the contents of the register. This happened to work if no other operations followed (because with no DW_OP_stack_value present, debuggers treat the expression as a memory location description, i.e.: deref the expression).



================
Comment at: lib/Bitcode/Reader/MetadataLoader.cpp:1536-1540
+        for (I = 0; I < N - 1; ++I) {
+          Elts[I] = Elts[I + 1];
+          if (N > 3 && I == N - 4 && Elts[I + 1] == dwarf::DW_OP_LLVM_fragment)
+            break;
+        }
----------------
dblaikie wrote:
> This seems a bit complicated - any way to simplify it/make it more obvious? (or at least add comments)
> 
> Some of it looks the same as the condition in the case 0 of this switch. Could it be factored into a common function wiht a name that might aid readability?
It doesn't have enough in common to be factored out, but perhaps adding the comment
```
//[DW_OP_deref, ..., DW_OP_LLVM_fragment, x, y]
// -> [..., DW_OP_deref, DW_OP_LLVM_fragment, x, y]
```
makes it more obvious what is happening there.


================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:1759-1760
   Record.reserve(N->getElements().size() + 1);
-
-  const uint64_t HasOpFragmentFlag = 1 << 1;
-  Record.push_back((uint64_t)N->isDistinct() | HasOpFragmentFlag);
+  const uint64_t Version = 2 << 1;
+  Record.push_back((uint64_t)N->isDistinct() | Version);
   Record.append(N->elements_begin(), N->elements_end());
----------------
dblaikie wrote:
> Where did the HasOpFragmentFlag go? Or it morphed into a version flag stored above the first bit?
It's rolled into the Version. I have a laundry list of future improvements to DIExpression, that will involve revving the version and are easier to handle as incremental upgrades rather than individual "features".


================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:177
+      // to emit a memory location description.
+      Ops.push_back(dwarf::DW_OP_deref);
       const MCSymbol *Sym = Asm->getSymbol(Global);
----------------
dblaikie wrote:
> So is this another "not quite DWARF expression" contract it DIExpression? (like the LLVM_fragment?)
Yes, this is a bit obtuse. We could also add an addAddress() call to DwarfExpression analogous to addMachineReg() that then sets the location description kind inside of DwarfExpression. We could effectively move all the code inside `if (Global)` into DwarfExpression::addAddress().


https://reviews.llvm.org/D31439





More information about the llvm-commits mailing list