[llvm-objcopy][ObjectYAML][mips] Add MIPS specific ELF section indexes

[argentite]

Summary

This fixes https://github.com/llvm/llvm-project/issues/53998
and displays correct information in obj2yaml for SHN_MIPS_*
sections according to
https://refspecs.linuxfoundation.org/elf/mipsabi.pdf

[argentite]

Added full issue URL

[jhenderson]

Could you add some testing, please? If you grep llvm/test for SHN_HEXAGON_SCOMMON, you'll find 3 tests that use it, and I imagine you'll want equivalent testing for your new values.

llvm/include/llvm/BinaryFormat/ELF.h

569–573

Comments should start with a capital letter, as per the prevailing style in this file.

llvm/lib/ObjectYAML/ELFYAML.cpp

768

This if isn't present for other machines (see the below Hexagon and AMDGPU values), and I'm not convinced we want it here. It is sometimes useful to be able to write YAML that uses one of these properties, but is not for MIPS. In this case, the value would be used, but it would be meaningless.

[argentite]

Fix comment capitalization

llvm/lib/ObjectYAML/ELFYAML.cpp

768

Apologies if my understanding is incorrect but it seems like without the machine check, the first case matching the value will be shown in obj2yaml. Some of the values such as SHN_MIPS_SCOMMON and SHN_HEXAGON_SCOMMON_4 conflict with each other (0xff03), so whichever appears first in the code will be displayed (also leading to some test failures like tools/obj2yaml/ELF/special-symbol-indices.yaml).

[jhenderson]

Your patch summary should probably have an [ObjectYAML] tag too, since it impacts more than just [llvm-objcopy].

llvm/lib/ObjectYAML/ELFYAML.cpp

768

If I'm not mistaken (it's been a while since I really dug into this code, so I could be), this piece of code is used for both obj2yaml and yaml2obj. We probably DO want it to work for yaml2obj, since there's no reason why it shouldn't, and it makes writing tests such as tools/llvm-objcopy/ELF/hexagon-unsupported-on-x86.test easier to write. I see your point about the other way around though: could you make the conditional also conditioned on it being the obj2yaml direction?

If you do that, you should probably make a small follow-up patch to do the same for the AMDGPU and Hexagon values below, or this code will be left in an inconsistent state.

[argentite]

This should accept both valid and invalid sections in input and only valid sections in output.

[MaskRay]

llvm/lib/ObjCopy/ELF/ELFObject.h

624

Move MIPS after HEXAGON to keep an alphabetical order for arch-specific values.

llvm/test/tools/llvm-objcopy/ELF/mips-symbol.test

25 ↗

