This is an archive of the discontinued LLVM Phabricator instance.

[readobj] Make JSON output consistent for Other flags
AbandonedPublic

Authored by paulkirth on Oct 6 2022, 6:27 PM.

Details

Summary

Currently in the LLVM/JSON output, the Other field can either be an
integer(when it is 0) or an object/list for other cases. This patch makes the
formatting consistent for all value ranges. This is particularly important for
machine readable output in JSON, as it makes handling the Other field a special
case

Depends on D134843

Diff Detail

Event Timeline

paulkirth created this revision.Oct 6 2022, 6:27 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 6 2022, 6:27 PM
paulkirth requested review of this revision.Oct 6 2022, 6:27 PM
phosek added inline comments.Oct 6 2022, 7:03 PM
llvm/test/tools/llvm-readobj/ELF/dyn-symbols-size-from-hash-table.test
40–41

Can we modify https://github.com/llvm/llvm-project/blob/f4e8f44811e25fd5547d879aafafd621386eb927/llvm/include/llvm/Support/ScopedPrinter.h#L444 to avoid printing \n and startLine() when Flags are empty so we end up with Other [ (0x0) ] for this case.

paulkirth added inline comments.Oct 6 2022, 7:06 PM
llvm/test/tools/llvm-readobj/ELF/dyn-symbols-size-from-hash-table.test
40–41

Yeah, that's a good suggestion. I'll look into handling that case more gracefully.

paulkirth planned changes to this revision.Oct 6 2022, 8:00 PM

I still need to update a number of tests outside of readobj, in addition to the suggestion from @phosek.

I think the approach seems reasonable to me, and am happy with @phosek's suggestion too, though that should probably be a separate patch (I assume it impacts more than this specific case...)?

Still looking into this, but one thing I'm increasingly hesitant about is how closely the LLVM and JSON implementations are coupled. It may make sense to separate the implementations more before we start introducing changes that affect more than the JSON output.

@jhenderson do we have any stability guarantees for the LLVM format? I don't want to introduce churn just for the sake of it, but is the LLVM output intended to be easily machine parsable, or to follow some format we've outlined somewhere? where changes to both formats make sense, I'm happy to share the implementation, but I expect there will eventually reach a point where the JSON and LLVM formats need to diverge to satisfy consumers. If possible, I'd like to establish where that point is, so that we can plan for/recognize it apriori.

Still looking into this, but one thing I'm increasingly hesitant about is how closely the LLVM and JSON implementations are coupled. It may make sense to separate the implementations more before we start introducing changes that affect more than the JSON output.

@jhenderson do we have any stability guarantees for the LLVM format? I don't want to introduce churn just for the sake of it, but is the LLVM output intended to be easily machine parsable, or to follow some format we've outlined somewhere? where changes to both formats make sense, I'm happy to share the implementation, but I expect there will eventually reach a point where the JSON and LLVM formats need to diverge to satisfy consumers. If possible, I'd like to establish where that point is, so that we can plan for/recognize it apriori.

I don't think there's any specific stability guarantees for the LLVM format output, but we'd have to have justifiable reasons to change it. In particular, there are large numbers of tests that use llvm-readobj for testing things, and changing the output may impact those tests. It's possible the changes will be in such a way that a test no longer would fail when a bug was introduced, for example, so you have to be very careful what changes. If you think any non-trivial changes are needed, it would be worth raising on Discourse first too, because other developers may well have a different point of view. In fact, this whole discussion about stability of the output might be worth a Discourse thread, though it would be worth coming up with one or two examples as to what you might want to change in order to make things work.

paulkirth abandoned this revision.Mar 3 2023, 4:04 PM

Abandoning in favor of D137088