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.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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?
Comment Actions
Fix tests after rebasing.
I hadn't accounted for the additional tests added earlier in the stack when rebasing.
Comment Actions
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?
Comment Actions
@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.
Comment Actions
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.