[Lldb-commits] [PATCH] D75750: WIP: [lldb] integrate debuginfod

[Frank Ch. Eigler via Phabricator via lldb-commits]

fche2 added inline comments.


================
Comment at: lldb/source/Host/common/DebugInfoD.cpp:59
+  char *cache_path = nullptr;
+  int rc = debuginfod_find_source(client, buildID.GetBytes().data(),
+                                  buildID.GetBytes().size(), path.c_str(),
----------------
jankratochvil wrote:
> labath wrote:
> > fche2 wrote:
> > > jankratochvil wrote:
> > > > Here it will contact the server even if the binary does not contain any build-id - LLDB then generates UUID as 4 bytes long one:
> > > > ```
> > > >               // Use 4 bytes of crc from the .gnu_debuglink section.
> > > >               u32le data(gnu_debuglink_crc);
> > > >               uuid = UUID::fromData(&data, sizeof(data));
> > > > ```
> > > > That is a needless performance regression.
> > > > I sure do not like making such decision on the LLDB side. Maybe libdebuginfod could rather make such optimization - IMO as Frank Eigler.
> > > Could kkleine reject uuid of length 4 in the above test, i.e. something like:
> > > 
> > >     if (!uuid.IsValid() || uuid.GetBytes().size() == sizeof(u32le)) // .gnu_debuglink crc32
> > >        continue;
> > Ideally, lldb would not use the debug link crc as a uuid (and instead store that elsewhere), but rejecting the short uuids here does not seem _that_ bad.
> We were discussing with @kwk that in fact sending anything stored in UUID as build-id may not be right. `debuginfod` wants specifically build-id, not any other identifier. Or @fche2 - does it? Would `debuginfod` for example accept some that Apple UUID for Apple dsym files? Maybe LLDB could store some identifier how was the UUID obtained.
> Would debuginfod for example accept some that Apple UUID for Apple dsym files? 

The debuginfod webapi specifies that buildids simply need to be lower case hex strings.  It will dutifully accept any such string, and correctly report 403's for unknown ones.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75750