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

Thu Dec 2 00:28:01 PST 2021

jhenderson added a comment.

In D113717#3162690 <https://reviews.llvm.org/D113717#3162690>, @noajshu wrote:

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

I'm afraid I don't know either. Perhaps ask on the llvm-dev mailing list?

================
Comment at: llvm/test/tools/llvm-symbolizer/debuginfod-cache.test:5
+RUN: llvm-objcopy --strip-debug %p/Inputs/addr.exe %t/addr.exe
+RUN: mkdir -p %t/buildid/127da749021c1fc1a58cba734a1f542cbe2b7ce4/
+# Symbolizing a stripped binary should fail.
----------------
Does this string need to be this specific value? Might be worth a comment if it does.

================
Comment at: llvm/test/tools/llvm-symbolizer/debuginfod-cache.test:10-11
+NOTFOUND: 0x40054d
+NOTFOUND: main
+NOTFOUND: ??:0:0
+
----------------
Use `-NEXT` suffixes here and below.

================
Comment at: llvm/test/tools/llvm-symbolizer/debuginfod-cache.test:14
+RUN: llvm-objcopy --keep-section=.debug_info %p/Inputs/addr.exe \
+RUN:   %t/llvmcache-9800707741016212219
+
----------------
Similar to my above comment: add a comment why this path is what it is.

================
Comment at: llvm/test/tools/llvm-symbolizer/debuginfod.py:2
+# This script is used to test the debuginfod client functionality of the symbolizer.
+# It first stands up a Python HTTP static file server and then executes the client.
+# This way the client can make debuginfod HTTP requests to the static file server.
----------------

================
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
----------------
noajshu wrote:
> 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`.
I personally am a big fan of keeping support files that are used in only a single test within the same file, as it means if the test is ever deleted/renamed, the support files aren't left lying around with no users. It also is easier to follow what the script is doing: by having them separate, it requires a developer to open up another file to see what a test is actually doing.

Note: An exception to the above is if a script is soon going to be used by other tests that don't yet exist.

================
Comment at: llvm/test/tools/llvm-symbolizer/debuginfod.py:17
+import http.server
+import functools
+import subprocess
----------------
noajshu wrote:
> 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.
I was referring specificaly to the `import functools` which I didn't immediately see a usage of.

================
Comment at: llvm/test/tools/llvm-symbolizer/debuginfod.test:2
+REQUIRES: curl
+# Uses source program from llvm/test/tools/llvm-symbolizer/split-debug.test
+RUN: rm -rf %t
----------------
If you're referring to `addr.exe` - that's not a program specific to one particular test: it's used by a fair number of tests (or at least used to be!). I'd therefore delete this comment, since it is misleading.

It would be useful to put a high-level comment explaining what this test is testing, so that the reason --strip-debug is used is explained somewhere.

================
Comment at: llvm/test/tools/llvm-symbolizer/debuginfod.test:6
+RUN: llvm-objcopy --strip-debug %p/Inputs/addr.exe %t/addr.exe
+RUN: mkdir -p %t/buildid/127da749021c1fc1a58cba734a1f542cbe2b7ce4/
+# Symbolizing a stripped binary should fail.
----------------
Similar to my earlier comment, it would be useful to explain why this string is what it is.

================
Comment at: llvm/test/tools/llvm-symbolizer/debuginfod.test:14
+
+# debuginfod.py stands up a debuginfod server and sets the DEBUGINFOD_URLS
+# before running llvm-symbolizer
----------------

================
Comment at: llvm/test/tools/llvm-symbolizer/debuginfod.test:15-16
+# debuginfod.py stands up a debuginfod server and sets the DEBUGINFOD_URLS
+# before running llvm-symbolizer
+# Now it should symbolize successfully.
+RUN: llvm-objcopy --keep-section=.debug_info %p/Inputs/addr.exe \
----------------

================
Comment at: llvm/test/tools/llvm-symbolizer/debuginfod.test:17-18
+# Now it should symbolize successfully.
+RUN: llvm-objcopy --keep-section=.debug_info %p/Inputs/addr.exe \
+RUN:   %t/buildid/127da749021c1fc1a58cba734a1f542cbe2b7ce4/debuginfo
+RUN: DEBUGINFOD_CACHE_PATH=%t python %S/debuginfod.py --server-path %t \
----------------
Similar to above: explain the need for the llvm-objcopy command.

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

https://reviews.llvm.org/D113717