[PATCH] D118633: [Symbolizer] Add Build ID flag to llvm-symbolizer.

Tue Feb 8 01:24:05 PST 2022

phosek accepted this revision.
phosek added a comment.

LGTM

================
Comment at: llvm/lib/DebugInfo/Symbolize/Symbolize.cpp:439
+    // Record that an error occurred.
+    recordPath("");
     return false;
----------------
mysterymath wrote:
> phosek wrote:
> > IIUC this slightly changes the semantics. Currently, even if we failed to download a file (for example because of the intermittent network error), we would try again on then next symbolization request. With this change, we would cache the error result (that is, an empty string) after the first attempt and never retry. Maybe it would be better to drop this line to preserve the current semantics?
> This is slightly tricky: we should retry the network request if it failed previously, but we probably shouldn't spam the console with repeated error messages for the build ID, since that differs from the way errors work for paths (one per path).
> 
> I've tried to keep the error behavior while still retrying the lookup under the hood. If a retry succeeds, the data becomes available for all future symbolization request; otherwise, the error is silenced.
Personally I'd prefer getting an error on every unsuccessful attempt; that may be more spammy but avoids hiding potentially useful information from the user. If the errors go to stderr, user can always redirect it to `/dev/null` if needed. Either solution is fine with me though.

================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:367-368

+  if (!Args.getLastArgValue(OPT_build_id_EQ).empty() &&
+      !Args.getLastArgValue(OPT_obj_EQ).empty()) {
+    errs() << "error: cannot specify both --build-id and --obj\n";
----------------
I'd use `hasArg`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118633