[PATCH] D114846: [llvm] [Debuginfod] LLVM debuginfod server.

[Daniel Thornburgh via Phabricator via llvm-commits]

mysterymath added inline comments.


================
Comment at: llvm/test/tools/llvm-debuginfod/llvm-debuginfod.test:25-27
+# RUN: DEBUGINFOD_CACHE_PATH=%t %python %s \
+# RUN:   --tool-cmd 'llvm-debuginfod-find --dump --executable 2c39b7557c50162aaeb5a3148c9f76e6e46012e3' | \
+# RUN:   diff - %S/Inputs/main.exe
----------------
It doesn't seem like the python script does anything for these two invocations; the DEBUGINFOD_CACHE_PATH is set manually, and there's nothing to wait for. It'd be clearer just to use `llvm-debuginfod-find` directly. This should also help remove conditional logic from the Python script.


================
Comment at: llvm/test/tools/llvm-debuginfod/llvm-debuginfod.test:51
+        server_args,
+        env=env,
+        stdout=subprocess.PIPE)
----------------
Consider just `env=os.environ`, since `env` is otherwise unused.


================
Comment at: llvm/test/tools/llvm-debuginfod/llvm-debuginfod.test:54
+    port = -1
+    # Obtain the port
+    stdout_reader = io.TextIOWrapper(process.stdout, encoding='ascii')
----------------



================
Comment at: llvm/test/tools/llvm-debuginfod/llvm-debuginfod.test:58
+    port = int(stdout_line.split()[-1])
+    # Wait until a directory scan is completed
+    while True:
----------------



================
Comment at: llvm/tools/llvm-debuginfod/llvm-debuginfod.cpp:43
+static cl::opt<double>
+    ScanInterval("t", cl::init(10000),
+                 cl::desc("Number of seconds to wait between subsequent scans "
----------------
noajshu wrote:
> noajshu wrote:
> > pcc wrote:
> > > phosek wrote:
> > > > 10000s is a long time between scans, elfutils defaults to 300s which I think would be better.
> > > If the server implements the following behavior, do we need a scan interval?
> > > 
> > > - If the build-id is not found by a local lookup, rescan immediately and look up the build-id again before returning 404.
> > > 
> > > - To protect against DoS attacks, do not rescan more frequently than once per N seconds (e.g. N = 30).
> > > 
> > > debuginfod does not have this behavior and it was somewhat of a pain point when using it for local development.
> > > 10000s is a long time between scans, elfutils defaults to 300s which I think would be better.
> > 
> > Thanks, will update this.
> > 
> > > If the server implements the following behavior, do we need a scan interval?
> > 
> > Thanks for this suggestion, this would be really nice for local development. I've just updated D114845 (which implements `DebuginfodCollection`) to support this behavior via `updateIfStale()`, though we still need to add the option here. It's not much trouble to support both periodic and on-demand update triggers, so perhaps we could allow the when-to-update behavior to be customized by the cl::opts.
> > 10000s is a long time between scans, elfutils defaults to 300s which I think would be better.
> 
> 
It looks like this hasn't yet been updated as of the most recent patch.


================
Comment at: llvm/tools/llvm-debuginfod/llvm-debuginfod.cpp:97
+        std::chrono::milliseconds(static_cast<int>(ScanInterval * 1000))));
+  Pool.wait();
+}
----------------
Consider adding an llvm_unreachable() and a comment after this for documentation purposes; it was surprising to discover that the structure of the pool means it will never finish, but that there's still a wait call.


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

https://reviews.llvm.org/D114846