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

[Mika Molenkamp via Phabricator via lldb-commits]

iridinite added a comment.

@kwk Thanks for the links, good to know that there is a debuginfod client in 
the LLVM project. However, doesn't lldb itself still need to integrate with 
this, like @fche2 mentioned, for end-users to be able to work with it? (Please 
correct me if I'm wrong, I am not familiar with the LLVM project structure.)

If there is any documentation or notes on how we can set this up, that would be 
much appreciated!


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

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

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

fche2 added a comment.

Closing this might have been premature.  I can't find an lldb debuginfod client 
side support.  Is that somehow behind llvm-symbolicator or something like that? 
 Is there documentation?


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

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

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk abandoned this revision.
kwk added a comment.

There already is a Debuginfod implementation in LLVM by now. Abandoning 
revision.


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

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

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk added a comment.

Looks like it  is already there: 
https://github.com/llvm/llvm-project/tree/main/llvm/include/llvm/Debuginfod


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

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

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk added a subscriber: phosek.
kwk added a comment.

@iridinite please see these:

- https://discourse.llvm.org/t/http-library-in-llvm/56317/10
- https://discourse.llvm.org/t/rfc-building-llvm-debuginfod/59011
- https://discourse.llvm.org/t/rfc-building-llvm-debuginfod/58994
- https://discourse.llvm.org/t/debuginfod-credential-helper-rfc/64092

I suggest, you contact @phosek on the status of debuginfod implementation in 
LLVM. I'd love to know where it is at, so please leave a trace ;)


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

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

[Mika Molenkamp via Phabricator via lldb-commits]

iridinite added a comment.

Hello all,

I am quite interested in integrating debuginfod with LLDB; my colleagues and I 
are currently exploring moving away from GDB to LLDB for general development 
work, since it appears to outperform GDB in many respects!

It appears there has been no activity on this patch for some time, so I was 
wondering, is this feature still something that is being investigated? Are 
there perhaps any alternatives I could look into, if debuginfod is not planned 
to be part of the LLDB mainline?

Thank you for your time!


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

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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D75750#1993990 , @jankratochvil 
wrote:

> In D75750#1991873 , @labath wrote:
>
> > In 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 
> .
>  You proposed to merge `SymbolServer` with `SymbolVendor` in @labath's 
> comment above .
>  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 
> .
>  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

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

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

In D75750#1991873 , @labath wrote:

> In 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 
.
You proposed to merge `SymbolServer` with `SymbolVendor` in @labath's comment 
above .
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 
.
I hope there is no misunderstanding which could lead to @kwk implementing a 
third solution nobody wants.


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

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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D75750#1988669 , @kwk wrote:

> In D75750#1988330 , @labath wrote:
>
> > The situation around finding symbols is messy enough already
>
>
> The way in which I integrated debuginfod for now is just to find source files 
> and not yet symbols. That being said. I don't fear the status quo so much. 
> The status quo is probably worse for symbols than it is for source files, 
> don't you think? So with *all* the CMake integration, the hosting inside 
> `lldb/include/lldb/Host/DebugInfoD.h` and your beloved test case,
>
> 
>
> I think it is fair to say that at least some work is there that can be taken 
> into LLDB. **As long** as I fix the retrieval of the module in 
> `SourceManager::File::CommonInitializer`. As suggested by @jankratochvil 
> either here or on IRC, I would like to give it a shot and try to pass down 
> the correct module to this function. I'd say, let's see if this function can 
> be passed a Module and if the changes are worth it. The whole part for 
> retrieving debug information can come when the architectural changes are done.


That all sounds reasonable, and I would not really have a big problem with 
integrating debuginfod in this way (through @clayborg might -- you will also 
want to check with him). However, I have doubts about how long will it take to 
do the architectural changes to get symbol downloading to work (or even if it 
will happen). I don't want to demean the work you have done so far, but I think 
that stuff will be much more complicated than this.

In that light, it's not clear to me whether having the ability to download the 
source files without being able to get the executable and symbol files is 
particularly useful. It does not seem very useful to me, but maybe I am missing 
something.

Do you find that useful? If not, I think the changes can just as easily sit in 
this patch instead of in the tree. This isn't touching any areas under active 
development, so its not like this functionality will rot quickly.

> But then it's a piece of cake to extend `lldb/include/lldb/Host/DebugInfoD.h` 
> with the right methods to call the debuginfod client lib.
> 
>> because one needs to understand the funky mac symbol searching mechanism, 
>> which is pretty much impossible without a mac machine.
> 
> I'm setting up my old mac to compile LLDB and I guess @jankratochvil might 
> soon also have his own Mac. This at least puts us in a position where we can 
> verify some of our changes.

That's great to hear. (though it's sad that is necessary)

> 
> 
>> After debuginfod, one will need to understand both, and have a linux machine 
>> with some debuginfod setup. The set of such people is likely to be empty of 
>> a long time...
> 
> I'm not sure if I understand you correctly but to me the *setup* is just to 
> point to a machine with *your* or a hosted server. At least for OS binaries 
> @fche2 @fche  (which is the correct one?) is making some effort to have those 
> debuginfos and source files available and setup. That is a great start for 
> most embedded systems with not much disk space to install all debug 
> information I guess. Correct my if this is a wrong anticipation. Sure, I mean 
> it will take a while before LLDB with debuginfod will make it into a 
> distribution but hey, time flies by.

