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 <https://github.com/llvm/llvm-project/blob/master/lldb/include/lldb/Core/Module.h#L972> 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
