[PATCH] D153080: Emit DW_LLE_base_address + DW_LLE_offset_pairs instead of DW_LLE_start_length in debug_loclists section

[Alexey Lapshin via Phabricator via llvm-commits]

avl added inline comments.


================
Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFLocationExpression.h:47
+struct DWARFLocationExpressionsFragment {
+  DWARFLocationExpressionsVector LinkedLocationExpression;
+  uint64_t BaseAddress = 0;
----------------
probably not name it "LinkedLocationExpression" as linked is dsymutil term. DebugInfo/DWARF does not name addresses linked or relocated.  maybe LinkedLocationExpression -> LocationExpressions or LinkedLocationExpression -> LocationExpressionsVector.


================
Comment at: llvm/lib/DWARFLinker/DWARFLinker.cpp:1988
+                            LinkedLocationExpressionsFrag.BaseAddress;
+        assert(LowPCVal >= 0 && "Low PC value should not be negative!");
         LinkedExpression.Range = {
----------------
JDevlieghere wrote:
> nit: Let's drop the exclamation mark: other asserts in this file either use a period or no punctuation at all. 
uint64_t could not be negative. probably:

```
uint64_t LowPCVal = CurExpression.Range->LowPC + CurLocAttr.RelocAdjustment - LinkedLocationExpressionsFrag.BaseAddress;
assert(LowPCVal <= CurExpression.Range->LowPC + CurLocAttr.RelocAdjustment && "Address overflow");
```


================
Comment at: llvm/lib/DWARFLinker/DWARFStreamer.cpp:614
     const CompileUnit &Unit,
-    const DWARFLocationExpressionsVector &LinkedLocationExpression,
+    const DWARFLocationExpressionsFragment &LinkedLocationExpressionsFrag,
     PatchLocation Patch) {
----------------
may be we can do all calculations inside DwarfStreamer::emitDwarfDebugLocListsTableFragment ? i.e. do not create new structure DWARFLocationExpressionsFragment, do not change DWARFLinker::generateUnitLocations, calculate and apply BaseAddress inside DwarfStreamer::emitDwarfDebugLocListsTableFragment ? Such approach allows to change output format not changing high level structures and makes overall patch smaller.


================
Comment at: llvm/lib/DWARFLinker/DWARFStreamer.cpp:648
+      LocListsSectionSize +=
+          MS->emitSLEB128IntValue(LocExpression.Range->HighPC);
     } else {
----------------
it looks like it should be emit_U_LEB128IntValue.
```
DW_LLE_offset_pair
This is a form of bounded location description entry that has two unsigned
LEB128 operands.
```
I think SLEB came from my original patch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153080/new/

https://reviews.llvm.org/D153080