I'm not worried about having symbols for all the system binaries. But just the 
effort to setup a environment with a foreign operating system and learn enough 
about it to be able to make changes to it are enough to dissuade a lot of 
potential developers (this is to your credit). After you start playing around 
with a mac, I think you'll find that a lot of things work differently there -- 
it took me about four years to understand how dsyms and debug maps work (I 
wasn't trying to learnt it all of that time, but still...).

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


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

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

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

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.


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

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

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk added a subscriber: fche.
kwk added a comment.

In D75750#1988330 , @labath wrote:

> lldb-dev is indeed a better place for the architectural discussion. However, 
> moving the discussion there does not automatically unblock this patch. "get 
> something in now and improve the architecture later" almost never works out 
> in practice. In fact I would say that adding debuginfod is a good way to 
> cement the status quo.


I get that, but hear me out...

> The situation around finding symbols is messy enough already

The way in which I integrated debuginfod for now is just to find source files 
and not yet symbols. That being said. I don't fear the status quo so much. The 
status quo is probably worse for symbols than it is for source files, don't you 
think? So with *all* the CMake integration, the hosting inside 
`lldb/include/lldb/Host/DebugInfoD.h` and your beloved test case,

testcase 


I think it is fair to say that at least some work is there that can be taken 
into LLDB. **As long** as I fix the retrieval of the module in 
`SourceManager::File::CommonInitializer`. As suggested by @jankratochvil either 
here or on IRC, I would like to give it a shot and try to pass down the correct 
module to this function. I'd say, let's see if this function can be passed a 
Module and if the changes are worth it. The whole part for retrieving debug 
information can come when the architectural changes are done. But then it's a 
piece of cake to extend `lldb/include/lldb/Host/DebugInfoD.h` with the right 
methods to call the debuginfod client lib.

> because one needs to understand the funky mac symbol searching mechanism, 
> which is pretty much impossible without a mac machine.

I'm setting up my old mac to compile LLDB and I guess @jankratochvil might soon 
also have his own Mac. This at least puts us in a position where we can verify 
some of our changes.

> After debuginfod, one will need to understand both, and have a linux machine 
> with some debuginfod setup. The set of such people is likely to be empty of a 
> long time...

I'm not sure if I understand you correctly but to me the *setup* is just to 
point to a machine with *your* or a hosted server. At least for OS binaries 
@fche2 @fche  (which is the correct one?) is making some effort to have those 
debuginfos and source files available and setup. That is a great start for most 
embedded systems with not much disk space to install all debug information I 
guess. Correct my if this is a wrong anticipation. Sure, I mean it will take a 
while before LLDB with debuginfod will make it into a distribution but hey, 
time flies by.


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

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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

lldb-dev is indeed a better place for the architectural discussion. However, 
moving the discussion there does not automatically unblock this patch. "get 
something in now and improve the architecture later" almost never works out in 
practice. In fact I would say that adding debuginfod is a good way to cement 
the status quo. The situation around finding symbols is messy enough already 
because one needs to understand the funky mac symbol searching mechanism, which 
is pretty much impossible without a mac machine. After debuginfod, one will 
need to understand both, and have a linux machine with some debuginfod setup. 
The set of such people is likely to be empty of a long time...


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

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

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk added a comment.

In D75750#1971446 , @labath wrote:

> In D75750#1967019 , @fche2 wrote:
>
> > >> So it might be good to have the SymbolVendors use one or more 
> > >> SymbolServer plug-ins.
> > > 
> > > I don't believe we have anything that would require all modules in a 
> > > given target (or whatever) to use the same symbol vendor type.  [...]
> >
> > Just for clarity, is someone proposing to undertake such a rework of that 
> > infrastructure?  It sounds like this is becoming a prerequisite for 
> > Konrad's patch, but if no one's actually doing it, that means Konrad's work 
> > is on hold indefinitely.  Is that the intent?
>
>
> Yes, I believe that is becoming a prerequisite. I believe Konrad is willing 
> to try to implement that, but I have advised him to hold on a bit until the 
> exact details are hashed out.


@labath, I'm not really keen on implementing the architectural changes that you 
mentioned because it will take ages when I do that. And the cross-platform bit 
makes me nervous as well. Initially I hoped we might be able to integrate my 
work and improve on the architecture later. Then we're not fighting on too many 
fronts at the same time?

Shall we maybe move the discussion about the architectural changes to lldb-dev 
instead of this patch? @clayborg @labath @jingham @jankratochvil ?


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

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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D75750#1967019 , @fche2 wrote:

> >> So it might be good to have the SymbolVendors use one or more SymbolServer 
> >> plug-ins.
> > 
> > I don't believe we have anything that would require all modules in a given 
> > target (or whatever) to use the same symbol vendor type.  [...]
>
> Just for clarity, is someone proposing to undertake such a rework of that 
> infrastructure?  It sounds like this is becoming a prerequisite for Konrad's 
> patch, but if no one's actually doing it, that means Konrad's work is on hold 
> indefinitely.  Is that the intent?


Yes, I believe that is becoming a prerequisite. I believe Konrad is willing to 
try to implement that, but I have advised him to hold on a bit until the exact 
details are hashed out.


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

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

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

fche2 added a comment.

>> So it might be good to have the SymbolVendors use one or more SymbolServer 
>> plug-ins.
> 
> I don't believe we have anything that would require all modules in a given 
> target (or whatever) to use the same symbol vendor type.  [...]

Just for clarity, is someone proposing to undertake such a rework of that 
infrastructure?  It sounds like this is becoming a prerequisite for Konrad's 
patch, but if no one's actually doing it, that means Konrad's work is on hold 
indefinitely.  Is that the intent?


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

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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D75750#1953924 , @clayborg wrote:

> The main issue is that the symbol vendors currently are ELF, macOS and WASM. 
> Right now we have one SymbolVendor for a triple, but I can see a SymbolVendor 
> wanting to use multiple symbol servers to get information: one for the OS 
> binaries (debuginfod or DebugSymbols.framework at Apple) and one for the 
> current application with company specific symbol servers. At Apple, they can 
> download any symbols for macOS, iOS, watchOS and tvOS OSes and applications. 
> At Facebook we can download symbols for android, linux and iOS. Linux distros 
> might have ways to download symbols for their OS stuff, which might work 
> along side debuginfod? Also windows has the ability to download symbols.
>
> So it might be good to have the SymbolVendors use one or more SymbolServer 
> plug-ins.


I don't believe we have anything that would require all modules in a given 
target (or whatever) to use the same symbol vendor type. Each module gets its 
own instance of the object, which is obtained and manipulated through the 
generic interface. It is true that our current symbol vendors key off of the 
triple (more like object file type, really), so all modules ale likely to have 
the same vendor, but nothing really requires it to be that way. The symbol 
vendors get a ModuleSP, and they can use any information there to determine 
whether they are relevant. So if we had multiple symbol vendors interested in 
say elf files, we would just ask each of them in turn whether they can handle 
this module, and the first one would "win".


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

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

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

In D75750#1954086 , @clayborg wrote:

> Another idea for the SymbolServers: be able to specify a source repository 
> (git, svn etc) and hash or revision ID. The symbol server can grab the source 
> from the repo and cache is locally for display.


When you talk about it FYI I use "build-id" to "GIT hash" mapping text file 
during build to retrieve source files later according to binary's build-id. But 
it expects you do not strip symbols from the binaries as otherwise one cannot 
rebuild the binaries later ("reproducible build" problem - dependency on 
versions of system packages being updated in the meantime). 
https://www.jankratochvil.net/t/BUILDID-git-checkout
`debuginfod` solves this better although with higher storage requirements.


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

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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Another idea for the SymbolServers: be able to specify a source repository 
(git, svn etc) and hash or revision ID. The symbol server can grab the source 
from the repo and cache is locally for display.


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

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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

The main issue is that the symbol vendors currently are ELF, macOS and WASM. 
Right now we have one SymbolVendor for a triple, but I can see a SymbolVendor 
wanting to use multiple symbol servers to get information: one for the OS 
binaries (debuginfod or DebugSymbols.framework at Apple) and one for the 
current application with company specific symbol servers. At Apple, they can 
download any symbols for macOS, iOS, watchOS and tvOS OSes and applications. At 
Facebook we can download symbols for android, linux and iOS. Linux distros 
might have ways to download symbols for their OS stuff, which might work along 
side debuginfod? Also windows has the ability to download symbols.

So it might be good to have the SymbolVendors use one or more SymbolServer 
plug-ins.


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

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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Making a plugin out of this sounds like a good idea to me, and I could 
immediately find several downstream users for it. However, it seems to me there 
is a great deal of overlap between this SymbolServer thingy and the existing 
SymbolVendor plugin (I mean, "vend" and "serve" are basically synonyms in this 
context). The main difference is that SymbolVendor is responsible for just 
finding the symbol file (in case it is not in the main executable), where as 
this new thing could also be used for finding the main executable too (as well 
as the relevant source files).

I think it would be very confusing to have both symbol "vendors" and "servers" 
and we should try hard to implement that with a single interface. The 
SymbolVendor doesn't do much nowadays (it's basically just a single function 
that tries to search in various locations -- the rest is boilerplate). If we 
add more functionality to it, it might make it seem less baroque.

