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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D75750#1993990 <https://reviews.llvm.org/D75750#1993990>, @jankratochvil wrote:

> In D75750#1991873 <https://reviews.llvm.org/D75750#1991873>, @labath wrote:
>
> > In D75750#1988694 <https://reviews.llvm.org/D75750#1988694>, @jankratochvil wrote:
> >
> > > The current plan discussed with @kwk is to create the new `SymbolServer` abstract superclass and some its inherited implementation and move there the appropriate parts of existing `lldb/source/Symbol/LocateSymbolFile.cpp`. Current `SymbolVendor` implementations would then iterate new `SymbolServer`s by the existing `LocateExecutableSymbolFile` function. That may be enough for a patch of its own.
> >
> >
> > I'll have to see the actual patch for a definitive opinion, but I have to say that a priori I am sceptical of this direction. And yes, that should definitely be a separate patch.
>
>
> This separate `SymbolServer` is following @clayborg's comment above <https://reviews.llvm.org/D75750#1950734>.
>  You proposed to merge `SymbolServer` with `SymbolVendor` in @labath's comment above <https://reviews.llvm.org/D75750#1952130>.
>  I found more clean the separate `SymbolServer` variant as there is orthogonal functionality of locating the files (on disk or from symbol server - `SymbolServer`) vs. extracting the unique ID from current file (extracting build-id - `SymbolVendor` functionality). So from the both proposed solutions I preferred the @clayborg's comment above <https://reviews.llvm.org/D75750#1950734>.
>  I hope there is no misunderstanding which could lead to @kwk implementing a third solution nobody wants.


I think that you two and Greg are mostly in sync, but I am yet to be convinced that this indeed the right solution. My reasons for that are two fold:

- the existing SymbolVendor implementations are very simple (and most importantly, stateless). In fact they are so simple, I was contemplating simply removing them and replacing by a couple of free functions. Surely, we don't need an entire class hierarchy to implement "extracting the unique ID from current file". Implementing symbol server functionality would give these classes a reason to exist, as there would now be an actual state that they need to maintain (a connection to the symbol server, or at least its url)
- I don't think the separation between "SymbolServer" and "SymbolVendor" will be as clear as you make it sound to be. The "searching" aspect is fairly trivial in SymbolVendorELF, and it does boil down to a `LocateExecutableSymbolFile` call. The situation is a bit fuzzier with SymbolVendorMacOSX, which also handles some path remapping aspects. But that is the job of the symbol "server" in the debuginfod world. It's not clear to me how you're going to align these two things without "vendors" and "servers" separate.

Now you can try to implement a patch and demonstrate how things will work that way and hope to convince me that way, or we can to talk about abstractly, and come up with some sort of a "design doc" for this thing. Up to you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75750