This is an archive of the discontinued LLVM Phabricator instance.

[llvm-readobj] Rename JSON Flag fields to be more consistent
ClosedPublic

Authored by paulkirth on Oct 31 2022, 10:42 AM.

Details

Summary

Today the JSON uses Value and RawValue when printing Flags, when really
the Value field is always the name of an Enum variant, and RawValue is its
underlying numeric value. Similarly, we rename the RawFlags key to Value,
to match the new scheme. This also allows JSON parsing to use consistent logic
for Flag types.

Diff Detail

Event Timeline

paulkirth created this revision.Oct 31 2022, 10:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 31 2022, 10:42 AM
paulkirth requested review of this revision.Oct 31 2022, 10:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 31 2022, 10:42 AM
paulkirth updated this revision to Diff 484125.Dec 19 2022, 3:53 PM

Fix lit test, not updated during a rebase

paulkirth updated this revision to Diff 484392.Dec 20 2022, 3:02 PM

Fix test and rebase

paulkirth updated this revision to Diff 492287.Jan 25 2023, 4:12 PM
paulkirth retitled this revision from [readobj] Rename JSON Flag fields to be more consistent to [llvm-readobj] Rename JSON Flag fields to be more consistent.
paulkirth edited the summary of this revision. (Show Details)

Rebase.

jhenderson accepted this revision.Feb 23 2023, 12:56 AM

LGTM, with one caveat: has there been an LLVM release which produced the old name in a usable form? If so, this probably needs a release note. If on the other hand, the JSON would have been invalid, or the name wasn't emitted at all, there's no need to change anything.

Sorry for taking so long to get back around to this patch series. Is there a particular one you'd like me to look at as a priority? I'll otherwise aim to do 1 or 2 a day, depending on my other work and review load.

This revision is now accepted and ready to land.Feb 23 2023, 12:56 AM

LGTM, with one caveat: has there been an LLVM release which produced the old name in a usable form? If so, this probably needs a release note. If on the other hand, the JSON would have been invalid, or the name wasn't emitted at all, there's no need to change anything.

I'm not sure. I'm almost positive that we always emit something fundamentally broken when we emit anything that contains Flags. Still it's probably best to just add the release note, and err on the side of caution, so I'll add that when I get a chance later today.

Sorry for taking so long to get back around to this patch series. Is there a particular one you'd like me to look at as a priority? I'll otherwise aim to do 1 or 2 a day, depending on my other work and review load.

I guess bottom of the stack to the top is preferable, just to make landing easier, but other than that I don't think there is much preference.

LGTM, with one caveat: has there been an LLVM release which produced the old name in a usable form? If so, this probably needs a release note. If on the other hand, the JSON would have been invalid, or the name wasn't emitted at all, there's no need to change anything.

I'm not sure. I'm almost positive that we always emit something fundamentally broken when we emit anything that contains Flags. Still it's probably best to just add the release note, and err on the side of caution, so I'll add that when I get a chance later today.

You might want to make the release note more general. Something along the lines of "Made significant changes to JSON output format of llvm-readobj/llvm-readelf to improve correctness and clarity." That way, it'll cover all the other issues too.

Sorry for taking so long to get back around to this patch series. Is there a particular one you'd like me to look at as a priority? I'll otherwise aim to do 1 or 2 a day, depending on my other work and review load.

I guess bottom of the stack to the top is preferable, just to make landing easier, but other than that I don't think there is much preference.

No problem, will do.

You might want to make the release note more general. Something along the lines of "Made significant changes to JSON output format of llvm-readobj/llvm-readelf to improve correctness and clarity." That way, it'll cover all the other issues too.

That's a good point. I was thinking that we'd add more specific notes as needed in the other patches, but this is probably better.

Don't know what the culprit is, but the premerge checks here have failed miserably on some of the llvm-readobj testing, so it's probably worth investigating them.

Looks like you went with the feature-specific update to the release note, rather than the more generic suggestion I made?

paulkirth updated this revision to Diff 502215.Mar 3 2023, 12:13 PM

Fix tests after rebasing.

I hadn't accounted for the additional tests added earlier in the stack when rebasing.

paulkirth updated this revision to Diff 502278.Mar 3 2023, 3:46 PM

Update Release Notes to be more general

jhenderson accepted this revision.Mar 17 2023, 1:20 AM
hans added a subscriber: hans.Mar 21 2023, 5:29 AM

This broke some Chromium builds: https://bugs.chromium.org/p/chromium/issues/detail?id=1426287

It turns out we have a tool that consumes this JSON. Maybe that one can be made to accept the encoding both before and after this change, but there may be others.

That raises the question of what the compatibility story is here. Are we okay with breaking all consumers of this JSON?

This broke some Chromium builds: https://bugs.chromium.org/p/chromium/issues/detail?id=1426287

It turns out we have a tool that consumes this JSON. Maybe that one can be made to accept the encoding both before and after this change, but there may be others.

That raises the question of what the compatibility story is here. Are we okay with breaking all consumers of this JSON?

@paulkirth will be better placed to discuss this, but it's worth noting that a significant amount of the JSON output prior to @paulkirth's work was fundamentally broken. I think it makes a degree of sense to make other changes like this that aren't necessarily required for compatibility, whilst everything else is being overhauled. Longer-term, I do believe we should aim for compatibility, unless there's a good reason to break it.

This broke some Chromium builds: https://bugs.chromium.org/p/chromium/issues/detail?id=1426287

It turns out we have a tool that consumes this JSON. Maybe that one can be made to accept the encoding both before and after this change, but there may be others.

That raises the question of what the compatibility story is here. Are we okay with breaking all consumers of this JSON?

Unfortunately, I think we kind of need to be OK with that here. The fact is the JSON output should probably have been considered experimental, or kept behind a hidden flag until it had been vetted a bit more.

Prior to my patches, almost nothing out of llvm-readobj was valid JSON. Portions of it would be OK(sometimes even large portions), but there were many changes we needed to make to have the output be considered valid, and usable by existing tools like jq. It wasn't so bad that you couldn't work around it, but it was pretty painful. This particular renaming could arguably have been avoided, but it does make it possible to process all "flag" types uniformly, which didn't happen under the old schema. Since we were already breaking everything else, this seems to be a good time to make a change like that. I don't foresee the need to do more updates like this, where we modify the schema.

Given the trouble I've had using the JSON output programmatically, I doubt there are many consumers. I think it's also likely that anyone affected would appreciate the tool emitting proper JSON that any off-the-shelf parser can consume, so that they can ditch the custom logic required to use the JSON emitted by llvmreadobj`.

I do dislike that this is probably breaking downstream consumers though. I don't know what else we can do since we're fixing a fundamental problem.

hans added a comment.Mar 22 2023, 6:59 AM

Fair enough. It looks like the fix on our side is easy at least.