That might also help the path remapping situation. Since symbol vendors sort of 
sit in between the SymbolFile and Module classes, it should be possible to 
arrange things such that they see the raw paths coming out of the symbol file, 
before they are mangled by various mappings.


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

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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D75750#1949678 , @fche2 wrote:

> In D75750#1949527 , @labath wrote:
>
> >
>
>
>
>
> > I am expecting that this feature will hook in very near to 
> > `DownloadObjectAndSymbolFile` for downloading the debug info, but it's not 
> > clear to me how would the source files fit in. Currently, debuginfod only 
> > provides an api to retrieve a single source file, so this code would have 
> > to parse all of the debug info, pry out the source files, and download them 
> > one by one -- a complicated and slow process.
>
> Yeah, as debuginfod does not support a batch type of source download, maybe 
> this particular lldb site is not an ideal fit..


We can make it ideal. debuginfod has nice stuff in it and we should adapt LLDB 
for sure! See my SymbolServer plug-in interface in my previous comments and let 
me know what you think.

> 
> 
>> (*) Though it seems very wasteful to download all files when we are going to 
>> need only a handful of them, it may not really be that way -- we're going to 
>> be downloading all of debug info anyway, and this is going to be much larger 
>> that all of source code put together.
> 
> I see your point, OTOH you only download the whole debuginfo because you 
> currently have no choice.  (Someday with debuginfod or such, you might be 
> able to offload the DWARF searches, and then you won't have to download the 
> whole thing.)  We do have the choice to download sources on demand.

We should be able to make this work lazily and not having to download all files.


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

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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D75750#1949527 , @labath wrote:

> In D75750#1948273 , @clayborg wrote:
>
> > Currently we have a solution for macOS to locate symbol files in the 
> > "lldb/source/Symbol/LocateSymbolFile.cpp" file in the 
> > Symbols::LocateExecutableSymbolFile(...) function:
> >
> >   static FileSpec Symbols::LocateExecutableSymbolFile(const ModuleSpec 
> > _spec, const FileSpecList _search_paths);
> >
> >
> > This will locate any files that are already on the system and return the 
> > symbol file. When you don't have a symbol file, we can call:
> >
> >   static bool Symbols::DownloadObjectAndSymbolFile(ModuleSpec _spec, 
> > bool force_lookup = true);
> >
> >
> > This might ping a build server and download the symbols.
> >
> > As for source file remappings, as Jim stated, on mac, each dSYM has path 
> > remappings already inside of it that are applied on the Module (not a 
> > target wide setting) itself and no modifications need to be done to the 
> > SourceManager.
> >
> > So my question is: can be use debuginfod to find the symbol file for a 
> > given build ID via Symbols::LocateExecutableSymbolFile(...), and when/if a 
> > symbol file is fetched from debuginfod, apply all path remappings to the 
> > module itself that we hand out? Then no changes would be needed in the 
> > SourceManager, we would just ask for a symbol file and get one back with 
> > all the remappings that are needed.
>
>
> I've been thinking about that a lot too. The thing that's not clear to me is, 
> does `DownloadObjectAndSymbolFile` download source files too? If so, how?


We should probably make a new SymbolServer plug-in and convert the Apple 
version to compile and be installed only for Apple. The API for this should be 
something like:

  class SymbolServer {
/// Get a cached symbol file that is already present on this machine.
/// This gets called by all LLDB during normal debugging to fetch 
/// and cached symbol files. 
virtual ModuleSP GetSymbolFile(ModuleSpec module_spec);
  
/// Download a symbol file when requested by the user. 
/// This only gets run in response to the use requesting the symbols, 
/// not as part of the normal debug work flow
virtual FileSpec DownloadSymbolFile(ModuleSpec module_spec);
  
/// New function that allows individual access to source files when 
/// they aren't available on disk.
virtual FileSpec GetSourceFile(FileSpec file, )
  };

Then debuginfod would fit right in there. The one thing that this interace 
doesn't cover is adding source remappings to modules, but it would be great if 
we can do this somehow with this new interface. Maybe 
SymbolServer::GetSymbolFile() can take a module_sp of the existing module so it 
can modify the source remappings if it has any?

> I am expecting that this feature will hook in very near to 
> `DownloadObjectAndSymbolFile` for downloading the debug info, but it's not 
> clear to me how would the source files fit in. Currently, debuginfod only 
> provides an api to retrieve a single source file, so this code would have to 
> parse all of the debug info, pry out the source files, and download them one 
> by one -- a complicated and slow process.

This would be taken care of by the SymbolServer plug-in described above. For 
the Apple version, it would download the symbol file and remap the paths as it 
already does and the SymbolServer::GetSourceFile(FileSpec file) would just 
return the FileSpec that was passed in since it already is an NFS mount path. 
For debuginfod it can grab the source file one by one.

One possibility is to apply a module level source remapping that is unique to 
the symbol file that is returned from SymbolServer::GetSymbolFile(). Maybe we 
prepend the UUID of the module to all paths in the debug info. Something like 
mapping:

  "/..." to "/ Now if debuginfod provided an api to download all source files in a single 
> request (*), that might be workable. However, in principle, I see nothing 
> wrong with being able to download the files on demand, if we have the option 
> to do that. (debuginfod's long term goal seems to be to provide an api to 
> download the debug info in chunks too -- that would be very interesting, 
> though I also expect it to be very complicated.)

Agreed, lazy is good. I don't see the need to download sources in chunks though.

