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
