[PATCH] D66886: [yaml2obj][obj2yaml] - Use a single "Other" field instead of "Other", "Visibility" and "StOther".

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: lib/ObjectYAML/ELFYAML.cpp:850
+      uint8_t FlagValue = P.second;
+      if ((*Original & FlagValue) != FlagValue)
+        continue;
----------------
I'm a bit confused with seeing an Optional being passed in without any check to see if it is None... but I see that was the case before.


================
Comment at: lib/ObjectYAML/ELFYAML.cpp:857
+    if (*Original != 0) {
+      UnknownFlagsHolder = std::to_string(*Original);
+      Ret.push_back(UnknownFlagsHolder);
----------------
llvm::to_string I think is preferred.


================
Comment at: lib/ObjectYAML/ELFYAML.cpp:877-880
+    llvm::WithColor::error()
+        << "an unknown value is used for symbol's 'Other' field: " << Name
+        << ".\n";
+    exit(1);
----------------
Do other yaml2obj/obj2yaml error messages end with a full stop?

Also, this error reporting method here makes me uncomfortable, since we're in the middle of a library. Is there any sensible way to propagate the error back up the stack?


================
Comment at: lib/ObjectYAML/ELFYAML.cpp:897-899
+    // STV_* values are just enumeration values. We add them in a reversed order
+    // because when we converting the st_other to named constants we want to use
+    // a maximum number of bits on each step.
----------------
converting -> convert

I'm not sure what's "reversed" about the order, or what the bits have to do with anything?


================
Comment at: lib/ObjectYAML/ELFYAML.cpp:910-911
+    // MIPS is not consistent. All of the STO_MIPS_* values are bit flags,
+    // except STO_MIPS_MIPS16 which overlaps them. It should be checked first,
+    // so we add it first.
+    if (EMachine == ELF::EM_MIPS) {
----------------
Why does MIPS16 need checking first? This comment should explain this point.


================
Comment at: lib/ObjectYAML/ELFYAML.cpp:939-943
   // Symbol's Other field is a bit special. It is a bit field that represents
-  // st_other and usually holds symbol visibility. When we write a YAML document
-  // we split it into two fields named "Visibility" and "Other". The latter one
-  // usually holds no value, and so is almost never printed, although some
-  // targets (e.g. MIPS) may use it to specify the named bits to set (e.g.
-  // STO_MIPS_OPTIONAL). For producing broken objects we want to allow writing
-  // any value to st_other. To do this we allow one more field called "StOther".
-  // If it is present in a YAML document, we set st_other to its integer value
-  // whatever it is.
-  // obj2yaml should not print 'StOther', it should print 'Visibility' and
-  // 'Other' fields instead.
-  assert(!IO.outputting() || !Symbol.StOther.hasValue());
-  IO.mapOptional("StOther", Symbol.StOther);
+  // st_other and usually holds symbol visibility. But also depending on a
+  // platform it can contain bitfields and regular values, or even sometimes a
+  // crazy mix of them (see comments for NormalizedOther). Because of that we
+  // need a special handling.
----------------
Calling st_other/Other a bit field is incorrect, even for regular ELFs, since symbol visibility isn't a bit field (STV_PROTECTED is 0x3 for example).

I'd change the first sentence to say "It is usually a field that represents st_other and holds the symbol visibility."

Then I'd make the next sentence say "However, on some platforms, it can contain bit field and regular values, or even ..."

And the last sentence "Because of this, we need special handling."


================
Comment at: test/tools/obj2yaml/elf-symbol-visibility.yaml:14
+# CHECK-NEXT:   - Name: internal
+# CHECK-NEXT:     Other:
+# CHECK-NEXT:      - STV_INTERNAL
----------------
Is there any way straightforward way of forcing obj2yaml to produce output in the form `Other: [ STV_HIDDEN ]` etc?


================
Comment at: test/tools/yaml2obj/elf-symbol-stother.yaml:72
 
-# ERR2: error: StOther cannot be specified for Symbol with either Visibility or Other
+## The same, but the different syntax is used to describe "Other".
 
----------------
but the -> but a

Does this alternative syntax just come out automatically? Otherwise, I don't think we need to support both forms.


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

https://reviews.llvm.org/D66886