[Lldb-commits] [PATCH] D57037: BreakpadRecords: Address post-commit feedback

[Pavel Labath via Phabricator via lldb-commits]

labath marked 5 inline comments as done.
labath added a comment.

After some internal discussion, it seems that the situation with the all-zero UUIDs is as follows:

- breakpad symbol files do not attach a special meaning to a zero UUID - if a file does not have a build-id, the dump_syms tool will use a hash of the first page of the text section (or something equally silly)
- minidump files may treat the missing build-id by replacing it with zeroes depending on the tool used to produce the minidump: breakpad doesn't do that (it does the same hash as above), crashpad does.

So it seems like there is nothing to do here. Maybe the UUID reading code in ProcessMinidump needs revising though.



================
Comment at: source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp:69
+  llvm::StringRef chunk = str.take_front(hex_digits<T>());
+  uintmax_t t;
+  if (!to_integer(chunk, t, 16))
----------------
lemo wrote:
> = 0; ?
That is not necessary, as to_integer initializes it. Perhaps more importantly, not initializing this allows tools like msan and valgrind to actually detect the cases when you end up using an uninitialized value.


================
Comment at: source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h:48
 
 class ModuleRecord : public Record {
 public:
----------------
lemo wrote:
> coding-convention-wise: should these definitions use struct instead of class?
I don't think we have a strict rule about this case, but personally, when something starts using inheritance, I tend to think of it as a class.


================
Comment at: source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h:59
 
-bool operator==(const ModuleRecord &L, const ModuleRecord &R);
+bool operator==(const ModuleRecord &L, const ModuleRecord &R) {
+  return L.OS == R.OS && L.Arch == R.Arch && L.ID == R.ID;
----------------
lemo wrote:
> const method qualifier?
This is a free function, not a method. :)


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

https://reviews.llvm.org/D57037