[yaml2obj] - Don't allow setting StOther and Other/Visibility at the same time.

[grimar]

Summary

This is a follow up discussed in the comments of D66583.

Currently, if for example, we have both StOther and Other set in YAML document for a symbol,
then yaml2obj reports an "unknown key 'Other'" error.
It happens because 'mapOptional()' is never called for 'Other/Visibility' in this case,
leaving those unhandled.

This message does not describe the reason of the error well. This patch fixes it.

[jhenderson]

include/llvm/ObjectYAML/ELFYAML.h

112 ↗

(On Diff #216809)

sh_other -> st_other

113–115 ↗

(On Diff #216809)

I don't think you need the majority of this comment. How about

"when it is not possible to do so using the "Other" field, which only takes specific named constants".

lib/ObjectYAML/ELFEmitter.cpp

471 ↗

(On Diff #216809)

I just want to be clear that st_other is zero-initialized?

lib/ObjectYAML/ELFYAML.cpp

914–915 ↗

(On Diff #216809)

What if it's not possible to represent the value of st_other using Visibility and Other?

931 ↗

(On Diff #216809)

Maybe "StOther cannot be specified for Symbol with either Visibility or Other"?

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

81 ↗

(On Diff #216809)

How about "Check we can't set StOther for a symbol if Visibility or Other is also specified."

[MaskRay]

lib/ObjectYAML/ELFEmitter.cpp

471 ↗

(On Diff #216809)

It is uninitialized.

[grimar]

lib/ObjectYAML/ELFEmitter.cpp

471 ↗

(On Diff #216809)

It is uninitialized.

I think you're mistaken. It should be initialized because of 'Ret.resize(Symbols.size() + 1)' call. It is value initialized, i.e. zero initialized in this case.
(https://en.cppreference.com/w/cpp/container/vector/resize).

That is how it works for st_name or st_shndx I think. Am I missing something?

[MaskRay]

lib/ObjectYAML/ELFEmitter.cpp

471 ↗

(On Diff #216809)

Sorry, I was mistaken. I didn't notice the vector::resize call above. The elements are value initialized (zeroed), so the code is ok.

[grimar]

• Addressed review comments.

include/llvm/ObjectYAML/ELFYAML.h

113–115 ↗

(On Diff #216809)

LG, thanks!

lib/ObjectYAML/ELFYAML.cpp

914–915 ↗

(On Diff #216809)

Yes, this is a right question that I thought about too, but still have no good answer for now.

It is related to obj2yaml, not to yaml2obj and technically the straightforward way to solve this for the common case would probably be
to know somehow if the mapping operation failed or not. Then we can fall back to dumping the field just as "StOther" in some cases.
I think the existent API can't handle that.

There are other options probably. We can perhaps stop trying dumping st_other as flags at all, because:

PPC64 stores int values in this field, not just flags:

// Special values for the st_other field in the symbol table entry for PPC64.
enum {
  STO_PPC64_LOCAL_BIT = 5,
  STO_PPC64_LOCAL_MASK = (7 << STO_PPC64_LOCAL_BIT)
};
static inline int64_t decodePPC64LocalEntryOffset(unsigned Other) {
  unsigned Val = (Other & STO_PPC64_LOCAL_MASK) >> STO_PPC64_LOCAL_BIT;
  return ((1 << Val) >> 2) << 2;
}
static inline unsigned encodePPC64LocalEntryOffset(int64_t Offset) {
  unsigned Val =
      (Offset >= 4 * 4 ? (Offset >= 8 * 4 ? (Offset >= 16 * 4 ? 6 : 5) : 4)
                       : (Offset >= 2 * 4 ? 3 : (Offset >= 1 * 4 ? 2 : 0)));
  return Val << STO_PPC64_LOCAL_BIT;
}

So handling st_value as a set of flags is technically not 100% correct for this particular case.
But perhaps we can just try to isolate this corner case (that is inline with my patches at the current step I think).

I need to investigate the possible solutions for obj2yaml a bit deeper to suggest a good approach. Seems it is a subject for a different patch though?

931 ↗

(On Diff #216809)

Done.

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

81 ↗

(On Diff #216809)

Done.

[MaskRay]

lib/ObjectYAML/ELFYAML.cpp

914–915 ↗

(On Diff #216809)

I agree that dumping raw StOther is probably the best for PPC64 (its ELFv2 ABI uses the high bits)...

[jhenderson]

LGTM.

lib/ObjectYAML/ELFYAML.cpp

914–915 ↗

(On Diff #216809)

Okay, I agree that it's a possible future extension to obj2yaml to handle that case, as long as it's not going to produce incorrect ELF objects (e.g. with a value other than requested) in the case where it doesn't work (I'm okay with it emitting an error in this case for now). Using a raw StOther seems like the right thing to do longer term when things don't work.

A slightly more radical solution could be to only have one field "Other" (i.e. no Visibility or StOther), which simply takes a list of named parameters or an arbitrary integer. STV_HIDDEN etc could be allowed via the named parameters. I don't know if this is a good idea or not though.

[MaskRay]

lib/ObjectYAML/ELFYAML.cpp

914–915 ↗

(On Diff #216809)

Allow all of the following?

Other: [ STV_HIDDEN, STO_MIPS_OPTIONAL ] # MIPS
Other: [ STV_HIDDEN, 192 ] # PPC64 ELFv2
Other: 2

I think that will be great.

[grimar]

lib/ObjectYAML/ELFYAML.cpp

914–915 ↗

(On Diff #216809)

It looks good. I am not sure which/how much changes needs to be done to achieve that though,
but it perhaps worth investigating. (Hopefully this might help to simplify the code, the current approach with MappingNormalization and NormalizedOther is a bit too complex for my eyes).

Perhaps there are other places which can switch to such approach too.

I'll try to investigate.

[MaskRay]

This was reverted by rL370198

[grimar]

In D66642#1648848, @MaskRay wrote:

This was reverted by rL370198

Yes. Seems there is an uninitialized variable, I do not see it though (http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap-msan/builds/14395/steps/check-llvm%20msan/logs/stdio)
I am going to post a patch that suggests to eliminate the 'Visibility' and 'StOther' soon. So we probably will not need this.

[grimar]

I also can't understand how this could trigger MachO test failtures:

Failing Tests (3):

LLVM :: ObjectYAML/MachO/DWARF-debug_info.yaml
LLVM :: ObjectYAML/MachO/DWARF-debug_line.yaml
LLVM :: ObjectYAML/MachO/DWARF5-debug_info.yaml

I do not see how my changes can affect this.

[grimar]

It was reverted by a mistake and relanded in r370206

llvm/include/llvm/ObjectYAML/ELFYAML.h

struct Symbol {
   StringRef Name;
   Optional<uint32_t> NameIndex;
   ELF_STT Type;
   StringRef Section;
   Optional<ELF_SHN> Index;
   ELF_STB Binding;
   llvm::yaml::Hex64 Value;
   llvm::yaml::Hex64 Size;
-  uint8_t Other;
+  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;

llvm/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;
-    Symbol.st_other = Sym.Other;
+
+    if (Sym.Other)
+      Symbol.st_other = *Sym.Other;
+    else if (Sym.StOther)
+      Symbol.st_other = *Sym.StOther;
+
     Symbol.st_size = Sym.Size;
   }
 
   return Ret;
 }
 
 template <class ELFT>
 void ELFState<ELFT>::initSymtabSectionHeader(Elf_Shdr &SHeader,

llvm/lib/ObjectYAML/ELFYAML.cpp

void MappingTraits<ELFYAML::ProgramHeader>::mapping(
   IO.mapOptional("FileSize", Phdr.FileSize);
   IO.mapOptional("MemSize", Phdr.MemSize);
   IO.mapOptional("Offset", Phdr.Offset);
 }
 
 namespace {
 
 struct NormalizedOther {
-  NormalizedOther(IO &)
-      : Visibility(ELFYAML::ELF_STV(0)), Other(ELFYAML::ELF_STO(0)) {}
-  NormalizedOther(IO &, uint8_t Original)
-      : Visibility(Original & 0x3), Other(Original & ~0x3) {}
-
-  uint8_t denormalize(IO &) { return Visibility | Other; }
+  NormalizedOther(IO &) {}
+  NormalizedOther(IO &, Optional<uint8_t> Original) {
+    if (uint8_t Val = *Original & 0x3)
+      Visibility = Val;
+    if (uint8_t Val = *Original & ~0x3)
+      Other = Val;
+  }
+
+  Optional<uint8_t> denormalize(IO &) {
+    if (!Visibility && !Other)
+      return None;
+
+    uint8_t Ret = 0;
+    if (Visibility)
+      Ret |= *Visibility;
+    if (Other)
+      Ret |= *Other;
+    return Ret;
+  }
 
-  ELFYAML::ELF_STV Visibility;
-  ELFYAML::ELF_STO Other;
+  Optional<ELFYAML::ELF_STV> Visibility;
+  Optional<ELFYAML::ELF_STO> Other;
 };
 
 } // 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 that integer,
-  // ignoring the other fields.
-  Optional<llvm::yaml::Hex64> Other;
-  IO.mapOptional("StOther", Other);
-  if (Other) {
-    Symbol.Other = *Other;
-  } else {
-    MappingNormalization<NormalizedOther, uint8_t> Keys(IO, Symbol.Other);
-    IO.mapOptional("Visibility", Keys->Visibility, ELFYAML::ELF_STV(0));
-    IO.mapOptional("Other", Keys->Other, ELFYAML::ELF_STO(0));
-  }
+  // 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);
+  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/test/tools/yaml2obj/elf-symbol-stother.yaml

FileHeader:
   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 ]
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS32
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_MIPS
+Symbols:
+  - Name:    foo
+    StOther: 0
+    Visibility: STV_DEFAULT