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

Wed Feb 8 12:09:20 PST 2023

aidengrossman abandoned this revision.
aidengrossman added inline comments.

================
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());
----------------
paulkirth wrote:
> 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.
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143310