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

[Simon Giesecke via Phabricator via llvm-commits]

simon.giesecke added a comment.

In D102224#2752153 <https://reviews.llvm.org/D102224#2752153>, @clayborg wrote:

> 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

Ah, sorry for missing that. It seems so obvious now that I don't know how I missed it.

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

In a first step, we just wanted to switch from one command line tool (addr2line) to another. For some use cases, linking against the library would be an option, and probably indeed one we will take in a future step. In other use cases, this might be less feasible, notably pprof, which is written in Go, which also calls other command line utilities such as nm, addr2line or llvm-symbolizer.

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

I agree that having a JSON output would be better for further processing. I just wanted to keep changes minimal for now, and stick with the output format the tool has. Just after putting up this review, I noticed that https://reviews.llvm.org/D96883 added a `--json` option to llvm-symbolizer.

We should probably use the same format here.

That makes me wonder whether the direction this takes is actually the right one. Another option might be to add support for GSYM to llvm-symbolizer. Do you think this would be a better direction?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102224