So using SymbolServer with unique path mappings ("/..." to "/ (*) Though it seems very wasteful to download all files when we are going to 
> need only a handful of them, it may not really be that way -- we're going to 
> be downloading all of debug info anyway, and this is going to be much larger 
> that all of source code put together.

I think we should be able to figure out how to make this lazy with a new 
plug-in interface


Repository:
  rG LLVM Github Monorepo

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

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

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

fche2 added a comment.

In D75750#1949527 , @labath wrote:

>




> I am expecting that this feature will hook in very near to 
> `DownloadObjectAndSymbolFile` for downloading the debug info, but it's not 
> clear to me how would the source files fit in. Currently, debuginfod only 
> provides an api to retrieve a single source file, so this code would have to 
> parse all of the debug info, pry out the source files, and download them one 
> by one -- a complicated and slow process.

Yeah, as debuginfod does not support a batch type of source download, maybe 
this particular lldb site is not an ideal fit..

> (*) Though it seems very wasteful to download all files when we are going to 
> need only a handful of them, it may not really be that way -- we're going to 
> be downloading all of debug info anyway, and this is going to be much larger 
> that all of source code put together.

I see your point, OTOH you only download the whole debuginfo because you 
currently have no choice.  (Someday with debuginfod or such, you might be able 
to offload the DWARF searches, and then you won't have to download the whole 
thing.)  We do have the choice to download sources on demand.


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

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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D75750#1948273 , @clayborg wrote:

> Currently we have a solution for macOS to locate symbol files in the 
> "lldb/source/Symbol/LocateSymbolFile.cpp" file in the 
> Symbols::LocateExecutableSymbolFile(...) function:
>
>   static FileSpec Symbols::LocateExecutableSymbolFile(const ModuleSpec 
> _spec, const FileSpecList _search_paths);
>
>
> This will locate any files that are already on the system and return the 
> symbol file. When you don't have a symbol file, we can call:
>
>   static bool Symbols::DownloadObjectAndSymbolFile(ModuleSpec _spec, 
> bool force_lookup = true);
>
>
> This might ping a build server and download the symbols.
>
> As for source file remappings, as Jim stated, on mac, each dSYM has path 
> remappings already inside of it that are applied on the Module (not a target 
> wide setting) itself and no modifications need to be done to the 
> SourceManager.
>
> So my question is: can be use debuginfod to find the symbol file for a given 
> build ID via Symbols::LocateExecutableSymbolFile(...), and when/if a symbol 
> file is fetched from debuginfod, apply all path remappings to the module 
> itself that we hand out? Then no changes would be needed in the 
> SourceManager, we would just ask for a symbol file and get one back with all 
> the remappings that are needed.


I've been thinking about that a lot too. The thing that's not clear to me is, 
does `DownloadObjectAndSymbolFile` download source files too? If so, how?

I am expecting that this feature will hook in very near to 
`DownloadObjectAndSymbolFile` for downloading the debug info, but it's not 
clear to me how would the source files fit in. Currently, debuginfod only 
provides an api to retrieve a single source file, so this code would have to 
parse all of the debug info, pry out the source files, and download them one by 
one -- a complicated and slow process.

Now if debuginfod provided an api to download all source files in a single 
request (*), that might be workable. However, in principle, I see nothing wrong 
with being able to download the files on demand, if we have the option to do 
that. (debuginfod's long term goal seems to be to provide an api to download 
the debug info in chunks too -- that would be very interesting, though I also 
expect it to be very complicated.)

(*) Though it seems very wasteful to download all files when we are going to 
need only a handful of them, it may not really be that way -- we're going to be 
downloading all of debug info anyway, and this is going to be much larger that 
all of source code put together.


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

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

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk updated this revision to Diff 253531.
kwk added a comment.

- Use file:// and require debuginfod 0.179
- simplify FindDebuginfod.cmake


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75750

Files:
  lldb/cmake/modules/FindDebuginfod.cmake
  lldb/cmake/modules/LLDBConfig.cmake
  lldb/include/lldb/Host/Config.h.cmake
  lldb/include/lldb/Host/DebugInfoD.h
  lldb/source/Core/SourceManager.cpp
  lldb/source/Host/CMakeLists.txt
  lldb/source/Host/common/DebugInfoD.cpp
  lldb/test/CMakeLists.txt
  lldb/test/Shell/SymbolFile/DWARF/source-list.cpp
  lldb/test/Shell/lit.cfg.py
  lldb/test/Shell/lit.site.cfg.py.in

Index: lldb/test/Shell/lit.site.cfg.py.in
===
--- lldb/test/Shell/lit.site.cfg.py.in
+++ lldb/test/Shell/lit.site.cfg.py.in
@@ -16,6 +16,7 @@
 config.python_executable = "@PYTHON_EXECUTABLE@"
 config.have_zlib = @LLVM_ENABLE_ZLIB@
 config.lldb_enable_lzma = @LLDB_ENABLE_LZMA@
+config.lldb_enable_debuginfod = @LLDB_ENABLE_DEBUGINFOD@
 config.host_triple = "@LLVM_HOST_TRIPLE@"
 config.lldb_bitness = 64 if @LLDB_IS_64_BITS@ else 32
 config.lldb_enable_python = @LLDB_ENABLE_PYTHON@
Index: lldb/test/Shell/lit.cfg.py
===
--- lldb/test/Shell/lit.cfg.py
+++ lldb/test/Shell/lit.cfg.py
@@ -117,6 +117,9 @@
 if config.lldb_enable_lzma:
 config.available_features.add('lzma')
 
+if config.lldb_enable_debuginfod:
+config.available_features.add('debuginfod')
+
 if find_executable('xz') != None:
 config.available_features.add('xz')
 
Index: lldb/test/Shell/SymbolFile/DWARF/source-list.cpp
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/source-list.cpp
@@ -0,0 +1,114 @@
+// clang-format off
+// REQUIRES: debuginfod
+// UNSUPPORTED: darwin, windows
+
+//  Test that we can display the source of functions using debuginfod when the
+//  original source file is no longer present.
+//  
+//  The debuginfod client requires a buildid in the binary, so we compile one in.
+//  We can create the directory structure on disc that the client expects on a
+//  webserver that serves source files. We then set DEBUGINFOD_URLS to the mock
+//  directory using file://. This avoids the need for a
+//  debuginfod server to be run. 
+//  
+//  Go here to find more about debuginfod:
+//  https://sourceware.org/elfutils/Debuginfod.html
+
+
+//We copy this file because we want to compile and later move it away
+
+// RUN: mkdir -p %t.buildroot
+// RUN: cp %s %t.buildroot/test.cpp
+
+//We use the prefix map in order to overwrite all DW_AT_decl_file paths
+//in the DWARF. We cd into the directory before compiling to get
+//DW_AT_comp_dir pickup %t.buildroot as well so it will be replaced by
+///my/new/path.
+
+// RUN: cd %t.buildroot
+// RUN: %clang_host \
+// RUN:   -g \
+// RUN:   -Wl,--build-id="0xaabbccdd" \
+// RUN:   -fdebug-prefix-map=%t.buildroot=/my/new/path \
+// RUN:   -o %t \
+// RUN:   %t.buildroot/test.cpp
+
+
+//We move the original source file to a directory that looks like a debuginfod
+//URL part.
+
+// RUN: rm -rf %t.mock
+// RUN: mkdir -p   %t.mock/buildid/aabbccdd/source/my/new/path
+// RUN: mv%t.buildroot/test.cpp %t.mock/buildid/aabbccdd/source/my/new/path
+
+
+//Adjust where debuginfod stores cache files:
+
+// RUN: rm -rf %t.debuginfod_cache_path
+// RUN: mkdir -p %t.debuginfod_cache_path
+// RUN: export DEBUGINFOD_CACHE_PATH=%t.debuginfod_cache_path
+
+
+//-- TEST 1 --  No debuginfod awareness 
+
+
+// RUN: DEBUGINFOD_URLS="" \
+// RUN: %lldb -f %t -o 'source list -n main' | FileCheck --dump-input=fail %s --check-prefix=TEST-1
+
+// TEST-1: (lldb) source list -n main
+// TEST-1: File: /my/new/path/test.cpp
+// TEST-1-EMPTY:
+
+
+//-- TEST 2 -- debuginfod URL pointing in wrong place --
+
+
+// RUN: DEBUGINFOD_URLS="http://example.com/debuginfod; \
+// RUN: %lldb -f %t -o 'source list -n main' | FileCheck --dump-input=fail %s --check-prefix=TEST-2
+
+// TEST-2: (lldb) source list -n main
+// TEST-2: File: /my/new/path/test.cpp
+// TEST-2-EMPTY:
+
+
+//-- TEST 3 -- debuginfod URL pointing corectly 
+
+
+// RUN: DEBUGINFOD_URLS="file://%t.mock/" \
+// RUN: %lldb -f %t -o 'source list -n main' | FileCheck --dump-input=fail %s --check-prefix=TEST-3
+
+// TEST-3: (lldb) source list -n main
+// TEST-3: File: /my/new/path/test.cpp
+// TEST-3: {{[0-9]+}}
+// TEST-3-NEXT:{{[0-9]+}}   // Some context lines before
+// TEST-3-NEXT:{{[0-9]+}}   // the function.
+// TEST-3-NEXT:{{[0-9]+}}
+// TEST-3-NEXT:{{[0-9]+}}
+// TEST-3-NEXT:{{[0-9]+}}   int main(int argc, char **argv) {
+// 

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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Currently we have a solution for macOS to locate symbol files in the 
"lldb/source/Symbol/LocateSymbolFile.cpp" file in the 
Symbols::LocateExecutableSymbolFile(...) function:

  static FileSpec Symbols::LocateExecutableSymbolFile(const ModuleSpec 
_spec, const FileSpecList _search_paths);

This will locate any files that are already on the system and return the symbol 
file. When you don't have a symbol file, we can call:

  static bool Symbols::DownloadObjectAndSymbolFile(ModuleSpec _spec, 
bool force_lookup = true);

This might ping a build server and download the symbols.

As for source file remappings, as Jim stated, on mac, each dSYM has path 
remappings already inside of it that are applied on the Module (not a target 
wide setting) itself and no modifications need to be done to the SourceManager.

So my question is: can be use debuginfod to find the symbol file for a given 
build ID via Symbols::LocateExecutableSymbolFile(...), and when/if a symbol 
file is fetched from debuginfod, apply all path remappings to the module itself 
that we hand out? Then no changes would be needed in the SourceManager, we 
would just ask for a symbol file and get one back with all the remappings that 
are needed.


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

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

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk marked 3 inline comments as done.
kwk added a comment.

@labath I made a signficant simplification of starting and killing the server. 
I hope you like that better.




Comment at: lldb/source/Core/SourceManager.cpp:408
+  if ((!file_spec.GetDirectory() && file_spec.GetFilename()) ||
+  !FileSystem::Instance().Exists(m_file_spec)) {
 // If this is just a file name, lets see if we can find it in the

labath wrote:
> jankratochvil wrote:
> > I do not like this extra line as it changes behavior of LLDB unrelated to 
> > `debuginfod`. IIUC if the source file with fully specified 
> > directory+filename in DWARF does not exist but the same filename exists in 
> > a different directory of the sourcetree LLDB will now quietly use the 
> > different file. That's a bug.
> > I think it is there as you needed to initialize `sc.module_sp`.
> Yes, that does not sound right. It may be good to break this function into 
> smaller pieces so you can invoke the thing you need when you need it.
My intention wasn't to leave this as is to be honest. I had comments in here 
that I removed upon request but they existed to remind myself that I haven't 
double checked the logic well enough. I just wanted access to the symbol 
context further down below and thought, that I can take it from up here.



Comment at: lldb/source/Host/common/DebugInfoD.cpp:50
+   "invalid build ID: %s",
+   buildID.GetAsString("").c_str());
+

labath wrote:
> kwk wrote:
> > jankratochvil wrote:
> > > It should not be an error:
> > > ```
> > > echo 'int main(void) { return 0; }' >/tmp/main2.c;gcc -o /tmp/main2 
> > > /tmp/main2.c -Wall -g -Wl,--build-id=none;rm 
> > > /tmp/main2.c;DEBUGINFOD_URLS=http://localhost:8002/ ./bin/lldb /tmp/main2 
> > > -o 'l main' -o q
> > > (lldb) target create "/tmp/main2"
> > > Current executable set to '/tmp/main2' (x86_64).
> > > (lldb) l main
> > > warning: (x86_64) /tmp/main2 An error occurred while finding the source 
> > > file /tmp/main2.c using debuginfod for build ID A9C3D738: invalid build 
> > > ID: A9C3D738
> > > File: /tmp/main2.c
> > > (lldb) q
> > > ```
> > > 
> > Okay, I'll have it return just an empty string. And adjust the comment on 
> > the empty string in findSource documentation. I fully understand that an 
> > error is undesirable in your test case. My question is if the caller should 
> > sanitize it's parameters passed to `findSource` of if the latter should 
> > silently ignore those wrong UUIDs. For now I silently ignore them and treat 
> > a wrong build ID like a not found (e.g. empty string is returned).
> It would be nice to make a test case out of that.
I agree, a test would be nice but not at this stage, where the whole patch 
seems to be at danger.



Comment at: lldb/test/Shell/SymbolFile/DWARF/source-list.cpp:57
+// RUN: timeout 5 python3 -u -m http.server 0 --directory %t.mock --bind 
"localhost" &> %t.server.log & export PID=$!
+// RUN: trap 'kill $PID;' EXIT INT
+

@labath My bad. I interpreted `timeout 5` wrongly. It will kill the python 
server after `5 seconds` no matter what. If we increase this time to `timeout 
5m` it will kill the server after 5 minutes and we don't need the bash trap. 
Does that sound better? At least the only ugly part would be done this way.  
The whole section would look like this:

```lang=yaml
// RUN: rm -f "%t.server.log"
// RUN: timeout 5m python3 -u -m http.server 0 --directory %t.mock --bind 
"localhost" &> %t.server.log &
```


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

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

[Jim Ingham via Phabricator via lldb-commits]

jingham added a reviewer: clayborg.
jingham added a comment.

Greg originally designed the macOS equivalent of this, so I've added him.

I totally agree that you should only do wide searches for source files when 
there's no way to get a narrower context.  Even "source list" could use the 
current thread & frame as a start context, and only fall back to a full search 
when that fails.  In a big project with many shlibs, you might have multiple 
files of the same name, but if someone specifies foo.cpp while stopped in a 
method of libMyLib.dylib, they probably do mean the one in libMyLib.dylib, if 
it exists...

I'm not terribly happy with the way the module-level source-file remapping 
interacts with debug info.  It overrides the source-map without a way to undo 
that.  The dSYM's that Apple uses internally have module-level source remapping 
in them that point to some NFS mounted directory.  We've had problems where 
somebody has copied over one of these dSYM's and the associated sources, but 
doesn't want to remote mount the directories that host the sources.  The only 
way to point lldb at those is to either copy them over to a directory structure 
that matches the remote path, or edit the dSYM to remove the source  remapping. 
 That's pretty annoying.

On the other hand, I don't think we should show the build locations (or at 
least not as a primary thing) for source files that have come from debuginfod 
or from another module level remapping.  That's confusing to anyone who wants 
to open the file in some other way (for instance if you wanted to hand the file 
off to an external editor.)  If we a way to get at both pieces of information, 
the `source info` command could be used to show the "debug info path" and the 
"local path" for a given source file.  We might want some API (on SBFileSpec?) 
to get the original path - that would actually be useful when trying to figure 
out what you should use for a target.source-map.  But if we have a local copy 
of the file you need to be able to get to that path.