(On Diff #426286)

Add a space for all lines below

[argentite]

Fixed ordering and formatting

[jhenderson]

llvm/lib/ObjectYAML/ELFYAML.cpp

769

I'd like to see test cases in yaml2obj and obj2yaml showing that the MIPS-specific values are understood under the right circumstances only. I think extending obj2yaml\ELF\special-symbol-indices.yaml should be a minimum for that.

llvm/test/tools/llvm-objcopy/ELF/mips-symbol.test

1 ↗

(On Diff #426402)

You may also wish to rename and extend llvm-objcopy\ELF\hexagon-unsupported-on-x86.test, or alternatively, modify this test to cover that case, by attempting the same run, but with a different value for the Machine field (you can use yaml2obj's -D option to parameterise the field without needing to duplicate the document).

[argentite]

This handles the testing partially. I think it can only be properly done when the condition has been added for Hexagon and AMDGPU as well beacuse currently it falls back to Hexagon if MIPS does not match.

[jhenderson]

LGTM, but please rename the test file as indicated, before landing.

llvm/test/tools/llvm-objcopy/ELF/hexagon-unsupported-on-x86.test

1 ↗

(On Diff #427641)

If you're adding MIPS to this test, I'd rename it to something like unsupported-machine-specific-shndx.test.

[argentite]

Renamed test

[argentite]

This is my first patch. I don't have push access. Can you push it for me if nobody has any other comments?

[MaskRay]

In D123902#3537437, @argentite wrote:

This is my first patch. I don't have push access. Can you push it for me if nobody has any other comments?

I will test and push this:)

llvm/include/llvm/BinaryFormat/ELF.h

enum : unsigned {
   EF_MIPS_ARCH_64 = 0x60000000,   // MIPS64 instruction set per linux not elf.h
   EF_MIPS_ARCH_32R2 = 0x70000000, // mips32r2, mips32r3, mips32r5
   EF_MIPS_ARCH_64R2 = 0x80000000, // mips64r2, mips64r3, mips64r5
   EF_MIPS_ARCH_32R6 = 0x90000000, // mips32r6
   EF_MIPS_ARCH_64R6 = 0xa0000000, // mips64r6
   EF_MIPS_ARCH = 0xf0000000       // Mask for applying EF_MIPS_ARCH_ variant
 };
 
+// MIPS-specific section indexes
+enum {
+  SHN_MIPS_ACOMMON = 0xff00,   // Common symbols which are defined and allocated
+  SHN_MIPS_TEXT = 0xff01,      // Not ABI compliant
+  SHN_MIPS_DATA = 0xff02,      // Not ABI compliant
+  SHN_MIPS_SCOMMON = 0xff03,   // Common symbols for global data area
+  SHN_MIPS_SUNDEFINED = 0xff04 // Undefined symbols for global data area
+};
+
 // ELF Relocation types for Mips
 enum {
 #include "ELFRelocs/Mips.def"
 };
 
 // Special values for the st_other field in the symbol table entry for MIPS.
 enum {
   STO_MIPS_OPTIONAL = 0x04,  // Symbol whose definition is optional

llvm/lib/ObjCopy/ELF/ELFObject.h

 // field means. Most of the values are just copies of the special SHN_* values.
 // SYMBOL_SIMPLE_INDEX means that the st_shndx is just an index of a section.
 enum SymbolShndxType {
   SYMBOL_SIMPLE_INDEX = 0,
   SYMBOL_ABS = ELF::SHN_ABS,
   SYMBOL_COMMON = ELF::SHN_COMMON,
   SYMBOL_LOPROC = ELF::SHN_LOPROC,
   SYMBOL_AMDGPU_LDS = ELF::SHN_AMDGPU_LDS,
+  SYMBOL_MIPS_ACOMMON = ELF::SHN_MIPS_ACOMMON,
+  SYMBOL_MIPS_TEXT = ELF::SHN_MIPS_TEXT,
+  SYMBOL_MIPS_DATA = ELF::SHN_MIPS_DATA,
+  SYMBOL_MIPS_SCOMMON = ELF::SHN_MIPS_SCOMMON,
+  SYMBOL_MIPS_SUNDEFINED = ELF::SHN_MIPS_SUNDEFINED,
   SYMBOL_HEXAGON_SCOMMON = ELF::SHN_HEXAGON_SCOMMON,
   SYMBOL_HEXAGON_SCOMMON_2 = ELF::SHN_HEXAGON_SCOMMON_2,
   SYMBOL_HEXAGON_SCOMMON_4 = ELF::SHN_HEXAGON_SCOMMON_4,
   SYMBOL_HEXAGON_SCOMMON_8 = ELF::SHN_HEXAGON_SCOMMON_8,
   SYMBOL_HIPROC = ELF::SHN_HIPROC,
   SYMBOL_LOOS = ELF::SHN_LOOS,
   SYMBOL_HIOS = ELF::SHN_HIOS,
   SYMBOL_XINDEX = ELF::SHN_XINDEX,

llvm/lib/ObjCopy/ELF/ELFObject.cpp

static bool isValidReservedSectionIndex(uint16_t Index, uint16_t Machine) {
   case SHN_COMMON:
     return true;
   }
 
   if (Machine == EM_AMDGPU) {
     return Index == SHN_AMDGPU_LDS;
   }
 
+  if (Machine == EM_MIPS) {
+    switch (Index) {
+    case SHN_MIPS_ACOMMON:
+    case SHN_MIPS_SCOMMON:
+    case SHN_MIPS_SUNDEFINED:
+      return true;
+    }
+  }
+
   if (Machine == EM_HEXAGON) {
     switch (Index) {
     case SHN_HEXAGON_SCOMMON:
     case SHN_HEXAGON_SCOMMON_1:
     case SHN_HEXAGON_SCOMMON_2:
     case SHN_HEXAGON_SCOMMON_4:
     case SHN_HEXAGON_SCOMMON_8:
       return true;

llvm/lib/ObjectYAML/ELFYAML.cpp

default:
     // Nothing to do.
     break;
   }
 #undef BCase
 }
 
 void ScalarEnumerationTraits<ELFYAML::ELF_SHN>::enumeration(
     IO &IO, ELFYAML::ELF_SHN &Value) {
+  const auto *Object = static_cast<ELFYAML::Object *>(IO.getContext());
+  assert(Object && "The IO context is not initialized");
 #define ECase(X) IO.enumCase(Value, #X, ELF::X)
   ECase(SHN_UNDEF);
   ECase(SHN_LORESERVE);
   ECase(SHN_LOPROC);
   ECase(SHN_HIPROC);
   ECase(SHN_LOOS);
   ECase(SHN_HIOS);
   ECase(SHN_ABS);
   ECase(SHN_COMMON);
   ECase(SHN_XINDEX);
   ECase(SHN_HIRESERVE);
   ECase(SHN_AMDGPU_LDS);
+  //ECase(SHN_MIPS_ACOMMON);
+  //ECase(SHN_MIPS_TEXT);
+  //ECase(SHN_MIPS_DATA);
+  //ECase(SHN_MIPS_SCOMMON);
+  //ECase(SHN_MIPS_SUNDEFINED);
   ECase(SHN_HEXAGON_SCOMMON);
   ECase(SHN_HEXAGON_SCOMMON_1);
   ECase(SHN_HEXAGON_SCOMMON_2);
   ECase(SHN_HEXAGON_SCOMMON_4);
   ECase(SHN_HEXAGON_SCOMMON_8);
 #undef ECase
   IO.enumFallback<Hex16>(Value);
 }