[PATCH] D68744: [GSYM] Add GsymCreator and GsymReader.

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

Mechanically, this looks mostly fine.



================
Comment at: include/llvm/DebugInfo/GSYM/GsymCreator.h:140
+  std::vector<llvm::gsym::FileEntry> Files;
+  std::vector<uint8_t> UUID;
+  bool Finalized = false;
----------------
In other places we use `uint8_t UUID[16]` for this.


================
Comment at: include/llvm/DebugInfo/GSYM/GsymCreator.h:168
+  uint32_t insertString(StringRef S) {
+    std::lock_guard<std::recursive_mutex> Guard(Mutex);
+    if (S.empty())
----------------
Just personal opinion, but anything with locks I would put into the cpp file.


================
Comment at: include/llvm/DebugInfo/GSYM/GsymCreator.h:213
+  /// \param UUIDBytes The new UUID bytes.
+  void setUUID(llvm::ArrayRef<uint8_t> UUIDBytes) {
+    UUID.assign(UUIDBytes.begin(), UUIDBytes.end());
----------------
same here


================
Comment at: include/llvm/DebugInfo/GSYM/GsymReader.h:51
+  // local storage and set point the ArrayRef objects above to these swapped
+  // copies.
+  struct SwappedData {
----------------
///


================
Comment at: include/llvm/DebugInfo/GSYM/GsymReader.h:68
+  // Accessor functions that allow iteration across all addresses in the GSYM
+  // file.
+  size_t getNumAddresses() const;
----------------
///


================
Comment at: include/llvm/DebugInfo/GSYM/GsymReader.h:71
+  Optional<uint64_t> getAddress(size_t Index) const;
+  Optional<FileEntry> getFile(uint32_t Index) const {
+    if (Index < Files.size())
----------------
why is this interface useful? Wouldn't an iterator of a ForEach function be cleaner?


================
Comment at: lib/DebugInfo/GSYM/GsymCreator.cpp:1
+//===- GsymCreator.cpp ------------------------------------------*- C++ -*-===//
+//
----------------
a `-*- C++ -*-` marker only makes sense in a .h file where the language is ambiguous.


================
Comment at: lib/DebugInfo/GSYM/GsymCreator.cpp:196
+    if (Prev != Funcs.end()) {
+      if (Prev->Range.intersects(Curr->Range)) {
+        // Overlapping address ranges.
----------------
This is confusing to read because of all the nested-ness. Would it be possible to convert this into a
```
if (error) {
 OS << warning
 continue;
}
```
form?


================
Comment at: lib/DebugInfo/GSYM/GsymReader.cpp:1
+//===- GsymReader.cpp -------------------------------------------*- C++ -*-===//
+//
----------------
same here


================
Comment at: unittests/DebugInfo/GSYM/GSYMTest.cpp:1300
+    VerifyFunctionInfo(GR, Func2Addr+FuncSize-1, Func2);
+    VerifyFunctionInfoError(GR, Func2Addr+FuncSize,
+                            "address 0x1030 not in GSYM");
----------------
clang-format


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

https://reviews.llvm.org/D68744