[PATCH] D157210: [symbolizer] Change reaction on invalid input

[James Henderson via Phabricator via llvm-commits]

jhenderson requested changes to this revision.
jhenderson added a comment.
This revision now requires changes to proceed.

Generally looks fine, but there are a number of small issues, and I'm not convinced by the JSON output behaviour change, hence requesting changes to clear the "accepted" state.



================
Comment at: llvm/docs/CommandGuide/llvm-symbolizer.rst:21
 standard input. If no input name is specified on the command-line, but addresses
-are, or if at any time an input value is not recognized, the input is simply
-echoed to the output.
+are, the first address value is treated as input name. If an input value is not
+recognized, it reports that source information is not found.
----------------



================
Comment at: llvm/docs/ReleaseNotes.rst:137
+  information is not found.
+
+
----------------
Nit: I don't think you want 2 blank lines (1 will do).


================
Comment at: llvm/test/tools/llvm-symbolizer/file-prefix.test:8
+RUN: llvm-symbolizer "CODE FILE:FILE:%p/Inputs/addr.exe 0x40054d" 2>%t.err | count 0
+RUN:   FileCheck %s --check-prefix=FILEFILE <%t.err
+FILEFILE: llvm-symbolizer{{.*}}: error: 'CODE FILE:FILE:{{.*}}/Inputs/addr.exe 0x40054d': Duplicate input file specification prefix
----------------
Rather than redirecting stdin, use the `--input-file` option.

(Motivation for me is that the shell I commonly use, Windows PowerShell, doesn't support `<` for stdin redirection)


================
Comment at: llvm/test/tools/llvm-symbolizer/invalid-input-address.test:2
 # Use address that can't fit in a 64-bit number. Show that llvm-symbolizer
-# simply echoes it as per other malformed input addresses.
+# simply treats it as unknown symbol.
 RUN: llvm-symbolizer --obj=%p/Inputs/addr.exe 0x10000000000000000 | FileCheck --check-prefix=LARGE-ADDR %s
----------------



================
Comment at: llvm/test/tools/llvm-symbolizer/invalid-input-address.test:26
+BAD-INPUT-NEXT: ??:0
\ No newline at end of file

----------------
Nit: missing new line at EOF.


================
Comment at: llvm/test/tools/llvm-symbolizer/options-from-env.test:1-2
-# RUN: env LLVM_SYMBOLIZER_OPTS='0 1 --verbose' llvm-symbolizer 2 | FileCheck %s
-# RUN: env LLVM_ADDR2LINE_OPTS='0 1 --verbose' llvm-addr2line 2 | FileCheck %s
+RUN: env LLVM_SYMBOLIZER_OPTS='-e %p/Inputs/discrim --verbose' llvm-symbolizer 0x400590 | FileCheck --check-prefix=SYMB %s
+RUN: env LLVM_ADDR2LINE_OPTS='-e %p/Inputs/discrim --verbose' llvm-addr2line 0x400590 | FileCheck %s
 
----------------
Let's use `LLVM` and `GNU` prefixes rather than `SYMB` and `CHECK` (corresponds to LLVM and GNU styles).


================
Comment at: llvm/test/tools/llvm-symbolizer/output-style-json-code.test:28
 ## Invalid first argument before any valid one.
-# NO-INLINES:{"Error":{"Message":"unable to parse arguments: some text"},"ModuleName":"{{.*}}/Inputs/addr.exe"}
+# NO-INLINES:{"ModuleName":"{{.*}}/Inputs/addr.exe","Symbol":[{"Column":0,"Discriminator":0,"FileName":"","FunctionName":"","Line":0,"StartAddress":"","StartFileName":"","StartLine":0}]}
 ## Resolve valid address.
----------------
These seem like a regression to me in the JSON output, as it's no longer recording the error?


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:161
 
-static bool parseCommand(StringRef BinaryName, bool IsAddr2Line,
-                         StringRef InputString, Command &Cmd,
-                         std::string &ModuleName, object::BuildID &BuildID,
-                         uint64_t &ModuleOffset) {
+static Error getStringError(StringRef Msg) {
+  return make_error<StringError>(Msg, inconvertibleErrorCode());
----------------
`createStringError` or `makeStringError` would be better than `getStringError` in my opinion - `get` implies to me that the Error exists and that you're just fetching it from somewhere.


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:188
       if (HasFilePrefix || HasBuildIDPrefix)
-        // Input file specification prefix is already seen.
-        return false;
+        return getStringError("Duplicate input file specification prefix");
       HasFilePrefix = true;
----------------
Nit: here and below, error messages should use lower case for the first letter. See https://llvm.org/docs/CodingStandards.html#error-and-warning-messages.


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:233
+  if (Offset.getAsInteger(IsAddr2Line ? 16 : 0, ModuleOffset))
+    return errorCodeToError(errc::invalid_argument);
+  return Error::success();
----------------
Rather than just producing "invalid argument", could we give additional information about what the argument is that is deemed to be invalid, e.g. "1foo is not a valid address" or something like that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157210