The test does seem like it would be much better as a Python test.


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

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

[Pavel Labath via Phabricator via lldb-commits]

labath added a reviewer: jingham.
labath requested changes to this revision.
labath added a subscriber: jingham.
labath added a comment.
This revision now requires changes to proceed.

Adding @jingham. Jim, what do you make of this patch and the feature overall?

I know I said this looks "mostly good", but thinking about this further (and 
reading Jan's comments), I do find that there are still couple of things that 
trouble me here. :/

The first is the module_sp searching logic. I think that was previously here 
mainly to support the case when one enters `source list 
I_am_too_lazy_to_enter_the_full_path.cc`, and it would not normally fire when 
displaying the context after the process stops. But this makes a full-fledged 
feature out of it, as it will run every time we look up a file (if debuginfod 
is enabled, etc.). It seems fine to do this for the "source list" command 
(though it also may be nice to give the user an option to override this logic, 
just like he can specify a full path if he wants to), but doing it for 
stop-context purposes seems wrong, as there we should already have right module 
somewhere up the stack.

The second is the interaction between this and the `target.source-map` setting. 
For searching the file on the local filesystem, we want to use the remapped 
path, but in case of debuginfod, we would want to use the original path 
(ideally the one which doesn't even have the per-module mappings 

 applied). The two of these things make me wonder if this new code is plugged 
in at the right level.

The last one is the test case. I've already said why I don't think this is a 
good test. Now I'll just add one more reason. With python it would be easy to 
create a function which handles the details of starting a fake debug info 
server. With lit, each new test for this (there are going to be more that one, 
I hope) will have to copy the `// RUN:` goo needed to start the server in a 
separate process. Sure, maybe you could do something similar here too and move 
that logic into a shell script, but then this will look even less like a 
"normal" lit test: a RUN line, which invokes a shell script, which invokes 
python in a background process... -- it would be much simpler (and portable) if 
it was python all the way.




Comment at: lldb/source/Core/SourceManager.cpp:408
+  if ((!file_spec.GetDirectory() && file_spec.GetFilename()) ||
+  !FileSystem::Instance().Exists(m_file_spec)) {
 // If this is just a file name, lets see if we can find it in the

jankratochvil wrote:
> I do not like this extra line as it changes behavior of LLDB unrelated to 
> `debuginfod`. IIUC if the source file with fully specified directory+filename 
> in DWARF does not exist but the same filename exists in a different directory 
> of the sourcetree LLDB will now quietly use the different file. That's a bug.
> I think it is there as you needed to initialize `sc.module_sp`.
Yes, that does not sound right. It may be good to break this function into 
smaller pieces so you can invoke the thing you need when you need it.



Comment at: lldb/source/Host/common/DebugInfoD.cpp:50
+   "invalid build ID: %s",
+   buildID.GetAsString("").c_str());
+

kwk wrote:
> jankratochvil wrote:
> > It should not be an error:
> > ```
> > echo 'int main(void) { return 0; }' >/tmp/main2.c;gcc -o /tmp/main2 
> > /tmp/main2.c -Wall -g -Wl,--build-id=none;rm 
> > /tmp/main2.c;DEBUGINFOD_URLS=http://localhost:8002/ ./bin/lldb /tmp/main2 
> > -o 'l main' -o q
> > (lldb) target create "/tmp/main2"
> > Current executable set to '/tmp/main2' (x86_64).
> > (lldb) l main
> > warning: (x86_64) /tmp/main2 An error occurred while finding the source 
> > file /tmp/main2.c using debuginfod for build ID A9C3D738: invalid build ID: 
> > A9C3D738
> > File: /tmp/main2.c
> > (lldb) q
> > ```
> > 
> Okay, I'll have it return just an empty string. And adjust the comment on the 
> empty string in findSource documentation. I fully understand that an error is 
> undesirable in your test case. My question is if the caller should sanitize 
> it's parameters passed to `findSource` of if the latter should silently 
> ignore those wrong UUIDs. For now I silently ignore them and treat a wrong 
> build ID like a not found (e.g. empty string is returned).
It would be nice to make a test case out of that.


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

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

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk updated this revision to Diff 252510.
kwk marked 10 inline comments as done.
kwk added a comment.

- Add newline to end of FindDebuginfod.cmake
- Describe empty string returned from debuginfod::findSource()
- Don't treat build IDs of len <= 8 as an error but simply as not found
- move inexpensive debuginfod::isAvailable() check to beginning of if-stmt
- Simplify line number check in test file to avoid adjusting the line number 
every time the test changes
- Add newline to source-list.cpp test file


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75750

Files:
  lldb/cmake/modules/FindDebuginfod.cmake
  lldb/cmake/modules/LLDBConfig.cmake
  lldb/include/lldb/Host/Config.h.cmake
  lldb/include/lldb/Host/DebugInfoD.h
  lldb/source/Core/SourceManager.cpp
  lldb/source/Host/CMakeLists.txt
  lldb/source/Host/common/DebugInfoD.cpp
  lldb/test/CMakeLists.txt
  lldb/test/Shell/SymbolFile/DWARF/source-list.cpp
  lldb/test/Shell/lit.cfg.py
  lldb/test/Shell/lit.site.cfg.py.in

Index: lldb/test/Shell/lit.site.cfg.py.in
===
--- lldb/test/Shell/lit.site.cfg.py.in
+++ lldb/test/Shell/lit.site.cfg.py.in
@@ -16,6 +16,7 @@
 config.python_executable = "@PYTHON_EXECUTABLE@"
 config.have_zlib = @LLVM_ENABLE_ZLIB@
 config.lldb_enable_lzma = @LLDB_ENABLE_LZMA@
+config.lldb_enable_debuginfod = @LLDB_ENABLE_DEBUGINFOD@
 config.host_triple = "@LLVM_HOST_TRIPLE@"
 config.lldb_bitness = 64 if @LLDB_IS_64_BITS@ else 32
 config.lldb_enable_python = @LLDB_ENABLE_PYTHON@
Index: lldb/test/Shell/lit.cfg.py
===
--- lldb/test/Shell/lit.cfg.py
+++ lldb/test/Shell/lit.cfg.py
@@ -117,6 +117,9 @@
 if config.lldb_enable_lzma:
 config.available_features.add('lzma')
 
+if config.lldb_enable_debuginfod:
+config.available_features.add('debuginfod')
+
 if find_executable('xz') != None:
 config.available_features.add('xz')
 
Index: lldb/test/Shell/SymbolFile/DWARF/source-list.cpp
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/source-list.cpp
@@ -0,0 +1,135 @@
+// clang-format off
+// REQUIRES: debuginfod
+// UNSUPPORTED: darwin, windows
+
+//  Test that we can display the source of functions using debuginfod when the
+//  original source file is no longer present.
+//  
+//  The debuginfod client requires a buildid in the binary, so we compile one in.
+//  We can create the directory structure on disc that the client expects on a
+//  webserver that serves source files. Then we fire up a python based http
+//  server in the root of that mock directory, set the DEBUGINFOD_URLS
+//  environment variable and let LLDB do the rest. 
+//  
+//  Go here to find more about debuginfod:
+//  https://sourceware.org/elfutils/Debuginfod.html
+
+
+//We copy this file because we want to compile and later move it away
+
+// RUN: mkdir -p %t.buildroot
+// RUN: cp %s %t.buildroot/test.cpp
+
+//We use the prefix map in order to overwrite all DW_AT_decl_file paths
+//in the DWARF. We cd into the directory before compiling to get
+//DW_AT_comp_dir pickup %t.buildroot as well so it will be replaced by
+///my/new/path.
+
+// RUN: cd %t.buildroot
+// RUN: %clang_host \
+// RUN:   -g \
+// RUN:   -Wl,--build-id="0xaabbccdd" \
+// RUN:   -fdebug-prefix-map=%t.buildroot=/my/new/path \
+// RUN:   -o %t \
+// RUN:   %t.buildroot/test.cpp
+
+
+//We move the original source file to a directory that looks like a debuginfod
+//URL part.
+
+// RUN: rm -rf %t.mock
+// RUN: mkdir -p   %t.mock/buildid/aabbccdd/source/my/new/path
+// RUN: mv%t.buildroot/test.cpp %t.mock/buildid/aabbccdd/source/my/new/path
+
+
+//Adjust where debuginfod stores cache files:
+
+// RUN: rm -rf %t.debuginfod_cache_path
+// RUN: mkdir -p %t.debuginfod_cache_path
+// RUN: export DEBUGINFOD_CACHE_PATH=%t.debuginfod_cache_path
+
+
+//Start HTTP file server on port picked by OS and wait until it is ready
+//The server will be closed on exit of the test.
+
+// RUN: rm -f "%t.server.log"
+// RUN: timeout 5 python3 -u -m http.server 0 --directory %t.mock --bind "localhost" &> %t.server.log & export PID=$!
+// RUN: trap 'kill $PID;' EXIT INT
+
+
+//Extract HTTP address from the first line of the server log
+//(e.g. "Serving HTTP on 127.0.0.1 port 40587 (http://127.0.0.1:40587/) ..")
+
+// RUN: echo -n "Waiting for server to be ready"
+// RUN: SERVER_ADDRESS=""
+// RUN: while [ -z "$SERVER_ADDRESS" ]; do \
+// RUN: echo -n "."; \
+// RUN: sleep 0.01; \
+// RUN: SERVER_ADDRESS=$(head -n1 %t.server.log | grep "http://.\+/\+; -o); \
+// RUN: done
+// RUN: echo "DONE"
+
+
+//-- TEST 1 --  No debuginfod awareness 
+
+
+// RUN: 

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

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk added a comment.

@jankratochvil thanks for this thorough review. I have to think about one 
comment more precisely but the rest was fixed.




Comment at: lldb/source/Host/common/DebugInfoD.cpp:50
+   "invalid build ID: %s",
+   buildID.GetAsString("").c_str());
+

