[PATCH] D143310: [llvm-readobj] Make relocations output valid JSON

[Paul Kirth via Phabricator via llvm-commits]

paulkirth added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/json-relocations.test:7
+
+# CHECK:        "Section (1) .rela.text": {
+# CHECK-NEXT:          "Relocation": {
----------------
jhenderson wrote:
> Nit: you can get rid of the excessive indentation for the purposes of the test. Just have it all indented consistently.
oh, I just noticed this is a key ... We can't do this. There is no way to parse keys like this programmatically. I know the current output is broken, but I don't know if this is much of an improvement.  It just papers over the underlying problem w/ JSON output, and introduces a new problem for consumers that isn't readily obvious.   


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:6641
+    std::stringstream DictName;
+    DictName << "Section (" << std::to_string(SecNdx) << ") " << Name.str();
+    DictScope D(W, DictName.str());
----------------
aidengrossman wrote:
> paulkirth wrote:
> > I’m not sure that this is the approach we should be going for with JSON output. Ideally we’d be providing better structure than the text output and not lumping things together like this. 
> > 
> > My take for some relocs is here https://reviews.llvm.org/D137089.  That builds on earlier work cleaning up and making changes to the json output. There’s about 7 patches in that stack so once you dig in you’ll find lots of things actually need fixing in the JSON output. 
> I totally didn't see that you had a patch up for relocations before writing this. There's definitely a lot of things that need fixing for JSON output. This patch was intended solely to get output from `--relocs --expand-relocs` parseable, but richer information would definitely be more useful. Based on a quick glance, it seems like this should be complementary to the current patches you have up (so we're not duplicating effort)? I'll work on some modifications to make the output richer/more JSONic.
>  Based on a quick glance, it seems like this should be complementary to the current patches you have up (so we're not duplicating effort)? I'll work on some modifications to make the output richer/more JSONic.

I'm not opposed to your work here, but I'm fairly sure that w/ my stack this path would never be taken when outputting JSON. IDK if that's super relevant, but I wanted to point that out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143310