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

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

LGTM, with one remaining comment nit.



================
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
----------------
grimar wrote:
> 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.
Yes, sorry.


================
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.
----------------
grimar wrote:
> 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?
Doing it in the validate function seems right to me (we already do that for other fields). Happy for it to be a follow-up though, if it's easier.


================
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.
----------------
jhenderson wrote:
> report -> reports
> when -> when using an
You missed the "when using an" bit...


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

https://reviews.llvm.org/D66583