jankratochvil wrote:
> It should not be an error:
> ```
> echo 'int main(void) { return 0; }' >/tmp/main2.c;gcc -o /tmp/main2 
> /tmp/main2.c -Wall -g -Wl,--build-id=none;rm 
> /tmp/main2.c;DEBUGINFOD_URLS=http://localhost:8002/ ./bin/lldb /tmp/main2 -o 
> 'l main' -o q
> (lldb) target create "/tmp/main2"
> Current executable set to '/tmp/main2' (x86_64).
> (lldb) l main
> warning: (x86_64) /tmp/main2 An error occurred while finding the source file 
> /tmp/main2.c using debuginfod for build ID A9C3D738: invalid build ID: 
> A9C3D738
> File: /tmp/main2.c
> (lldb) q
> ```
> 
Okay, I'll have it return just an empty string. And adjust the comment on the 
empty string in findSource documentation. I fully understand that an error is 
undesirable in your test case. My question is if the caller should sanitize 
it's parameters passed to `findSource` of if the latter should silently ignore 
those wrong UUIDs. For now I silently ignore them and treat a wrong build ID 
like a not found (e.g. empty string is returned).



Comment at: lldb/test/Shell/SymbolFile/DWARF/source-list.cpp:103
+// TEST-3: File: /my/new/path/test.cpp
+// TEST-3: 123
+// TEST-3-NEXT:{{[0-9]+}}   // Some context lines before

