[PATCH] D123902: [llvm-objcopy][mips] Add MIPS specific ELF section indexes

Mon Apr 18 23:46:25 PDT 2022

jhenderson added a comment.

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.

================
Comment at: llvm/include/llvm/BinaryFormat/ELF.h:569-573
+  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
----------------
Comments should start with a capital letter, as per the prevailing style in this file.

================
Comment at: llvm/lib/ObjectYAML/ELFYAML.cpp:768
+
+  if (Object->getMachine() == ELF::EM_MIPS) {
+    ECase(SHN_MIPS_ACOMMON);
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123902