<div dir="ltr"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">After some internal discussion, it seems that the situation with the all-zero UUIDs is as follows:<br>- 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)<br>- 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.<br>So it seems like there is nothing to do here. Maybe the UUID reading code in ProcessMinidump needs revising though.  <br></blockquote><div><br></div><div>I agree. Sorry for mixing the minidump UUID case with the Breakpad symbol files UUIDs.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jan 23, 2019 at 1:23 PM Pavel Labath via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">labath marked 5 inline comments as done.<br>
labath added a comment.<br>
<br>
After some internal discussion, it seems that the situation with the all-zero UUIDs is as follows:<br>
<br>
- 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)<br>
- 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.<br>
<br>
So it seems like there is nothing to do here. Maybe the UUID reading code in ProcessMinidump needs revising though.<br>
<br>
<br>
<br>
================<br>
Comment at: source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp:69<br>
+  llvm::StringRef chunk = str.take_front(hex_digits<T>());<br>
+  uintmax_t t;<br>
+  if (!to_integer(chunk, t, 16))<br>
----------------<br>
lemo wrote:<br>
> = 0; ?<br>
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.<br>
<br>
<br>
================<br>
Comment at: source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h:48<br>
<br>
 class ModuleRecord : public Record {<br>
 public:<br>
----------------<br>
lemo wrote:<br>
> coding-convention-wise: should these definitions use struct instead of class?<br>
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.<br>
<br>
<br>
================<br>
Comment at: source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h:59<br>
<br>
-bool operator==(const ModuleRecord &L, const ModuleRecord &R);<br>
+bool operator==(const ModuleRecord &L, const ModuleRecord &R) {<br>
+  return L.OS == R.OS && L.Arch == R.Arch && <a href="http://L.ID" rel="noreferrer" target="_blank">L.ID</a> == <a href="http://R.ID" rel="noreferrer" target="_blank">R.ID</a>;<br>
----------------<br>
lemo wrote:<br>
> const method qualifier?<br>
This is a free function, not a method. :)<br>
<br>
<br>
CHANGES SINCE LAST ACTION<br>
  <a href="https://reviews.llvm.org/D57037/new/" rel="noreferrer" target="_blank">https://reviews.llvm.org/D57037/new/</a><br>
<br>
<a href="https://reviews.llvm.org/D57037" rel="noreferrer" target="_blank">https://reviews.llvm.org/D57037</a><br>
<br>
<br>
<br>
</blockquote></div>