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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: lib/ObjectYAML/ELFYAML.cpp:850
+      uint8_t FlagValue = P.second;
+      if ((*Original & FlagValue) != FlagValue)
+        continue;
----------------
jhenderson wrote:
> 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.
Yes. This constructor is only used by `MappingNormalization` for outputing, i.e. for obj2yaml.
So we always have a value here. I wasn't sure if it worth to add a comment or not, since it is by design of `MappingNormalization` class.
I added an assert with a comment.


================
Comment at: lib/ObjectYAML/ELFYAML.cpp:857
+    if (*Original != 0) {
+      UnknownFlagsHolder = std::to_string(*Original);
+      Ret.push_back(UnknownFlagsHolder);
----------------
MaskRay wrote:
> jhenderson wrote:
> > llvm::to_string I think is preferred.
> `to_string` returns an allocated string => `Other`'s type can't be Optional<std::vector<StringRef>>
> 
> See below.
> llvm::to_string I think is preferred.

I can't use it because it wants me to include the 'ScopedPrinter.h'. I do not think it worth doing for a single place?

> to_string returns an allocated string => Other's type can't be Optional<std::vector<StringRef>>

That is why I had to use `UnknownFlagsHolder`, it holds the string allocated until 'NormalizedOther' is destroyed.




================
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);
----------------
MaskRay wrote:
> jhenderson wrote:
> > 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?
> In ELFYAML.h
> 
> ```
>   template <>
>   struct MappingTraits<ELFYAML::Symbol> {
>   ...
> +   std::string Err;
> ```
> 
> In ELFYAML.cpp,
> 
> ```
>   StringRef MappingTraits<ELFYAML::Symbol>::validate(IO &IO,
>                                                    ELFYAML::Symbol &Symbol) {
> +  if (!Err.empty())
> +    return Err;
> ...
>   }
> 
> -   NormalizedOther(IO &IO, Optional<uint8_t> Original) : YamlIO(IO) {
> +  NormalizedOther(IO &IO, ELFYAML::Symbol &Sym) : YamlIO(IO) {
> 
> -  MappingNormalization<NormalizedOther, Optional<uint8_t>> Keys(IO, Symbol.Other);
> +  MappingNormalization<NormalizedOther, ELFYAML::Symbol> Keys(IO, Symbol);
> ```
> 
> I haven't verified if this approach works, if it does, the error can be propagated back via `validate()`.
> Do other yaml2obj/obj2yaml error messages end with a full stop?

Yes.

> 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?

This is a common problem, `exit(1)` is already used in a 3 more places in lib\ObjectYAML. Having that, what about if I suggest a follow-up refactoring(s) for all those places and this one?


================
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.
----------------
jhenderson wrote:
> converting -> convert
> 
> I'm not sure what's "reversed" about the order, or what the bits have to do with anything?
`STV_*`s have the following values:

```
STV_DEFAULT = 0,  // Visibility is specified by binding type
STV_INTERNAL = 1, // Defined by processor supplements
STV_HIDDEN = 2,   // Not visible to other components
STV_PROTECTED = 3 // Visible in other components but not preemptable
```

If we have, for example `st_other` == 3. We want to dump it as `STV_PROTECTED` and not as `STV_INTERNAL` + `STV_HIDDEN`.
So we need to handle the `STV_PROTECTED` first and because of that I had to list them as `3, 2, 1`, i.e. in inversed order.

I updated the comment.


================
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) {
----------------
jhenderson wrote:
> Why does MIPS16 need checking first? This comment should explain this point.
I updated the comment.


================
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.
----------------
jhenderson wrote:
> 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."
Thanks!


================
Comment at: test/tools/obj2yaml/elf-symbol-visibility.yaml:14
+# CHECK-NEXT:   - Name: internal
+# CHECK-NEXT:     Other:
+# CHECK-NEXT:      - STV_INTERNAL
----------------
MaskRay wrote:
> jhenderson wrote:
> > grimar wrote:
> > > MaskRay wrote:
> > > > MaskRay wrote:
> > > > > jhenderson wrote:
> > > > > > Is there any way straightforward way of forcing obj2yaml to produce output in the form `Other: [ STV_HIDDEN ]` etc?
> > > > > Not straightforward but I managed to find an approach..
> > > > > 
> > > > > ```
> > > > >  struct NormalizedOther {
> > > > > +  struct Piece {
> > > > > +    std::string Str;
> > > > > +  };
> > > > >  ...
> > > > > -  Optional<std::vector<StringRef>> Other;
> > > > > +  Optional<std::vector<Piece>> Other;
> > > > >    std::string UnknownFlagsHolder;
> > > > >  };
> > > > >  } // namespace
> > > > >  
> > > > > +template<>
> > > > > +struct ScalarTraits<NormalizedOther::Piece> {
> > > > > +  static void output(const NormalizedOther::Piece &Val, void *, raw_ostream &Out) {
> > > > > +    Out << Val.Str;
> > > > > +  }
> > > > > +  static StringRef input(StringRef Scalar, void *, NormalizedOther::Piece &Val) {
> > > > > +    Val.Str = Scalar.str();
> > > > > +    return {};
> > > > > +  }
> > > > > +  static QuotingType mustQuote(StringRef) { return QuotingType::None; }
> > > > > +};
> > > > > +template<> struct SequenceElementTraits<NormalizedOther::Piece> {
> > > > > +  static const bool flow = true;
> > > > > +};
> > > > > ```
> > > > > 
> > > > > This way obj2yaml will dump: `Other: [ STV_HIDDEN ]`
> > > > Oh, I think StringRef should be fine.
> > > > 
> > > > ```
> > > > +  struct Piece {
> > > > +    StringRef Str;
> > > > +  };
> > > > ```
> > > Thanks a lot for this! I was wondering yesterday how to achieve it, but did not have chance to experiment with it.
> > > 
> > > My question is: do we want it? I.e it adds a bit of complexity to the code, but makes the output better.
> > > I really like it the `Other: [ STV_HIDDEN ]` output style more, just want to check this point before addressing.
> > > 
> > > (I'll try to address this and all other comments today.)
> > I'm happy either way. I think it's a little confusing that obj2yaml might produce different output to what looks good and would be written in all the tests, so I think the extra complexity is probably worth it. It's not too bad, I think?
> I also think the extra complexity is worth it.
I was able to get rid of using `struct` (used `LLVM_YAML_STRONG_TYPEDEF(StringRef, StOtherPiece)` declaration instead).
Seems it is a little bit simpler.


================
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".
 
----------------
jhenderson wrote:
> but the -> but a
> 
> Does this alternative syntax just come out automatically? Otherwise, I don't think we need to support both forms.
Yes it is supported automatically. I added it to show we can use the syntax produced by obj2yaml, which was different before this diff.
Since now they have a similar syntax, I removed this test case completely.


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

https://reviews.llvm.org/D66886