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

[paulkirth]

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.

[paulkirth]

Rebase

[paulkirth]

Rebase

[paulkirth]

Rebase

[paulkirth]

Fix lit test, not updated during a rebase

[paulkirth]

Fix test and rebase

[paulkirth]

Rebase.

[paulkirth]

Rebase.

[jhenderson]

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.

[paulkirth]

In D137091#4146674, @jhenderson wrote:

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.

[jhenderson]

In D137091#4147838, @paulkirth wrote:

In D137091#4146674, @jhenderson wrote:

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.

[paulkirth]

In D137091#4149614, @jhenderson wrote:

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.

[paulkirth]

Rebase

[jhenderson]

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]

Fix tests after rebasing.

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

[paulkirth]

Rebase.

[paulkirth]

Update Release Notes to be more general

[paulkirth]

Rebase

[hans]

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?

[jhenderson]

In D137091#4209313, @hans wrote:

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.

[paulkirth]

In D137091#4209313, @hans wrote:

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]

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

llvm/include/llvm/Support/ScopedPrinter.h

private:
   void printAPSInt(const APSInt &Value) {
     JOS.rawValueBegin() << Value;
     JOS.rawValueEnd();
   }
 
   void printFlagsImpl(StringRef Label, HexNumber Value,
                       ArrayRef<FlagEntry> Flags) override {
     JOS.attributeObject(Label, [&]() {
-      JOS.attribute("RawFlags", hexNumberToInt(Value));
+      JOS.attribute("Value", hexNumberToInt(Value));
       JOS.attributeArray("Flags", [&]() {
         for (const FlagEntry &Flag : Flags) {
           JOS.objectBegin();
           JOS.attribute("Name", Flag.Name);
           JOS.attribute("Value", Flag.Value);
           JOS.objectEnd();
         }
       });
     });
   }
 
   void printFlagsImpl(StringRef Label, HexNumber Value,
                       ArrayRef<HexNumber> Flags) override {
     JOS.attributeObject(Label, [&]() {
-      JOS.attribute("RawFlags", hexNumberToInt(Value));
+      JOS.attribute("Value", hexNumberToInt(Value));
       JOS.attributeArray("Flags", [&]() {
         for (const HexNumber &Flag : Flags) {
           JOS.value(Flag.Value);
         }
       });
     });
   }
 
