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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lit/Modules/Breakpad/lit.local.cfg:1
+config.suffixes = ['.test']
----------------
zturner wrote:
> 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.
Yes, but then `lit/Modules/lit.local.cfg` overrides it by specifying it's own list of suffixes. I could fix that by adding `.test.` to that file, or by making that file use `+=`, but it's not clear to me whether that is better than just being explicit here.

If you have any preference, let me know.


================
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);
----------------
zturner wrote:
> 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.
I've already checked out the available functions in `llvm::Triple`, and unfortunately each of them uses a slightly different form for some of the values. For example, `getArchTypeNameForLLVMName` uses `x86-64` instead of `x86_64`, `parseArch` uses `i386` instead of `x86`, `parseOS` uses `linux` instead of `Linux`, and so on...

Since this particular encoding is specific to the breakpad format, it made sense to me to have the parsing functions live here (as opposed to adding new cases to the `Triple` functions for instance), and leave everything else working with the "canonical" forms.


================
Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp:53
+  static_assert(sizeof(data) == 20, "");
+  if (str.size() < 33 || str.size() > 40)
+    return UUID();
----------------
lemo wrote:
> these magic integer literals make it hard to follow the intent - what's special about 33, 40, 8, 16, ... ? (symbolic constants might help)
I've rewritten this to gradually chop bytes off from the start of the string, instead of always indexing into the original one. That should reduce the number of magic numbers (and hopefully reduce confusion).


================
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();
----------------
zturner wrote:
> Consider using `StringRef::consumeInteger()` here.
I don't think consumeInteger can help, as these "fields" are not delimited here in any way, so that function will happily try to parse the whole string. If you had a specific patter in mind let me know (but hopefully the new implementation won't be so bad either).


================
Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp:160-161
+  }
+  llvm::StringRef text(reinterpret_cast<const char *>(data_sp->GetBytes()),
+                       data_sp->GetByteSize());
+
----------------
zturner wrote:
> 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());`
Cool, I didn't know about that. Thanks.


================
Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.h:74
+
+  bool IsStripped() override { return false; }
+
----------------
zturner wrote:
> Is this always true for breakpad files?
Well.. the whole point of these files is to provide symbol information, so it would be weird if they were stripped. The breakpad `dump_syms` allows you to omit generating unwind information, but I don't think that's enough to call this "stripped". It is certainly possible to create a file by hand which contains just a `MODULE` directive and nothing else, but I would say that is a (non-stripped) file which describes an empty module, and not a stripped file.

In reality, this doesn't really matter, as this function is called from just one place <https://github.com/llvm-mirror/lldb/blob/master/source/Core/Module.cpp#L506>, and I don't think that will be relevant for breakpad files.


================
Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.h:98
+
+private:
+  struct Header {
----------------
zturner wrote:
> 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.
Sounds good.


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


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

https://reviews.llvm.org/D55214