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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
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
----------------
jhenderson wrote:
> into the two fields -> into two fields
> 
> The last -> The former
> The last -> The former

I think you meant "-> The latter", as I really meant that "Other" usually holds no 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.
----------------
jhenderson wrote:
> 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.
>> I do wonder whether it should be an error if StOther is specified at the same time as either Visibility or Other.

It is reasonable, but not that easy it seems.

Currently, if for example, you have both `StOther` and `Other`:
```
- Name:    bar
    StOther: 0xff
    Other: [ STO_MIPS_OPTIONAL ]
```

then yaml2obj reports an "unknown key" error:

```
YAML:72:14: error: unknown key 'Other'
    Other: [ STO_MIPS_OPTIONAL ]
```

(because `mapOptional` is never called for `Other`/`Visibility` in this case).

It is sure not ideal. At first look the right way seems to be to check it in `MappingTraits<ELFYAML::Symbol>::validate` below
somehow. Perhaps we might need to store both `Other` and `StOther` as `Optional<>` for that.
Or, as an alternative perhaps we could change `MappingNormalization`/`NormalizedOther` and report a correct
error right here. Seems it is not the best place for error reporting though.

Since we do not allow specifying these options at the same time currently,
what about if I'll investigate more how to improve the error we report and suggest a follow up patch?


================
Comment at: test/tools/yaml2obj/elf-symbol-stother.yaml:51
+# USE-STOTHER:      Other [ (0x4)
+# MIPS-NEXT:          STO_MIPS_OPTIONAL (0x4)
+# USE-STOTHER-NEXT: ]
----------------
jhenderson wrote:
> 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".
>> 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".

Yeah, I thougt about that a bit when wrote, but supposed there is no risk, since we have only two symbols here.
Though I see no problems with making the test a bit stricter. I've done it.


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

https://reviews.llvm.org/D66583