private:
   }
 
   void printHexImpl(StringRef Label, HexNumber Value) override {
     JOS.attribute(Label, hexNumberToInt(Value));
   }
 
   void printHexImpl(StringRef Label, StringRef Str, HexNumber Value) override {
     JOS.attributeObject(Label, [&]() {
-      JOS.attribute("Value", Str);
-      JOS.attribute("RawValue", hexNumberToInt(Value));
+      JOS.attribute("Name", Str);
+      JOS.attribute("Value", hexNumberToInt(Value));
     });
   }
 
   void printSymbolOffsetImpl(StringRef Label, StringRef Symbol,
                              HexNumber Value) override {
     JOS.attributeObject(Label, [&]() {
       JOS.attribute("SymName", Symbol);
       JOS.attribute("Offset", hexNumberToInt(Value));
     });
   }
 
   void printNumberImpl(StringRef Label, StringRef Str,
                        StringRef Value) override {
     JOS.attributeObject(Label, [&]() {
-      JOS.attribute("Value", Str);
-      JOS.attributeBegin("RawValue");
+      JOS.attribute("Name", Str);
+      JOS.attributeBegin("Value");
       JOS.rawValueBegin() << Value;
       JOS.rawValueEnd();
       JOS.attributeEnd();
     });
   }
 
   void printBinaryImpl(StringRef Label, StringRef Str, ArrayRef<uint8_t> Value,
                        bool Block, uint32_t StartOffset = 0) override {

llvm/unittests/Support/ScopedPrinterTest.cpp

TEST_F(ScopedPrinterTest, PrintEnum) {
   };
 
   const char *ExpectedOut = R"(Exists: Name2 (0x2)
 DoesNotExist: 0x5
 )";
 
   const char *JSONExpectedOut = R"({
   "Exists": {
-    "Value": "Name2",
-    "RawValue": 2
+    "Name": "Name2",
+    "Value": 2
   },
   "DoesNotExist": 5
 })";
   verifyAll(ExpectedOut, JSONExpectedOut, PrintFunc);
 }
 
 TEST_F(ScopedPrinterTest, PrintFlag) {
   auto PrintFunc = [](ScopedPrinter &W) {
FirstSecondThirdByteMask [ (0x333)
   FirstByte3 (0x3)
   SecondByte3 (0x30)
   ThirdByte3 (0x300)
 ]
 )";
 
   const char *JSONExpectedOut = R"({
   "ZeroFlag": {
-    "RawFlags": 0,
+    "Value": 0,
     "Flags": []
   },
   "NoFlag": {
-    "RawFlags": 8,
+    "Value": 8,
     "Flags": []
   },
   "Flag1": {
-    "RawFlags": 1,
+    "Value": 1,
     "Flags": [
       {
         "Name": "Name1",
         "Value": 1
       }
     ]
   },
   "Flag1&3": {
-    "RawFlags": 5,
+    "Value": 5,
     "Flags": [
       {
         "Name": "Name1",
         "Value": 1
       },
       {
         "Name": "Name3",
         "Value": 4
       }
     ]
   },
   "ZeroFlagRaw": {
-    "RawFlags": 0,
+    "Value": 0,
     "Flags": []
   },
   "NoFlagRaw": {
-    "RawFlags": 8,
+    "Value": 8,
     "Flags": [
       8
     ]
   },
   "Flag1Raw": {
-    "RawFlags": 1,
+    "Value": 1,
     "Flags": [
       1
     ]
   },
   "Flag1&3Raw": {
-    "RawFlags": 5,
+    "Value": 5,
     "Flags": [
       1,
       4
     ]
   },
   "FlagSorted": {
-    "RawFlags": 7,
+    "Value": 7,
     "Flags": [
       {
         "Name": "A",
         "Value": 4
       },
       {
         "Name": "B",
         "Value": 2
       },
       {
         "Name": "C",
         "Value": 1
       }
     ]
   },
   "NoBitMask": {
-    "RawFlags": 4095,
+    "Value": 4095,
     "Flags": [
       {
         "Name": "FirstByte1",
         "Value": 1
       },
       {
         "Name": "FirstByte2",
         "Value": 2
"Flags": [
       },
       {
         "Name": "ThirdByte3",
         "Value": 768
       }
     ]
   },
   "FirstByteMask": {
-    "RawFlags": 3,
+    "Value": 3,
     "Flags": [
       {
         "Name": "FirstByte3",
         "Value": 3
       }
     ]
   },
   "SecondByteMask": {
-    "RawFlags": 48,
+    "Value": 48,
     "Flags": [
       {
         "Name": "SecondByte3",
         "Value": 48
       }
     ]
   },
   "ValueOutsideMask": {
-    "RawFlags": 1,
+    "Value": 1,
     "Flags": [
       {
         "Name": "FirstByte1",
         "Value": 1
       }
     ]
   },
   "FirstSecondByteMask": {
-    "RawFlags": 255,
+    "Value": 255,
     "Flags": []
   },
   "FirstSecondThirdByteMask": {
-    "RawFlags": 819,
+    "Value": 819,
     "Flags": [
       {
         "Name": "FirstByte3",
         "Value": 3
       },
       {
         "Name": "SecondByte3",
         "Value": 48
)";
   "int32_t-max": 2147483647,
   "int32_t-min": -2147483648,
   "int16_t-max": 32767,
   "int16_t-min": -32768,
   "int8_t-max": 127,
   "int8_t-min": -128,
   "apsint": 9999999999999999999999,
   "label": {
-    "Value": "value",
-    "RawValue": 0
+    "Name": "value",
+    "Value": 0
   }
 })";
   verifyAll(ExpectedOut, JSONExpectedOut, PrintFunc);
 }
 
 TEST_F(ScopedPrinterTest, PrintBoolean) {
   auto PrintFunc = [](ScopedPrinter &W) {
     W.printBoolean("True", true);
TEST_F(ScopedPrinterTest, PrintHex) {
 
   const char *ExpectedOut = R"(HexNumber: 0x10
 HexLabel: Name (0x10)
 )";
 
   const char *JSONExpectedOut = R"({
   "HexNumber": 16,
   "HexLabel": {
-    "Value": "Name",
-    "RawValue": 16
+    "Name": "Name",
+    "Value": 16
   }
 })";
   verifyAll(ExpectedOut, JSONExpectedOut, PrintFunc);
 }
 
 TEST_F(ScopedPrinterTest, PrintHexList) {
   auto PrintFunc = [](ScopedPrinter &W) {
     const uint64_t HexList[] = {0x1, 0x10, 0x100};