[PATCH] D115635: [llvm-objcopy] Fix handling of MIPS64 little endian files

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Since the "Mips64EL" property is a propert of the input object, I wonder if we should just make it a member of the object, and then put a reference to that object in the relocation sections, so that we can query that, rather than needing to a) pass around the variable, and b) have to have the property in the relocation section? What do you think?



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/mips64el.test:5-7
+# RUN: yaml2obj %s -DENDIANNESS=ELFDATA2MSB -o %t
+# RUN: llvm-objcopy %t %t2
+# RUN: llvm-readobj --relocations %t2 | FileCheck %s
----------------
I'd name the input and output objects differently for the two cases, so that we don't overwrite the first case with the second case. This is sometimes useful for test debugging.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/mips64el.test:11-14
+  Class:           ELFCLASS64
+  Data:            [[ENDIANNESS]]
+  Type:            ET_REL
+  Machine:         EM_MIPS
----------------
We usually get rid of excessive spacing within a block, as per the suggested edit. Same below.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/mips64el.test:18-19
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    AddressAlign:    0x10
+    Content:         0000000000000000
----------------
I'd be surprised if you need either of these fields.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/mips64el.test:20
+    AddressAlign:    0x10
+    Content:         0000000000000000
+  - Name:            .rela.text
----------------
I'd replace this with a Size field, if the size is important to the test case. If it isn't important, consider just getting rid of the field entirely.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/mips64el.test:24-25
+    Flags:           [ SHF_INFO_LINK ]
+    Link:            .symtab
+    AddressAlign:    0x8
+    Info:            .text
----------------
These fields are redundant (either they're the default, or simply not needed).


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/mips64el.test:37-42
+  - Name:            bar
+    Type:            STT_FUNC
+    Section:         .text
+    Binding:         STB_GLOBAL
+    Value:           8
+    Size:            8
----------------
Do you need this second symbol? It doesn't seem to be being used.


================
Comment at: llvm/tools/llvm-objcopy/ELF/Object.h:799-800
       const DenseMap<SectionBase *, SectionBase *> &FromTo) override;
+  bool IsMips64EL() const { return IsObjMips64EL; }
+  void SetMips64EL(bool value) { IsObjMips64EL = value; }
 
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115635