[llvm-readobj] Make relocations output valid JSON

[aidengrossman]

Summary

Currently, outputting relocations using --relocs --expand-relocs in
llvm-readobj will produce invalid JSON since the section scopes are
outputted directly. This patch changes the section scopes to use a
DictScope which is automatically printed correctly by the
JSONScopedPrinter, making the JSON valid.

Regression test added.

[aidengrossman]

Probably not the most idiomatic way to work with strings here (and I'd prefer to avoid a new header inclusion for stringstream), so open to suggestions there. Otherwise should be good to go.

[jhenderson]

Thanks for the fix. Could you please post the output without your change, just so that it's obvious what's going on before this fix?

I've also added @paulkirth as a reviewer, as he's recently posted a whole series of patches to fix JSON things. I haven't had a chance to go through them yet due to a combination of vacation and catching up with my regular work, but it's possible there's some overlap.

llvm/test/tools/llvm-readobj/ELF/json-relocations.test

7–19

Nit: you can get rid of the excessive indentation for the purposes of the test. Just have it all indented consistently.

llvm/tools/llvm-readobj/ELFDumper.cpp

6640–6642

If I'm not mistaken, you can use Twines to do this. Something along the lines of the inline edit (NB: I've not got quick access to check the exact final converesion to string, but it'll be something like this).

[paulkirth]

llvm/tools/llvm-readobj/ELFDumper.cpp

6641

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.

[aidengrossman]

llvm/tools/llvm-readobj/ELFDumper.cpp

6641

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.

[paulkirth]

llvm/test/tools/llvm-readobj/ELF/json-relocations.test

7

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.

llvm/tools/llvm-readobj/ELFDumper.cpp

6641

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.

[aidengrossman]

llvm/tools/llvm-readobj/ELFDumper.cpp

6641

Just saw D137094. Not sure why I didn't see it before, but that would definitely eliminate the need for this work in the JSON case and the implementation is already shored up (ie outputting the relevant metadata into other tags rather than keeping everything in the key). So this patch is pretty much entirely redundant with that. Sorry for taking up everyone's time here.