[PATCH] D113717: [Symbolizer][Debuginfo] Add debuginfod client to llvm-symbolizer.

[Noah Shutty via Phabricator via llvm-commits]

noajshu added a comment.

Thank you for the helpful comments @jhenderson , I am uploading chnage

In D113717#3159258 <https://reviews.llvm.org/D113717#3159258>, @dblaikie wrote:

> In D113717#3157634 <https://reviews.llvm.org/D113717#3157634>, @jhenderson wrote:
>
>> Not looked at anything else in the patch stack, but I can more-or-less guess what is going on (storing the debug data on a remote server for the symbolizer and other tools to be able to look up?). If so, in general, this looks fine to me. My only slight concern is I wonder how many places (especially build bots) will exercise this test, since it requires a new dependency. Might be worth approaching some build bot owners and asking if they'd be willing to install curl as part of their setup.
>
> I seem to recall some mention that this API was usable with a local cache even without the curl dependency? Perhaps a test using only a local cache (not needing a server or curl) could be added as well? (the test with curl/etc might be good to keep too - though perhaps it could be removed if the API functionality is already tested elsewhere - if it isn't/can't readily be tested elsewhere then here's fine too)

D112758 <https://reviews.llvm.org/D112758> includes unit testing that verifies the local cache works without curl. I updated the end-to-end tests of this diff (D113717 <https://reviews.llvm.org/D113717>) and added an additional test that just hits the local cache and does not require Curl. I also added checks for the Curl-dependent test to verify that even after the server is taken down, the debuginfo is still accessible in the local cache.
Is it ok to keep all of these tests? If I had to pick between the end-to-end cache test here and the unit test in D112758 <https://reviews.llvm.org/D112758>, I would favor the unit test since it's more likely to be run on all bots. However the end-to-end test here is additionally checking that the debuginfod client has been properly integrated with the symbolizer.

I think it's a great idea to make sure libcurl will be tested on some buildbots. Since `LLVM_ENABLE_CURL` defaults to `ON` in D112753 <https://reviews.llvm.org/D112753>, llvm will be built with curl by default as long as it's found by cmake. It's possible the library is already installed on some buildbots. Do you know how to view the buildbot configuration scripts?



================
Comment at: llvm/lib/DebugInfo/Symbolize/Symbolize.cpp:390-393
+  if (!PathOrErr) {
+    consumeError(PathOrErr.takeError());
+    return false;
+  }
----------------
jhenderson wrote:
> It would be good if this failure path could be tested, unless this can be just said to be tested with the other generic symbolizer tests?
I agree. I've just added additional lit tests which hit this failure path.


================
Comment at: llvm/test/tools/llvm-symbolizer/debuginfod.py:8
+# RUN:   %t/buildid/127da749021c1fc1a58cba734a1f542cbe2b7ce4/debuginfo
+# RUN: python %S/debuginfod.py %t %t 'llvm-symbolizer --print-address \
+# RUN:   --obj=%t/addr.exe 0x40054d' | FileCheck %s
----------------
jhenderson wrote:
> 
Thanks, I just split this script into `debuginfod.test` and `debuginfod.py` to make it a bit easier to read. If there's a preference towards a single file I can instead merge them back together and use `%s`.


================
Comment at: llvm/test/tools/llvm-symbolizer/debuginfod.py:17
+import http.server
+import functools
+import subprocess
----------------
jhenderson wrote:
> What's this being used for?
I just added a comment at the top of this script to explain its purpose. The short version is we want to run an end-to-end test of the debuginfod client functionality of the symbolizer, and so we need a working debuginfod server running at the same time.


================
Comment at: llvm/test/tools/llvm-symbolizer/debuginfod.py:49-51
+    parser.add_argument('serve_path')
+    parser.add_argument('cache_directory')
+    parser.add_argument('tool_cmd')
----------------
jhenderson wrote:
> It would help the RUN line above be more self-descriptive (and flexible for the future) if these were dashed arguments, rather than positionals.
Good idea, changed this!


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

https://reviews.llvm.org/D113717