[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