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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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.



================
Comment at: llvm/lib/DebugInfo/Symbolize/Symbolize.cpp:390-393
+  if (!PathOrErr) {
+    consumeError(PathOrErr.takeError());
+    return false;
+  }
----------------
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?


================
Comment at: llvm/test/tools/llvm-symbolizer/debuginfod.py:2
+# REQUIRES: curl
+## Uses source from split-debug.test
+# RUN: rm -rf %t && mkdir %t
----------------
Not sure I follow which source you're referring to here...


================
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
----------------



================
Comment at: llvm/test/tools/llvm-symbolizer/debuginfod.py:17
+import http.server
+import functools
+import subprocess
----------------
What's this being used for?


================
Comment at: llvm/test/tools/llvm-symbolizer/debuginfod.py:23
+# Serves files at the serve_path, then runs the tool with specified args.
+# Sets the DEBUGINFOD_CACHE_PATH env var to point at given cache_directory.
+# Sets the DEBUGINFOD_URLS env var to point at the local server.
----------------



================
Comment at: llvm/test/tools/llvm-symbolizer/debuginfod.py:25
+# Sets the DEBUGINFOD_URLS env var to point at the local server.
+def test_tool(serve_path, cache_directory, tool_args) -> int:
+    httpd = http.server.ThreadingHTTPServer(
----------------
I'm pretty sure this explicit return type isn't necessary.

Also, should it be `server_path`? (Also applies below)


================
Comment at: llvm/test/tools/llvm-symbolizer/debuginfod.py:39
+        if process.wait() != 0:
+            print('process returned nontrivial error code')
+            return 1
----------------
Perhaps worth including the actual code in this message, to make test debugging easier.


================
Comment at: llvm/test/tools/llvm-symbolizer/debuginfod.py:47
+def main():
+    import argparse
+    parser = argparse.ArgumentParser()
----------------
Any particular reason not to put this at top-level like the other imports?


================
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')
----------------
It would help the RUN line above be more self-descriptive (and flexible for the future) if these were dashed arguments, rather than positionals.


================
Comment at: llvm/test/tools/llvm-symbolizer/debuginfod.py:55
+        args.tool_cmd.split())
+    os._exit(result)
+
----------------
`sys.exit` is the normal way to exit with an exit code.


================
Comment at: llvm/test/tools/llvm-symbolizer/lit.local.cfg:1
+config.suffixes.add('.py')
----------------
You could avoid this by just renaming the test file to have a `.test` suffix.


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

https://reviews.llvm.org/D113717