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

[grimar]

Summary

st_other field of a symbol usually contains its visibility.
Other bits are usually 0, though some targets, like
MIPS can set them using the named bit field values.

Problem is that there is no way to set an arbitrary value now,
though that might be useful for our test cases.

In this patch I introduced a way to set st_other to any numeric
value using the new StOther field.
I added a test and simplified the existent one to show the effect/benefit.

[jhenderson]

lib/ObjectYAML/ELFYAML.cpp

893 ↗

(On Diff #216587)

helds -> holds

YAML -> YAML document

894 ↗

(On Diff #216587)

into the two fields -> into two fields

The last -> The former

895 ↗

(On Diff #216587)

"and so almost never printed. Though some targets..." -> "and so is almost never printed, although some targets..."

897 ↗

(On Diff #216587)

producing the broken -> producing broken

898 ↗

(On Diff #216587)

values -> value

899 ↗

(On Diff #216587)

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.

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

1 ↗

(On Diff #216587)

"sets the value of a symbol's..."

3 ↗

(On Diff #216587)

report -> reports
when -> when using an

4 ↗

(On Diff #216587)

is used in -> to what is specified by the

8 ↗

(On Diff #216587)

Nit: the lining up here looks a bit messy. I think you can just delete a single space before "Other:"

20 ↗

(On Diff #216587)

I'd change this first line to "Test that STO_* can be used with their correct machine type."

41 ↗

(On Diff #216587)

we can use "StOther" -> we can use the "StOther"

to set any arbitrary value to -> to set st_other to any arbitrary value

51 ↗

(On Diff #216587)

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

[grimar]

• Addressed review comments.

lib/ObjectYAML/ELFYAML.cpp

894 ↗

(On Diff #216587)

The last -> The former

I think you meant "-> The latter", as I really meant that "Other" usually holds no value.

899 ↗

(On Diff #216587)

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?

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

51 ↗

(On Diff #216587)

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.

[jhenderson]

LGTM, with one remaining comment nit.

lib/ObjectYAML/ELFYAML.cpp

894 ↗

(On Diff #216587)

Yes, sorry.

899 ↗

(On Diff #216587)

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.

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

3 ↗

(On Diff #216587)

You missed the "when using an" bit...

[MaskRay]

All of MIPS/PPC64/AArch64 can make use of these st_other bits now.

[grimar]

In D66583#1642404, @MaskRay wrote:

All of MIPS/PPC64/AArch64 can make use of these st_other bits now.

But do they need it? I see that yaml2obj supports only MIPS flags atm:

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
}

A normal way for PPC64/AArch64/(?) would be to teach yaml2obj about their flags and use an existent "Other" field
(instead of "StOther" added by this patch).

This is still useful for testing new STO_* flags and broken values on these targets though.

[MaskRay]

In D66583#1642408, @grimar wrote:

In D66583#1642404, @MaskRay wrote:

All of MIPS/PPC64/AArch64 can make use of these st_other bits now.

But do they need it? I see that yaml2obj supports only MIPS flags atm:

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
}

A normal way for PPC64/AArch64/(?) would be to teach yaml2obj about their flags and use an existent "Other" field
(instead of "StOther" added by this patch).

This is still useful for testing new STO_* flags and broken values on these targets though.

In PPC64, these bits are not used separately for difference purposes. Instead, it is shifted to represent a number, which encodes the localentry value. StOther might be the best to represent that piece of information.

llvm/lib/ObjectYAML/ELFYAML.cpp

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));
   }
+}
 
 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";
   return StringRef();

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

 # Show that llvm-readobj prints the symbol visibility where recognised, or
 # something sensible when not, for both GNU and LLVM output.
 
-# Use --dyn-symbols because it is only possible to hand-craft symbols with
-# non-standard st_other values for .dynsym.
 # RUN: yaml2obj %s > %t
-# RUN: llvm-readobj --symbols --dyn-symbols %t | FileCheck %s --check-prefix=LLVM
-# RUN: llvm-readelf --symbols --dyn-symbols %t | FileCheck %s --check-prefix=GNU
+# RUN: llvm-readobj --symbols %t | FileCheck %s --check-prefix=LLVM
+# RUN: llvm-readelf --symbols %t | FileCheck %s --check-prefix=GNU
 
 # LLVM:      Name: default
 # LLVM:      Other: 0
 # LLVM:      Name: internal
 # LLVM:      Other [ (0x1)
 # LLVM-NEXT:   STV_INTERNAL (0x1)
 # LLVM-NEXT: ]
 # LLVM:      Name: hidden
 # LLVM:      Other [ (0x2)
 # LLVM-NEXT:   STV_HIDDEN (0x2)
 # LLVM-NEXT: ]
 # LLVM:      Name: protected
 # LLVM:      Other [ (0x3)
 # LLVM-NEXT:   STV_PROTECTED (0x3)
 # LLVM-NEXT: ]
 # LLVM:      Name: other
 # LLVM:      Other [ (0x4)
 # LLVM-NEXT: ]
 
 # FIXME - the "other" symbol should print something indicating its non-zero st_other value.
 # See https://bugs.llvm.org/show_bug.cgi?id=40785.
-# GNU:      DEFAULT   {{.*}} other
 # GNU:      DEFAULT   {{.*}} default
 # GNU-NEXT: INTERNAL  {{.*}} internal
 # GNU-NEXT: HIDDEN    {{.*}} hidden
 # GNU-NEXT: PROTECTED {{.*}} protected
+# GNU-NEXT: DEFAULT   {{.*}} other
 
 !ELF
 FileHeader:
   Class:   ELFCLASS32
   Data:    ELFDATA2LSB
   Type:    ET_REL
   Machine: EM_386
-Sections:
-  - Name: .dynstr
-    Type: SHT_STRTAB
-    #\0other\0
-    Content: "006f7468657200"
-  - Name: .dynsym
-    Type: SHT_DYNSYM
-    Link: .dynstr
-    EntSize: 16
-    # Null symbol
-    # Symbol with st_name = 1, st_other = 0x4
-    Content: "0000000000000000000000000000000001000000000000000000000000040000"
 Symbols:
   - Name: default
     Visibility: STV_DEFAULT
     Binding: STB_GLOBAL
   - Name: internal
     Visibility: STV_INTERNAL
     Binding: STB_GLOBAL
   - Name: hidden
     Visibility: STV_HIDDEN
     Binding: STB_GLOBAL
   - Name: protected
     Visibility: STV_PROTECTED
     Binding: STB_GLOBAL
+  - Name: other
+    Binding: STB_GLOBAL
+    StOther: 4

llvm/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 ]
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS32
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_386
+Symbols:
+  - Name:  foo
+    Other: [ STO_MIPS_OPTIONAL ]
+
+## Test that STO_* can be used with their correct machine type.
+## We use the same YAML as above, but with a change of machine type.
+
+# RUN: yaml2obj --docnum=2 %s > %t2
+# RUN: llvm-readobj --symbols %t2 | FileCheck %s --check-prefix=USE-OTHER
+
+# USE-OTHER:      Name: foo
+# USE-OTHER:      Other [ (0x4)
+# USE-OTHER-NEXT:   STO_MIPS_OPTIONAL (0x4)
+# USE-OTHER-NEXT: ]
+
+--- !ELF
+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.
+
+# 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)
+
+--- !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