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

[Leonard Mosescu via lldb-commits]

>
> 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.
>

I agree. Sorry for mixing the minidump UUID case with the Breakpad symbol
files UUIDs.

On Wed, Jan 23, 2019 at 1:23 PM Pavel Labath via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20190123/3c578c99/attachment.html>