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

Fri Apr 22 02:38:59 PDT 2022

jhenderson added a comment.

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

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123902