[PATCH] D94323: [dsymutil] Add preliminary support for DWARF 5.

[Alexey Lapshin via Phabricator via llvm-commits]

avl added inline comments.


================
Comment at: llvm/tools/dsymutil/DwarfLinkerForBinary.cpp:613
 
-/// Checks that there is a relocation against an actual debug
-/// map entry between \p StartOffset and \p NextOffset.
-///
-/// This function must be called with offsets in strictly ascending
-/// order because it never looks back at relocations it already 'went past'.
-/// \returns true and sets Info.InDebugMap if it is the case.
-bool DwarfLinkerForBinary::AddressManager::hasValidRelocationAt(
+bool DwarfLinkerForBinary::AddressManager::hasValidDebugAddrRelocationAt(
+    uint64_t Offset) {
----------------
hasValidDebugAddrRelocationAt() and hasValidDebugInfoRelocationAt() have different implementations - first uses binary search and second sequentially searches from the first till last. My previous comment was about using one implementation for both cases. But, let it would be done with D93106.


================
Comment at: llvm/tools/dsymutil/DwarfLinkerForBinary.cpp:775
+    return Offset;
+  return It->Mapping->getValue().BinaryAddress;
+}
----------------
it looks like we need to add ValidReloc.Addend here, the same as in applyValidRelocs():
```
     return It->Mapping->getValue().BinaryAddress + It->Addend;

```

Also, not for this review, but as a general thing: probably it would be better to make relocateIndexedAddr() to be more general(since it appears in AddressManager interface). i.e. make relocateIndexedAddr to be able to relocate any address, not only one from debug_addr section:

class AddressManager {
public:

uint64_t relocateAddr(const DWARFFormValue&);
}




================
Comment at: llvm/tools/dsymutil/DwarfLinkerForBinary.h:154
+    /// debug map entry between \p StartOffset and \p NextOffset.
+    bool hasValidDebugInfoRelocationAt(uint64_t StartOffset, uint64_t EndOffset,
+                                       CompileUnit::DIEInfo &Info);
----------------
The comment is about debug_addr section but the function works with debug_info section.


================
Comment at: llvm/tools/dsymutil/DwarfLinkerForBinary.h:163
+    /// \returns true and sets Info.InDebugMap if it is the case.
+    bool hasValidDebugAddrRelocationAt(uint64_t Offset);
 
----------------
>     /// This function must be called with offsets in strictly ascending order
>     /// because it never looks back at relocations it already 'went past'.
> 


above comment is old. This function could not be called with offsets in strictly ascending order. DIEs from debug_info are evaluated in  strictly ascending order, but addresses which they are referred may be in any order. The implementation of hasValidDebugAddrRelocationAt uses lower_bound search function and does not relate on order of offsets.

it looks like comments for hasValidDebugAddrRelocationAt and hasValidDebugInfoRelocationAt somehow mixed up.


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

https://reviews.llvm.org/D94323