[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