[PATCH] D66583: [yaml2obj] - Allow setting the symbol st_other field to any integer.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: lib/ObjectYAML/ELFYAML.cpp:893
+  // Symbol's Other field is a bit special. It is a bit field that represents
+  // st_other and usually helds symbol visibility. When we write a YAML
+  // we split it into the two fields named "Visibility" and "Other". The last
----------------
helds -> holds

YAML -> YAML document


================
Comment at: lib/ObjectYAML/ELFYAML.cpp:894
+  // st_other and usually helds symbol visibility. When we write a YAML
+  // we split it into the two fields named "Visibility" and "Other". The last
+  // one usually holds no value, and so almost never printed. Though some
----------------
into the two fields -> into two fields

The last -> The former


================
Comment at: lib/ObjectYAML/ELFYAML.cpp:895
+  // we split it into the two fields named "Visibility" and "Other". The last
+  // one usually holds no value, and so almost never printed. Though some
+  // targets (e.g. MIPS) may use it to specify the named bits to set (e.g.
----------------
"and so almost never printed. Though some targets..." -> "and so is almost never printed, although some targets..."


================
Comment at: lib/ObjectYAML/ELFYAML.cpp:897
+  // targets (e.g. MIPS) may use it to specify the named bits to set (e.g.
+  // STO_MIPS_OPTIONAL). For producing the broken objects we want to allow
+  // writing any values to st_other. To do this we allow one more field
----------------
producing the broken -> producing broken


================
Comment at: lib/ObjectYAML/ELFYAML.cpp:898
+  // STO_MIPS_OPTIONAL). For producing the broken objects we want to allow
+  // writing any values to st_other. To do this we allow one more field
+  // called "StOther". If it is present in a YAML, we set st_other as an
----------------
values -> value


================
Comment at: lib/ObjectYAML/ELFYAML.cpp:899
+  // writing any values to st_other. To do this we allow one more field
+  // called "StOther". If it is present in a YAML, we set st_other as an
+  // integer, ignoring the fact it is actually a bit field.
----------------
YAML -> YAML document

I'd rephrase the rest of the sentence as follows: "we set st_other to that integer, ignoring the other fields."

I do wonder whether it should be an error if StOther is specified at the same time as either Visibility or Other.


================
Comment at: test/tools/yaml2obj/elf-symbol-stother.yaml:1
+## Test how yaml2obj sets the values to symbol's st_other fields.
+
----------------
"sets the value of a symbol's..."


================
Comment at: test/tools/yaml2obj/elf-symbol-stother.yaml:3
+
+## Show that yaml2obj report an error when STO_* flag that belongs to a different
+## machine type is used in YAML.
----------------
report -> reports
when -> when using an


================
Comment at: test/tools/yaml2obj/elf-symbol-stother.yaml:4
+## Show that yaml2obj report an error when STO_* flag that belongs to a different
+## machine type is used in YAML.
+
----------------
is used in -> to what is specified by the


================
Comment at: test/tools/yaml2obj/elf-symbol-stother.yaml:8
+# ERR:      error: unknown bit value
+# ERR-NEXT:  Other: [ STO_MIPS_OPTIONAL ]
+
----------------
Nit: the lining up here looks a bit messy. I think you can just delete a single space before "Other:"


================
Comment at: test/tools/yaml2obj/elf-symbol-stother.yaml:20
+
+## Test there is no problem to use STO_* flag when a machine type is correct.
+## We use the same YAML as above, but with a change of machine type.
----------------
I'd change this first line to "Test that STO_* can be used with their correct machine type."


================
Comment at: test/tools/yaml2obj/elf-symbol-stother.yaml:41
+
+## Test that instead of using the "Other" field we can use "StOther" field to
+## set any arbitrary value to st_other.
----------------
we can use "StOther" -> we can use the "StOther"

to set any arbitrary value to -> to set st_other to any arbitrary value


================
Comment at: test/tools/yaml2obj/elf-symbol-stother.yaml:51
+# USE-STOTHER:      Other [ (0x4)
+# MIPS-NEXT:          STO_MIPS_OPTIONAL (0x4)
+# USE-STOTHER-NEXT: ]
----------------
Testing the exact printing here seems somewhat irrelevant to yaml2obj, I think? I think all that's important is that the Other value is correct (and that we don't error for MIPS values/unknown values etc as you already do).

In other words, I think you only need to test "Other [ (0x4)" and "Other [ (0xFF)" with their symbol names.

By the way, do you think there's a risk that the wrong symbol's Other value will be checked here? The check will match the first "Other [ (0x4)" line, not necessarily the Other line that goes with "foo".


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

https://reviews.llvm.org/D66583