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

[grimar]

Summary

Currenly we can encode the 'st_other' field of symbol using 3 fields.
'Visibility' is used to encode STV_* values.
'Other' is used to encode everything except the visibility, but it can't handle arbitrary values.
'StOther' is used to encode arbitrary values when 'Visibility'/'Other' are not helpfull enough.

'st_other' field is used to encode symbol visibility and platform-dependent
flags and values. Problem to encode it is that it consists of Visibility part (STV_* values)
which are enumeration values and the Other part, which is different and inconsistent.

For MIPS the Other part contains flags for all STO_MIPS_* values except STO_MIPS_MIPS16.
(Like comment in ELFDumper says: "Someones in their infinite wisdom decided to make
STO_MIPS_MIPS16 flag overlapped with other ST_MIPS_xxx flags."...)

And for PPC64 the Other part might actually encode any value.

This patch implements custom logic for handling the st_other and removes
'Visibility' and 'StOther' fields.

Here is an example of a new YAML style this patch allows:

- Name:  foo
  Other: [ 0x4 ]
- Name:  bar
  Other: [ STV_PROTECTED, 4 ]
- Name:  zed
  Other: [ STV_PROTECTED, STO_MIPS_OPTIONAL, 0xf8 ]

[jhenderson]

lib/ObjectYAML/ELFYAML.cpp

850 ↗

(On Diff #217655)

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.

857 ↗

(On Diff #217655)

llvm::to_string I think is preferred.

877–880 ↗

(On Diff #217655)

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?

897–899 ↗

(On Diff #217655)

converting -> convert

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

910–911 ↗

(On Diff #217655)

Why does MIPS16 need checking first? This comment should explain this point.

939–943 ↗

(On Diff #217655)

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."

test/tools/obj2yaml/elf-symbol-visibility.yaml

14 ↗

(On Diff #217655)

Is there any way straightforward way of forcing obj2yaml to produce output in the form Other: [ STV_HIDDEN ] etc?

test/tools/yaml2obj/elf-symbol-stother.yaml

72 ↗

(On Diff #217655)

but the -> but a

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

[MaskRay]

lib/ObjectYAML/ELFYAML.cpp

877–880 ↗

(On Diff #217655)

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().

[MaskRay]

lib/ObjectYAML/ELFYAML.cpp

857 ↗

(On Diff #217655)

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

See below.

test/tools/obj2yaml/elf-symbol-visibility.yaml

14 ↗

(On Diff #217655)

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 ]

[MaskRay]

test/tools/obj2yaml/elf-symbol-visibility.yaml

14 ↗

(On Diff #217655)

Oh, I think StringRef should be fine.

+  struct Piece {
+    StringRef Str;
+  };

[grimar]

test/tools/obj2yaml/elf-symbol-visibility.yaml

14 ↗

(On Diff #217655)

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.)

[jhenderson]

test/tools/obj2yaml/elf-symbol-visibility.yaml

14 ↗

(On Diff #217655)

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?

[MaskRay]

test/tools/obj2yaml/elf-symbol-visibility.yaml

14 ↗

(On Diff #217655)

I also think the extra complexity is worth it.

[grimar]

• Addressed review comments.

lib/ObjectYAML/ELFYAML.cpp

850 ↗

(On Diff #217655)

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.

857 ↗

(On Diff #217655)

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.

877–880 ↗

(On Diff #217655)

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?

897–899 ↗

(On Diff #217655)

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.

910–911 ↗

(On Diff #217655)

I updated the comment.

939–943 ↗

(On Diff #217655)

Thanks!

test/tools/obj2yaml/elf-symbol-visibility.yaml

14 ↗

(On Diff #217655)

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.

test/tools/yaml2obj/elf-symbol-stother.yaml

72 ↗

(On Diff #217655)

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.

[MaskRay]

lib/ObjectYAML/ELFYAML.cpp

877–880 ↗

(On Diff #217655)

This is a common problem, exit(1) is already used in a 3 more places in lib\ObjectYAML.

Yes. I think obj2yaml functions have better error handling than yaml2obj function. It should be fine to fix the error propagating problem later.

962 ↗

(On Diff #217855)

bit field -> bit fields

[grimar]

• Addressed review comment.

[jhenderson]

lib/ObjectYAML/ELFYAML.cpp

861 ↗

(On Diff #218038)

outputting the YAML -> outputting YAML

862 ↗

(On Diff #218038)

assumes a

932 ↗

(On Diff #218038)

another -> any other
Delete the "So we add it first": you already talk about it being consumed first earlier.

[grimar]

• Addressed review comments.

[jhenderson]

LGTM.

llvm/trunk/include/llvm/ObjectYAML/ELFYAML.h

 LLVM_YAML_STRONG_TYPEDEF(uint32_t, ELF_SHT)
 LLVM_YAML_STRONG_TYPEDEF(uint32_t, ELF_REL)
 LLVM_YAML_STRONG_TYPEDEF(uint8_t, ELF_RSS)
 // Just use 64, since it can hold 32-bit values too.
 LLVM_YAML_STRONG_TYPEDEF(uint64_t, ELF_SHF)
 LLVM_YAML_STRONG_TYPEDEF(uint16_t, ELF_SHN)
 LLVM_YAML_STRONG_TYPEDEF(uint8_t, ELF_STB)
 LLVM_YAML_STRONG_TYPEDEF(uint8_t, ELF_STT)
-LLVM_YAML_STRONG_TYPEDEF(uint8_t, ELF_STV)
-LLVM_YAML_STRONG_TYPEDEF(uint8_t, ELF_STO)
 
 LLVM_YAML_STRONG_TYPEDEF(uint8_t, MIPS_AFL_REG)
 LLVM_YAML_STRONG_TYPEDEF(uint8_t, MIPS_ABI_FP)
 LLVM_YAML_STRONG_TYPEDEF(uint32_t, MIPS_AFL_EXT)
 LLVM_YAML_STRONG_TYPEDEF(uint32_t, MIPS_AFL_ASE)
 LLVM_YAML_STRONG_TYPEDEF(uint32_t, MIPS_AFL_FLAGS1)
 LLVM_YAML_STRONG_TYPEDEF(uint32_t, MIPS_ISA)
 
struct Symbol {
   Optional<uint32_t> NameIndex;
   ELF_STT Type;
   StringRef Section;
   Optional<ELF_SHN> Index;
   ELF_STB Binding;
   llvm::yaml::Hex64 Value;
   llvm::yaml::Hex64 Size;
   Optional<uint8_t> Other;
-
-  // This can be used to set any custom value for the st_other field
-  // when it is not possible to do so using the "Other" field, which only takes
-  // specific named constants.
-  Optional<uint8_t> StOther;
 };
 
 struct SectionOrType {
   StringRef sectionNameOrType;
 };
 
 struct DynamicEntry {
   ELF_DYNTAG Tag;

 };
 
 template <>
 struct ScalarEnumerationTraits<ELFYAML::ELF_STT> {
   static void enumeration(IO &IO, ELFYAML::ELF_STT &Value);
 };
 
 template <>
-struct ScalarEnumerationTraits<ELFYAML::ELF_STV> {
-  static void enumeration(IO &IO, ELFYAML::ELF_STV &Value);
-};
-
-template <>
-struct ScalarBitSetTraits<ELFYAML::ELF_STO> {
-  static void bitset(IO &IO, ELFYAML::ELF_STO &Value);
-};
-
-template <>
 struct ScalarEnumerationTraits<ELFYAML::ELF_REL> {
   static void enumeration(IO &IO, ELFYAML::ELF_REL &Value);
 };
 
 template <>
 struct ScalarEnumerationTraits<ELFYAML::ELF_DYNTAG> {
   static void enumeration(IO &IO, ELFYAML::ELF_DYNTAG &Value);
 };

llvm/trunk/include/llvm/Support/YAMLTraits.h

: public std::integral_constant<
                     !has_MappingValidateTraits<T, Context>::value> {};
 
 // Base class for Input and Output.
 class IO {
 public:
   IO(void *Ctxt = nullptr);
   virtual ~IO();
 
-  virtual bool outputting() = 0;
+  virtual bool outputting() const = 0;
 
   virtual unsigned beginSequence() = 0;
   virtual bool preflightElement(unsigned, void *&) = 0;
   virtual void postflightElement(void*) = 0;
   virtual void endSequence() = 0;
   virtual bool canElideEmptySequence() = 0;
 
   virtual unsigned beginFlowSequence() = 0;
public:
 
   template <typename T>
   void maskedBitSetCase(T &Val, const char *Str, uint32_t ConstVal,
                         uint32_t Mask) {
     if (bitSetMatch(Str, outputting() && (Val & Mask) == ConstVal))
       Val = Val | ConstVal;
   }
 
-  void *getContext();
+  void *getContext() const;
   void setContext(void *);
 
   template <typename T> void mapRequired(const char *Key, T &Val) {
     EmptyContext Ctx;
     this->processKey(Key, Val, true, Ctx);
   }
 
   template <typename T, typename Context>
Input(MemoryBufferRef Input,
         SourceMgr::DiagHandlerTy DiagHandler = nullptr,
         void *DiagHandlerCtxt = nullptr);
   ~Input() override;
 
   // Check if there was an syntax or semantic error during parsing.
   std::error_code error();
 
 private:
-  bool outputting() override;
+  bool outputting() const override;
   bool mapTag(StringRef, bool) override;
   void beginMapping() override;
   void endMapping() override;
   bool preflightKey(const char *, bool, bool, bool &, void *&) override;
   void postflightKey(void *) override;
   std::vector<StringRef> keys() override;
   void beginFlowMapping() override;
   void endFlowMapping() override;
public:
 
   /// Set whether or not to output optional values which are equal
   /// to the default value.  By default, when outputting if you attempt
   /// to write a value that is equal to the default, the value gets ignored.
   /// Sometimes, it is useful to be able to see these in the resulting YAML
   /// anyway.
   void setWriteDefaultValues(bool Write) { WriteDefaultValues = Write; }
 
-  bool outputting() override;
+  bool outputting() const override;
   bool mapTag(StringRef, bool) override;
   void beginMapping() override;
   void endMapping() override;
   bool preflightKey(const char *key, bool, bool, bool &, void *&) override;
   void postflightKey(void *) override;
   std::vector<StringRef> keys() override;
   void beginFlowMapping() override;
   void endFlowMapping() override;

llvm/trunk/lib/ObjectYAML/ELFEmitter.cpp

if (!Sym.Section.empty()) {
         exit(1);
       }
       Symbol.st_shndx = Index;
     } else if (Sym.Index) {
       Symbol.st_shndx = *Sym.Index;
     }
     // else Symbol.st_shndex == SHN_UNDEF (== 0), since it was zero'd earlier.
     Symbol.st_value = Sym.Value;
-
-    if (Sym.Other)
-      Symbol.st_other = *Sym.Other;
-    else if (Sym.StOther)
-      Symbol.st_other = *Sym.StOther;
-
+    Symbol.st_other = Sym.Other ? *Sym.Other : 0;
     Symbol.st_size = Sym.Size;
   }
 
   return Ret;
 }
 
 template <class ELFT>
 void ELFState<ELFT>::initSymtabSectionHeader(Elf_Shdr &SHeader,

llvm/trunk/lib/ObjectYAML/ELFYAML.cpp

 //===- ELFYAML.cpp - ELF YAMLIO implementation ----------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file defines classes for handling the YAML representation of ELF.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ObjectYAML/ELFYAML.h"
+#include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/BinaryFormat/ELF.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/MipsABIFlags.h"
 #include "llvm/Support/YAMLTraits.h"
+#include "llvm/Support/WithColor.h"
 #include <cassert>
 #include <cstdint>
 
 namespace llvm {
 
 ELFYAML::Section::~Section() = default;
 
 namespace yaml {
#define ECase(X) IO.enumCase(Value, #X, ELF::X)
   ECase(STT_FILE);
   ECase(STT_COMMON);
   ECase(STT_TLS);
   ECase(STT_GNU_IFUNC);
 #undef ECase
   IO.enumFallback<Hex8>(Value);
 }
 
-void ScalarEnumerationTraits<ELFYAML::ELF_STV>::enumeration(
-    IO &IO, ELFYAML::ELF_STV &Value) {
-#define ECase(X) IO.enumCase(Value, #X, ELF::X)
-  ECase(STV_DEFAULT);
-  ECase(STV_INTERNAL);
-  ECase(STV_HIDDEN);
-  ECase(STV_PROTECTED);
-#undef ECase
-}
-
-void ScalarBitSetTraits<ELFYAML::ELF_STO>::bitset(IO &IO,
-                                                  ELFYAML::ELF_STO &Value) {
-  const auto *Object = static_cast<ELFYAML::Object *>(IO.getContext());
-  assert(Object && "The IO context is not initialized");
-#define BCase(X) IO.bitSetCase(Value, #X, ELF::X)
-  switch (Object->Header.Machine) {
-  case ELF::EM_MIPS:
-    BCase(STO_MIPS_OPTIONAL);
-    BCase(STO_MIPS_PLT);
-    BCase(STO_MIPS_PIC);
-    BCase(STO_MIPS_MICROMIPS);
-    break;
-  default:
-    break; // Nothing to do
-  }
-#undef BCase
-#undef BCaseMask
-}
 
 void ScalarEnumerationTraits<ELFYAML::ELF_RSS>::enumeration(
     IO &IO, ELFYAML::ELF_RSS &Value) {
 #define ECase(X) IO.enumCase(Value, #X, ELF::X)
   ECase(RSS_UNDEF);
   ECase(RSS_GP);
   ECase(RSS_GP0);
   ECase(RSS_LOC);
void MappingTraits<ELFYAML::ProgramHeader>::mapping(
   IO.mapOptional("VAddr", Phdr.VAddr, Hex64(0));
   IO.mapOptional("PAddr", Phdr.PAddr, Hex64(0));
   IO.mapOptional("Align", Phdr.Align);
   IO.mapOptional("FileSize", Phdr.FileSize);
   IO.mapOptional("MemSize", Phdr.MemSize);
   IO.mapOptional("Offset", Phdr.Offset);
 }
 
+LLVM_YAML_STRONG_TYPEDEF(StringRef, StOtherPiece)
+
+template <> struct ScalarTraits<StOtherPiece> {
+  static void output(const StOtherPiece &Val, void *, raw_ostream &Out) {
+    Out << Val;
+  }
+  static StringRef input(StringRef Scalar, void *, StOtherPiece &Val) {
+    Val = Scalar;
+    return {};
+  }
+  static QuotingType mustQuote(StringRef) { return QuotingType::None; }
+};
+template <> struct SequenceElementTraits<StOtherPiece> {
+  static const bool flow = true;
+};
+
 namespace {
 
 struct NormalizedOther {
-  NormalizedOther(IO &) {}
-  NormalizedOther(IO &, Optional<uint8_t> Original) {
-    if (uint8_t Val = *Original & 0x3)
-      Visibility = Val;
-    if (uint8_t Val = *Original & ~0x3)
-      Other = Val;
+  NormalizedOther(IO &IO) : YamlIO(IO) {}
+  NormalizedOther(IO &IO, Optional<uint8_t> Original) : YamlIO(IO) {
+    assert(Original && "This constructor is only used for outputting YAML and "
+                       "assumes a non-empty Original");
+    std::vector<StOtherPiece> Ret;
+    const auto *Object = static_cast<ELFYAML::Object *>(YamlIO.getContext());
+    for (std::pair<StringRef, uint8_t> &P :
+         getFlags(Object->Header.Machine).takeVector()) {
+      uint8_t FlagValue = P.second;
+      if ((*Original & FlagValue) != FlagValue)
+        continue;
+      *Original &= ~FlagValue;
+      Ret.push_back({P.first});
+    }
+
+    if (*Original != 0) {
+      UnknownFlagsHolder = std::to_string(*Original);
+      Ret.push_back({UnknownFlagsHolder});
+    }
+
+    if (!Ret.empty())
+      Other = std::move(Ret);
+  }
+
+  uint8_t toValue(StringRef Name) {
+    const auto *Object = static_cast<ELFYAML::Object *>(YamlIO.getContext());
+    MapVector<StringRef, uint8_t> Flags = getFlags(Object->Header.Machine);
+
+    auto It = Flags.find(Name);
+    if (It != Flags.end())
+      return It->second;
+
+    uint8_t Val;
+    if (to_integer(Name, Val))
+      return Val;
+
+    llvm::WithColor::error()
+        << "an unknown value is used for symbol's 'Other' field: " << Name
+        << ".\n";
+    exit(1);
   }
 
   Optional<uint8_t> denormalize(IO &) {
-    if (!Visibility && !Other)
+    if (!Other)
       return None;
-
     uint8_t Ret = 0;
-    if (Visibility)
-      Ret |= *Visibility;
-    if (Other)
-      Ret |= *Other;
+    for (StOtherPiece &Val : *Other)
+      Ret |= toValue(Val);
     return Ret;
   }
 
-  Optional<ELFYAML::ELF_STV> Visibility;
-  Optional<ELFYAML::ELF_STO> Other;
+  // st_other field is used to encode symbol visibility and platform-dependent
+  // flags and values. This method returns a name to value map that is used for
+  // parsing and encoding this field.
+  MapVector<StringRef, uint8_t> getFlags(unsigned EMachine) {
+    MapVector<StringRef, uint8_t> Map;
+    // STV_* values are just enumeration values. We add them in a reversed order
+    // because when we convert the st_other to named constants when printing
+    // YAML we want to use a maximum number of bits on each step:
+    // when we have st_other == 3, we want to print it as STV_PROTECTED (3), but
+    // not as STV_HIDDEN (2) + STV_INTERNAL (1).
+    Map["STV_PROTECTED"] = ELF::STV_PROTECTED;
+    Map["STV_HIDDEN"] = ELF::STV_HIDDEN;
+    Map["STV_INTERNAL"] = ELF::STV_INTERNAL;
+    // STV_DEFAULT is used to represent the default visibility and has a value
+    // 0. We want to be able to read it from YAML documents, but there is no
+    // reason to print it.
+    if (!YamlIO.outputting())
+      Map["STV_DEFAULT"] = ELF::STV_DEFAULT;
+
+    // MIPS is not consistent. All of the STO_MIPS_* values are bit flags,
+    // except STO_MIPS_MIPS16 which overlaps them. It should be checked and
+    // consumed first when we print the output, because we do not want to print
+    // any other flags that have the same bits instead.
+    if (EMachine == ELF::EM_MIPS) {
+      Map["STO_MIPS_MIPS16"] = ELF::STO_MIPS_MIPS16;
+      Map["STO_MIPS_MICROMIPS"] = ELF::STO_MIPS_MICROMIPS;
+      Map["STO_MIPS_PIC"] = ELF::STO_MIPS_PIC;
+      Map["STO_MIPS_PLT"] = ELF::STO_MIPS_PLT;
+      Map["STO_MIPS_OPTIONAL"] = ELF::STO_MIPS_OPTIONAL;
+    }
+    return Map;
+  }
+
+  const IO &YamlIO;
+  Optional<std::vector<StOtherPiece>> Other;
+  std::string UnknownFlagsHolder;
 };
 
 } // end anonymous namespace
 
 void MappingTraits<ELFYAML::Symbol>::mapping(IO &IO, ELFYAML::Symbol &Symbol) {
   IO.mapOptional("Name", Symbol.Name, StringRef());
   IO.mapOptional("NameIndex", Symbol.NameIndex);
   IO.mapOptional("Type", Symbol.Type, ELFYAML::ELF_STT(0));
   IO.mapOptional("Section", Symbol.Section, StringRef());
   IO.mapOptional("Index", Symbol.Index);
   IO.mapOptional("Binding", Symbol.Binding, ELFYAML::ELF_STB(0));
   IO.mapOptional("Value", Symbol.Value, Hex64(0));
   IO.mapOptional("Size", Symbol.Size, Hex64(0));
 
-  // 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);
+  // Symbol's Other field is a bit special. It is usually a field that
+  // represents st_other and holds the symbol visibility. However, on some
+  // platforms, it can contain bit fields and regular values, or even sometimes a
+  // crazy mix of them (see comments for NormalizedOther). Because of this, we
+  // need special handling.
   MappingNormalization<NormalizedOther, Optional<uint8_t>> Keys(IO,
                                                                 Symbol.Other);
-  IO.mapOptional("Visibility", Keys->Visibility);
   IO.mapOptional("Other", Keys->Other);
 }
 
 StringRef MappingTraits<ELFYAML::Symbol>::validate(IO &IO,
                                                    ELFYAML::Symbol &Symbol) {
   if (Symbol.Index && Symbol.Section.data())
     return "Index and Section cannot both be specified for Symbol";
   if (Symbol.NameIndex && !Symbol.Name.empty())
     return "Name and NameIndex cannot both be specified for Symbol";
-  if (Symbol.StOther && Symbol.Other)
-    return "StOther cannot be specified for Symbol with either Visibility or Other";
   return StringRef();
 }
 
 static void commonSectionMapping(IO &IO, ELFYAML::Section &Section) {
   IO.mapOptional("Name", Section.Name, StringRef());
   IO.mapRequired("Type", Section.Type);
   IO.mapOptional("Flags", Section.Flags);
   IO.mapOptional("Address", Section.Address, Hex64(0));

llvm/trunk/lib/Support/YAMLTraits.cpp

 //===----------------------------------------------------------------------===//
 //  IO
 //===----------------------------------------------------------------------===//
 
 IO::IO(void *Context) : Ctxt(Context) {}
 
 IO::~IO() = default;
 
-void *IO::getContext() {
+void *IO::getContext() const {
   return Ctxt;
 }
 
 void IO::setContext(void *Context) {
   Ctxt = Context;
 }
 
 //===----------------------------------------------------------------------===//

 
 // Pin the vtables to this file.
 void Input::HNode::anchor() {}
 void Input::EmptyHNode::anchor() {}
 void Input::ScalarHNode::anchor() {}
 void Input::MapHNode::anchor() {}
 void Input::SequenceHNode::anchor() {}
 
-bool Input::outputting() {
+bool Input::outputting() const {
   return false;
 }
 
 bool Input::setCurrentDocument() {
   if (DocIterator != Strm->end()) {
     Node *N = DocIterator->getRoot();
     if (!N) {
       assert(Strm->failed() && "Root is NULL iff parsing failed");

 //  Output
 //===----------------------------------------------------------------------===//
 
 Output::Output(raw_ostream &yout, void *context, int WrapColumn)
     : IO(context), Out(yout), WrapColumn(WrapColumn) {}
 
 Output::~Output() = default;
 
-bool Output::outputting() {
+bool Output::outputting() const {
   return true;
 }
 
 void Output::beginMapping() {
   StateStack.push_back(inMapFirstKey);
   PaddingBeforeContainer = Padding;
   Padding = "\n";
 }

llvm/trunk/test/tools/llvm-objcopy/ELF/localize-hidden.test

Relocations:
         Symbol: undefGlobal
         Type:   R_X86_64_PC32
 Symbols:
   - Name:     hiddenLocal
     Type:     STT_FUNC
     Section:  .text
     Value:    0x1008
     Size:     8
-    Visibility: STV_HIDDEN
+    Other:    [ STV_HIDDEN ]
   - Name:     defaultGlobal
     Type:     STT_FUNC
     Size:     8
     Section:  .text
     Value:    0x1000
     Binding:  STB_GLOBAL
   - Name:     hiddenGlobal
     Type:     STT_OBJECT
     Section:  .data
     Value:    0x2006
     Size:     2
-    Visibility: STV_HIDDEN
+    Other:    [ STV_HIDDEN ]
     Binding:  STB_GLOBAL
   - Name:     hiddenGlobalCommon
     Type:     STT_OBJECT
     Index:    SHN_COMMON
     Value:    0x2006
     Size:     2
-    Visibility: STV_HIDDEN
+    Other:    [ STV_HIDDEN ]
     Binding:  STB_GLOBAL
   - Name:     undefGlobal
     Type:     STT_FUNC
     Size:     8
     Binding:  STB_GLOBAL
   - Name:     internalGlobal
     Type:     STT_OBJECT
     Section:  .data
     Value:    0x2002
     Size:     2
-    Visibility: STV_INTERNAL
+    Other:    [ STV_INTERNAL ]
     Binding:  STB_GLOBAL
   - Name:     protectedGlobal
     Type:     STT_OBJECT
     Section:  .data
     Value:    0x2000
     Size:     4
-    Visibility: STV_PROTECTED
+    Other:    [ STV_PROTECTED ]
     Binding:  STB_GLOBAL
   - Name:     hiddenWeak
     Type:     STT_FUNC
     Section:  .text
     Value:    0x1010
     Size:     8
-    Visibility: STV_HIDDEN
+    Other:    [ STV_HIDDEN ]
     Binding:  STB_WEAK
 
 #CHECK:     Relocations [
 #CHECK-NEXT:  Section (3) .rel.text {
 #CHECK-NEXT:    0x1000 R_X86_64_PC32 undefGlobal 0x0
 #CHECK-NEXT:  }
 #CHECK-NEXT:]
 

llvm/trunk/test/tools/llvm-objcopy/ELF/symbol-copy.test

- Name:     _start
     Value:    0x1000
     Size:     4
     Binding:  STB_GLOBAL
   - Name:     bam
     Type:     STT_FUNC
     Section:  .text
     Value:    0x1001
     Size:     4
-    Visibility: STV_HIDDEN
+    Other:    [ STV_HIDDEN ]
     Binding:  STB_GLOBAL
   - Name:     foo
     Type:     STT_FUNC
     Section:  .text
     Value:    0x1004
     Binding:  STB_GLOBAL
   - Name:     faz
     Type:     STT_OBJECT
     Section:  .data
     Value:    0x2002
     Size:     2
-    Visibility: STV_INTERNAL
+    Other:    [ STV_INTERNAL ]
     Binding:  STB_GLOBAL
   - Name:     bar
     Type:     STT_OBJECT
     Section:  .data
     Value:    0x2000
     Size:     4
     Binding:  STB_GLOBAL
   - Name:     baz

llvm/trunk/test/tools/llvm-objdump/elf-symbol-visibility.test

 Sections:
   - Name: .text
     Type: SHT_PROGBITS
     Flags: [ SHF_ALLOC, SHF_EXECINSTR ]
 Symbols:
   - Name:    default
     Section: .text
   - Name:    internal
-    Visibility: STV_INTERNAL
+    Other:   [ STV_INTERNAL ]
     Section: .text
   - Name:    hidden
-    Visibility: STV_HIDDEN
+    Other:   [ STV_HIDDEN ]
     Section: .text
   - Name:    protected
-    Visibility: STV_PROTECTED
+    Other:   [ STV_PROTECTED ]
     Section: .text
   - Name:    mips_pic
     Other:   [ STO_MIPS_PIC ]
     Section: .text

llvm/trunk/test/tools/llvm-readobj/elf-symbol-visibility.test

 !ELF
 FileHeader:
   Class:   ELFCLASS32
   Data:    ELFDATA2LSB
   Type:    ET_REL
   Machine: EM_386
 Symbols:
   - Name: default
-    Visibility: STV_DEFAULT
+    Other: [ STV_DEFAULT ]
     Binding: STB_GLOBAL
   - Name: internal
-    Visibility: STV_INTERNAL
+    Other: [ STV_INTERNAL ]
     Binding: STB_GLOBAL
   - Name: hidden
-    Visibility: STV_HIDDEN
+    Other: [ STV_HIDDEN ]
     Binding: STB_GLOBAL
   - Name: protected
-    Visibility: STV_PROTECTED
+    Other: [ STV_PROTECTED ]
     Binding: STB_GLOBAL
   - Name: other
     Binding: STB_GLOBAL
-    StOther: 4
+    Other: [ 4 ]

llvm/trunk/test/tools/obj2yaml/elf-symbol-visibility.yaml

 ## Check obj2yaml is able to dump the Visibility field correctly.
 
 # RUN: yaml2obj %s | obj2yaml - | FileCheck %s
 
 # CHECK:      --- !ELF
 # CHECK-NEXT: FileHeader:
 # CHECK-NEXT:   Class:   ELFCLASS64
 # CHECK-NEXT:   Data:    ELFDATA2LSB
 # CHECK-NEXT:   Type:    ET_REL
 # CHECK-NEXT:   Machine: EM_X86_64
 # CHECK-NEXT: Symbols:
 # CHECK-NEXT:   - Name: default
-# CHECK-NEXT:   - Name:       internal
-# CHECK-NEXT:     Visibility: STV_INTERNAL
-# CHECK-NEXT:   - Name:       hidden
-# CHECK-NEXT:     Visibility: STV_HIDDEN
-# CHECK-NEXT:   - Name:       protected
-# CHECK-NEXT:     Visibility: STV_PROTECTED
+# CHECK-NEXT:   - Name: internal
+# CHECK-NEXT:     Other: [ STV_INTERNAL ]
+# CHECK-NEXT:   - Name: hidden
+# CHECK-NEXT:     Other: [ STV_HIDDEN ]
+# CHECK-NEXT:   - Name: protected
+# CHECK-NEXT:     Other: [ STV_PROTECTED ]
 # CHECK-NEXT: ...
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_REL
   Machine: EM_X86_64
 Symbols:
-  - Name:       default
-    Visibility: STV_DEFAULT
-  - Name:       internal
-    Visibility: STV_INTERNAL
-  - Name:       hidden
-    Visibility: STV_HIDDEN
-  - Name:       protected
-    Visibility: STV_PROTECTED
+  - Name:  default
+    Other: [ STV_DEFAULT ]
+  - Name:  internal
+    Other: [ STV_INTERNAL ]
+  - Name:  hidden
+    Other: [ STV_HIDDEN ]
+  - Name:  protected
+    Other: [ STV_PROTECTED ]

llvm/trunk/test/tools/yaml2obj/elf-symbol-stother.yaml

 ## Test how yaml2obj sets the value of a symbol's st_other fields.
 
 ## Show that yaml2obj reports an error when using an STO_* flag that belongs
 ## to a different machine type to what is specified by the YAML.
 
 # RUN: not yaml2obj --docnum=1 2>&1 %s | FileCheck %s --check-prefix=ERR
-# ERR:      error: unknown bit value
-# ERR-NEXT: Other: [ STO_MIPS_OPTIONAL ]
+# ERR: error: an unknown value is used for symbol's 'Other' field: STO_MIPS_OPTIONAL.
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS32
   Data:    ELFDATA2LSB
   Type:    ET_REL
   Machine: EM_386
 Symbols:
FileHeader:
   Class:   ELFCLASS32
   Data:    ELFDATA2LSB
   Type:    ET_REL
   Machine: EM_MIPS
 Symbols:
   - Name:  foo
     Other: [ STO_MIPS_OPTIONAL ]
 
-## Test that instead of using the "Other" field we can use the "StOther" field
-## to set st_other to any arbitrary value.
+## Test that we can mix named and unnamed constants and set
+## st_other to any arbitrary value.
 
 # RUN: yaml2obj --docnum=3 %s > %t3
-# RUN: llvm-readobj --symbols %t3 | FileCheck %s --check-prefix=USE-STOTHER
-# RUN: yaml2obj --docnum=4 %s > %t4
-# RUN: llvm-readobj --symbols %t4 | FileCheck %s --check-prefix=USE-STOTHER
-
-# USE-STOTHER:      Name: foo
-# USE-STOTHER:      Other [
-# USE-STOTHER-SAME: (0x4)
-
-# USE-STOTHER:      Name: bar
-# USE-STOTHER:      Other [
-# USE-STOTHER-SAME: (0xFF)
+# RUN: llvm-readobj --symbols %t3 | FileCheck %s --check-prefix=VALUE
 
---- !ELF
-FileHeader:
-  Class:   ELFCLASS32
-  Data:    ELFDATA2LSB
-  Type:    ET_REL
-  Machine: EM_MIPS
-Symbols:
-  - Name:    foo
-    StOther: 4
-  - Name:    bar
-    StOther: 0xff
-
---- !ELF
-FileHeader:
-  Class:   ELFCLASS32
-  Data:    ELFDATA2LSB
-  Type:    ET_REL
-  Machine: EM_386
-Symbols:
-  - Name:    foo
-    StOther: 4
-  - Name:    bar
-    StOther: 0xff
-
-## Check we can't set StOther for a symbol if Visibility or Other is also specified.
-
-# RUN: not yaml2obj --docnum=5 2>&1 %s | FileCheck %s --check-prefix=ERR2
-# RUN: not yaml2obj --docnum=6 2>&1 %s | FileCheck %s --check-prefix=ERR2
-
-# ERR2: error: StOther cannot be specified for Symbol with either Visibility or Other
-
---- !ELF
-FileHeader:
-  Class:   ELFCLASS32
-  Data:    ELFDATA2LSB
-  Type:    ET_REL
-  Machine: EM_MIPS
-Symbols:
-  - Name:    foo
-    StOther: 0
-    Other: [ STO_MIPS_OPTIONAL ]
+# VALUE:      Name: foo
+# VALUE:      Other [
+# VALUE-SAME: (0x4)
+
+# VALUE:      Name: bar
+# VALUE:      Other [
+# VALUE-SAME: (0x7)
+
+# VALUE:      Name: zed
+# VALUE:      Other [
+# VALUE-SAME: (0xFF)
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS32
   Data:    ELFDATA2LSB
   Type:    ET_REL
   Machine: EM_MIPS
 Symbols:
-  - Name:    foo
-    StOther: 0
-    Visibility: STV_DEFAULT
+  - Name:  foo
+    Other: [ 0x4 ]
+  - Name:  bar
+    Other: [ STV_PROTECTED, 4 ]
+  - Name:  zed
+    Other: [ STV_PROTECTED, STO_MIPS_OPTIONAL, 0xf8 ]

llvm/trunk/test/tools/yaml2obj/elf-symbol-visibility.yaml

 ## Check yaml2obj is able to parse the Visibility field and produce the output.
 
 # RUN: yaml2obj %s | llvm-readobj --symbols - | FileCheck --check-prefix OBJ %s
 
 # OBJ:      Symbol {
-# OBJ:        Name: default (1)
+# OBJ:        Name: default1
 # OBJ-NEXT:   Value: 0x0
 # OBJ-NEXT:   Size: 0
 # OBJ-NEXT:   Binding: Local (0x0)
 # OBJ-NEXT:   Type: None (0x0)
 # OBJ-NEXT:   Other: 0
 # OBJ-NEXT:   Section: Undefined (0x0)
 # OBJ-NEXT: }
 # OBJ-NEXT: Symbol {
-# OBJ-NEXT:   Name: internal (16)
+# OBJ-NEXT:   Name: default2
+# OBJ-NEXT:   Value: 0x0
+# OBJ-NEXT:   Size: 0
+# OBJ-NEXT:   Binding: Local (0x0)
+# OBJ-NEXT:   Type: None (0x0)
+# OBJ-NEXT:   Other: 0
+# OBJ-NEXT:   Section: Undefined (0x0)
+# OBJ-NEXT: }
+# OBJ-NEXT: Symbol {
+# OBJ-NEXT:   Name: internal
 # OBJ-NEXT:   Value: 0x0
 # OBJ-NEXT:   Size: 0
 # OBJ-NEXT:   Binding: Local (0x0)
 # OBJ-NEXT:   Type: None (0x0)
 # OBJ-NEXT:   Other [ (0x1)
 # OBJ-NEXT:     STV_INTERNAL (0x1)
 # OBJ-NEXT:   ]
 # OBJ-NEXT:   Section: Undefined (0x0)
 # OBJ-NEXT: }
 # OBJ-NEXT: Symbol {
-# OBJ-NEXT:   Name: hidden (9)
+# OBJ-NEXT:   Name: hidden
 # OBJ-NEXT:   Value: 0x0
 # OBJ-NEXT:   Size: 0
 # OBJ-NEXT:   Binding: Local (0x0)
 # OBJ-NEXT:   Type: None (0x0)
 # OBJ-NEXT:   Other [ (0x2)
 # OBJ-NEXT:     STV_HIDDEN (0x2)
 # OBJ-NEXT:   ]
 # OBJ-NEXT:   Section: Undefined (0x0)
 # OBJ-NEXT: }
 # OBJ-NEXT: Symbol {
-# OBJ-NEXT:   Name: protected (25)
+# OBJ-NEXT:   Name: protected
 # OBJ-NEXT:   Value: 0x0
 # OBJ-NEXT:   Size: 0
 # OBJ-NEXT:   Binding: Local (0x0)
 # OBJ-NEXT:   Type: None (0x0)
 # OBJ-NEXT:   Other [ (0x3)
 # OBJ-NEXT:     STV_PROTECTED (0x3)
 # OBJ-NEXT:   ]
 # OBJ-NEXT:   Section: Undefined (0x0)
 # OBJ-NEXT: }
 
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
   Type:    ET_REL
   Machine: EM_X86_64
 Symbols:
-  - Name:       default
-    Visibility: STV_DEFAULT
-  - Name:       internal
-    Visibility: STV_INTERNAL
-  - Name:       hidden
-    Visibility: STV_HIDDEN
-  - Name:       protected
-    Visibility: STV_PROTECTED
+  - Name:  default1
+  - Name:  default2
+    Other: [ STV_DEFAULT ]
+  - Name:  internal
+    Other: [ STV_INTERNAL ]
+  - Name:  hidden
+    Other: [ STV_HIDDEN ]
+  - Name:  protected
+    Other: [ STV_PROTECTED ]