[Lldb-commits] [PATCH] D55214: Introduce ObjectFileBreakpad

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.


================
Comment at: lit/Modules/Breakpad/lit.local.cfg:1
+config.suffixes = ['.test']
----------------
This shouldn't be necessary, the top-level `lit.cfg.py` already recognizes `.test` extension.  You only need a lit.local.cfg if you're changing something.


================
Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp:20-43
+static llvm::Triple::OSType toOS(llvm::StringRef str) {
+  using llvm::Triple;
+  return llvm::StringSwitch<Triple::OSType>(str)
+      .Case("Linux", Triple::Linux)
+      .Case("mac", Triple::MacOSX)
+      .Case("windows", Triple::Win32)
+      .Default(Triple::UnknownOS);
----------------
LLVM already has these functions in `Triple.cpp`, but they are hidden as private implementations in the CPP file.  Perhaps we should expose them from headers in Triple.h.


================
Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp:56-59
+  if (to_integer(str.substr(0, 8), t, 16))
+    data.uuid1 = t;
+  else
+    return UUID();
----------------
Consider using `StringRef::consumeInteger()` here.


================
Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp:60-77
+  for (int i = 0; i < 2; ++i) {
+    if (to_integer(str.substr(8 + i * 4, 4), t, 16))
+      data.uuid2[i] = t;
+    else
+      return UUID();
+  }
+  for (int i = 0; i < 8; ++i) {
----------------
Similarly for these lines, by using `consume` functions everywhere we can get rid of a lot of the math and I think make the code easier to follow.


================
Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp:92-101
+  std::tie(token, line) = getToken(line);
+  llvm::Triple triple;
+  triple.setOS(toOS(token));
+  if (triple.getOS() == llvm::Triple::UnknownOS)
+    return llvm::None;
+
+  std::tie(token, line) = getToken(line);
----------------
Instead of having the custom parsing functions above, how about just:

```
std::tie(os, line) = getToken(line);
std::tie(arch, line) = getToken(line);
llvm::Triple triple(os, "unknown", arch);
if (triple.getArch() == Unknown || triple.getOS() == Unknown)
  return llvm::None;
```

This way we don't even need to expose the parse functions I commented on earlier, and we can just delete them.


================
Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp:160-161
+  }
+  llvm::StringRef text(reinterpret_cast<const char *>(data_sp->GetBytes()),
+                       data_sp->GetByteSize());
+
----------------
We have `GetData()` which returns an `ArrayRef`, and another function `toStringRef` which converts an `ArrayRef` to a `StringRef`.  So this might be cleaner to write as `auto text = llvm::toStringRef(data_sp->GetData());`


================
Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.h:74
+
+  bool IsStripped() override { return false; }
+
----------------
Is this always true for breakpad files?


================
Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.h:98
+
+private:
+  struct Header {
----------------
lemo wrote:
> Nit: I personally prefer not to mix data, type and function members in the same "access" section - is there an LLVM/LLDB guideline which requires everything in the same place?
> 
> If not, can you please add a private section for the destructor, followed by a section for the private data members?
Given that we don't actually store an instance of the header anywhere, we just use it as a constructor parameter, perhaps we could go one step further and move this entire type to an anonymous namespace in the cpp file, and update the constructor to take an `ArchSpec` and a `UUID`.

I prefer to avoid nested classes wherever possible since it clutters up the interface, so hiding it to the cpp file is nice.


================
Comment at: tools/lldb-test/SystemInitializerTest.cpp:120
 
+  breakpad::ObjectFileBreakpad::Initialize();
   ObjectFileELF::Initialize();
----------------
Shouldn't we also initialize this in `SystemInitializerFull`?


================
Comment at: tools/lldb-test/lldb-test.cpp:737
 
+    auto *ObjectPtr = ModulePtr->GetObjectFile();
+
----------------
I would use an explicit type spelling here, but since the function is called `GetObjectFile`, I don't feel too strongly.  It's pretty clear what the return type is.


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

https://reviews.llvm.org/D55214