jankratochvil wrote:
> `s/123/{{[0-9]+}}/?`
Both are fine, but I'll go with your's if that helps. If you can tell me how to 
get a lit `CHECK` statement that checks for incremental numbers, that'll be 
awesome ;)


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

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

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added inline comments.



Comment at: lldb/source/Core/SourceManager.cpp:415
 target->GetImages().ResolveSymbolContextForFilePath(
 file_spec.GetFilename().AsCString(), 0, check_inlines,
 SymbolContextItem(eSymbolContextModule |

This code could be more efficient than my previously proposed 
`GetImages.ForEach()` as it should be able to find the only one `Module` having 
that source file.
But there should be passed the full pathname incl. directories to prevent 
wrongly chosen accidentally filename-matching source files:
```
FileSystem::Instance().Exists(m_file_spec) ? 
file_spec.GetFilename().AsCString() : file_spec.GetCString(false/*denormalize*/)
```
And the `Exists()` check should be cached in this whole function as it is 
expensive.




Comment at: lldb/source/Core/SourceManager.cpp:460
+  if (!FileSystem::Instance().Exists(m_file_spec) &&
+  debuginfod::isAvailable() && sc.module_sp) {
+llvm::Expected cache_path = debuginfod::findSource(

jankratochvil wrote:
> Make the `debuginfod::isAvailable()` check first as it is zero-cost, 
> `FileSystem::Instance().Exists` is expensive filesystem operation.
> The problem with that `sc.module_sp` is it is initialized above with some 
> side effects. I think you should be fine without needing any `sc`. The 
> following code does not pass the testcase for me but I guess you may fix it 
> better:
> ```
>   // Try finding the file using elfutils' debuginfod
>   if (!FileSystem::Instance().Exists(m_file_spec) &&
>   debuginfod::isAvailable())
> target->GetImages().ForEach(
> [&](const ModuleSP _sp) -> bool {
>   llvm::Expected cache_path = debuginfod::findSource(
>   module_sp->GetUUID(), file_spec.GetCString());
>   if (!cache_path) {
> module_sp->ReportWarning(
> "An error occurred while finding the "
> "source file %s using debuginfod for build ID %s: %s",
> file_spec.GetCString(),
> sc.module_sp->GetUUID().GetAsString("").c_str(),
> llvm::toString(cache_path.takeError()).c_str());
>   } else if (!cache_path->empty()) {
> m_file_spec = FileSpec(*cache_path);
> m_mod_time = 
> FileSystem::Instance().GetModificationTime(*cache_path);
> return false;
>   }
>   return true;
> });
> ```
> 
Please ignore this comment + code fragment, I think it should not be needed.
(Just the `isAvailable()` check should be moved.)


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

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

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil requested changes to this revision.
jankratochvil added inline comments.
This revision now requires changes to proceed.



Comment at: lldb/cmake/modules/FindDebuginfod.cmake:59
+endif()
\ No newline at end of file


"No newline at end of file", this is what saving this diff, git apply --index 
and git diff says to me.



Comment at: lldb/include/lldb/Host/DebugInfoD.h:27
+// cached version of the file. If there  was an error, we return that instead.
+llvm::Expected findSource(const UUID ,
+   const std::string );

Describe what does mean a returned `std::string("")` - that no error happened 
but server does not know this UUID/path.



Comment at: lldb/source/Core/SourceManager.cpp:408
+  if ((!file_spec.GetDirectory() && file_spec.GetFilename()) ||
+  !FileSystem::Instance().Exists(m_file_spec)) {
 // If this is just a file name, lets see if we can find it in the

I do not like this extra line as it changes behavior of LLDB unrelated to 
`debuginfod`. IIUC if the source file with fully specified directory+filename 
in DWARF does not exist but the same filename exists in a different directory 
of the sourcetree LLDB will now quietly use the different file. That's a bug.
I think it is there as you needed to initialize `sc.module_sp`.



Comment at: lldb/source/Core/SourceManager.cpp:460
+  if (!FileSystem::Instance().Exists(m_file_spec) &&
+  debuginfod::isAvailable() && sc.module_sp) {
+llvm::Expected cache_path = debuginfod::findSource(

Make the `debuginfod::isAvailable()` check first as it is zero-cost, 
`FileSystem::Instance().Exists` is expensive filesystem operation.
The problem with that `sc.module_sp` is it is initialized above with some side 
effects. I think you should be fine without needing any `sc`. The following 
code does not pass the testcase for me but I guess you may fix it better:
```
  // Try finding the file using elfutils' debuginfod
  if (!FileSystem::Instance().Exists(m_file_spec) &&
  debuginfod::isAvailable())
target->GetImages().ForEach(
[&](const ModuleSP _sp) -> bool {
  llvm::Expected cache_path = debuginfod::findSource(
  module_sp->GetUUID(), file_spec.GetCString());
  if (!cache_path) {
module_sp->ReportWarning(
"An error occurred while finding the "
"source file %s using debuginfod for build ID %s: %s",
file_spec.GetCString(),
sc.module_sp->GetUUID().GetAsString("").c_str(),
llvm::toString(cache_path.takeError()).c_str());
  } else if (!cache_path->empty()) {
m_file_spec = FileSpec(*cache_path);
m_mod_time = 
FileSystem::Instance().GetModificationTime(*cache_path);
return false;
  }
  return true;
});
```




Comment at: lldb/source/Host/common/DebugInfoD.cpp:50
+   "invalid build ID: %s",
+   buildID.GetAsString("").c_str());
+

It should not be an error:
```
echo 'int main(void) { return 0; }' >/tmp/main2.c;gcc -o /tmp/main2 
/tmp/main2.c -Wall -g -Wl,--build-id=none;rm 
/tmp/main2.c;DEBUGINFOD_URLS=http://localhost:8002/ ./bin/lldb /tmp/main2 -o 'l 
main' -o q
(lldb) target create "/tmp/main2"
Current executable set to '/tmp/main2' (x86_64).
(lldb) l main
warning: (x86_64) /tmp/main2 An error occurred while finding the source file 
/tmp/main2.c using debuginfod for build ID A9C3D738: invalid build ID: A9C3D738
File: /tmp/main2.c
(lldb) q
```




Comment at: lldb/test/Shell/SymbolFile/DWARF/source-list.cpp:103
+// TEST-3: File: /my/new/path/test.cpp
+// TEST-3: 123
+// TEST-3-NEXT:{{[0-9]+}}   // Some context lines before

`s/123/{{[0-9]+}}/?`



Comment at: lldb/test/Shell/SymbolFile/DWARF/source-list.cpp:136
+// the function.
\ No newline at end of file


"No newline at end of file", this is what saving this diff, git apply --index 
and git diff says to me.


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

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

[Konrad Wilhelm Kleine via Phabricator via lldb-commits]

kwk marked 10 inline comments as done.
kwk added a comment.

@labath @jankratochvil @fche2 I've addressed all your comments and hope the 
patch is good to go now.




Comment at: lldb/source/Host/common/DebugInfoD.cpp:43
+  buildID.GetBytes().size() ==
+  sizeof(llvm::support::ulittle32_t)) // .gnu_debuglink crc32
+return llvm::createStringError(llvm::inconvertibleErrorCode(),

labath wrote:
> jankratochvil wrote:
> > If it is done this way (and not in `libdebuginfod.so`) I think there should 
> > be `<=8` because LLDB contains:
> > ```
> > if (gnu_debuglink_crc) {
> >   // Use 4 bytes of crc from the .gnu_debuglink section.
> >   u32le data(gnu_debuglink_crc);
> >   uuid = UUID::fromData(, sizeof(data));
> > } else if (core_notes_crc) {
> >   // Use 8 bytes - first 4 bytes for *magic* prefix, mainly to 
> > make
> >   // it look different form .gnu_debuglink crc followed by 4 
> > bytes
> >   // of note segments crc.
> >   u32le data[] = {u32le(g_core_uuid_magic), 
> > u32le(core_notes_crc)};
> >   uuid = UUID::fromData(data, sizeof(data));
> > }
> > ```
> > 
> 4 would have probably been fine too, as I don't think a core file "uuid" can 
> make its way into here. In either case, we should document what is this 
> working around, as 4 or 8 byte uuids are technically valid.
@labath. I've added a documentation for the workaround.


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