[PATCH] D102224: Add option to llvm-gsymutil to read addresses from stdin.

[Greg Clayton via Phabricator via llvm-commits]

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

In D102224#2750014 <https://reviews.llvm.org/D102224#2750014>, @simon.giesecke wrote:

> I know this isn't covered by tests right now. I am not sure what the policy is for the command line utilities. Currently, llvm-gsymutil doesn't have any tests IIUC. If tests should be added, please direct me to a good example, and I will be happy to add them.

yes tests should be added and there are indeed llvm-gsymutil tests. See this directory:

llvm/test/tools/llvm-gsymutil

In D102224#2750529 <https://reviews.llvm.org/D102224#2750529>, @simon.giesecke wrote:

> Hm, clang-tidy complains about two issues in code I copied from llvm-symbolizer. Should these be addressed?

if it is new code, then yes.

One thing to note is that there is library code that tools can directly link to, just like llvm-gsymutil links against the "DebugInfoGSYM" library. Then your lookups are very simple and you would call a function just like doLookup() that you added. Is there a reason you are wanting to run a command line tool instead of linking against the code?

Also, I am not a fan of text scraping from the output of a tool unless it is purely for humans to read. If you are going to use this output from another tools, I would vote to add a new option: "--json" so that the output would be formatted using structured data like JSON. Something like:

  $ llvm-gsymutil --json --addresses-from-stdin
  0x1000 /tmp/a.gsym
  0x2000 /tmp/b.gsym

And this would return output in a specific JSON format, something like:

  [
    { "lookupAddress": 4096, "gsym": "/tmp/a.gsym", ... },
    { "lookupAddress": 8192, "gsym": "/tmp/b.gsym", ... }
  ]

The "..." above would be a JSON version of the llvm::gsym::LookupResult object, or an error message.



================
Comment at: llvm/tools/llvm-gsymutil/llvm-gsymutil.cpp:117
+    desc("Lookup addresses in a GSYM file that are read from stdin"),
+    cat(LookupOptions));
 } // namespace
----------------
You will need to specify what format the STDIN format needs to be. From reading to the code below it seems to be:

```
<addr> <gsym-path> [<addr> <gsym-path> ...]
```


================
Comment at: llvm/tools/llvm-gsymutil/llvm-gsymutil.cpp:490
+      OS << "error: no input files or addresses can be specified when using "
+            "the --addresses-from-stdin "
+            "option.\n";
----------------
There should be a test for this error


================
Comment at: llvm/tools/llvm-gsymutil/llvm-gsymutil.cpp:501
+    while (fgets(InputString, sizeof(InputString), stdin)) {
+      // Strip newline characters.
+      std::string StrippedInputString(InputString);
----------------
Can we use C++ STL here? Or there might be some other LLVM tools that use STDIN using some other LLVM input wrapping?

```
std::string line;
std::getline(std::cin, line);
````



================
Comment at: llvm/tools/llvm-gsymutil/llvm-gsymutil.cpp:505
+                     [](char c) { return c == '\r' || c == '\n'; });
+
+      StringRef AddrStr, GSYMPath;
----------------
What happens if you send the following input into this tool:

"0x1000 /tmp/a.gsym
0x2000 /tmp/b.gsym
"

If I read the code above correctly, it will end up with a string:

"0x1000 /tmp/a.gsym0x2000 /tmp/b.gsym"

because you are erasing all newline characters and the newline between "0x1000 /tmp/a.gsym" and "0x2000 /tmp/b.gsym" will be removed.


================
Comment at: llvm/tools/llvm-gsymutil/llvm-gsymutil.cpp:507-509
+      std::tie(AddrStr, GSYMPath) =
+          llvm::StringRef{StrippedInputString}.split(' ');
+
----------------
Does any other tool use this type of stdin format where you specify an address and a file? Unless any other existing tool does (like atos?) I would think that we would run the tool with a GSYM file and then do multiple lookups on that one GSYM file:

```
$ llvm-gsymutil /tmp/a.gsym --addresses-from-stdin
0x1000 0x2000 0x3000
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102224