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



_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to