[PATCH] D150870: [lld][Arm] Big Endian - Byte invariant support.

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

A few small follow up comments. I don't have anything beyond comments or naming. So hopefully can approve next round.



================
Comment at: lld/ELF/Arch/ARM.cpp:973
+
+// Add a mapping symbol to the mapping symbol list.
+void elf::addArmSyntheticSectionMappingSymbol(Defined *sym) {
----------------
Suggest changing the comment to explain why. For example:
```
Synthetic Sections are not backed by an ELF file where we can access the symbol table, instead mapping symbols added to Synthetic Sections are stored in the Synthetic Symbol Table. Due to the presence of strip, we cannot rely on the Synthetic Symbol Table retaining the mapping symbols. Instead we record the mapping symbols locally.
```


================
Comment at: lld/ELF/Options.td:39
 
+def be8: F<"be8">, HelpText<"Big endian with byte invariant">;
+
----------------
MaskRay wrote:
> peter.smith wrote:
> > peter.smith wrote:
> > > I'd go with something like "Write a Big Endian ELF file using BE8 format (Arm only)".
> > We'll also need to add be8 to the man page `llvm-project/lld/docs/ld/lld.1`
> `format(Arm only)` => `format (AArch32 only)`
Looks like this line is missing a space between format and (AArch32 only)


================
Comment at: lld/ELF/Target.h:221
 
+void convertArmInstructionstoBE8(InputSection *sec, uint8_t *buf);
+
----------------
It would be good to keep the Arm BE8 functionality together in a single chunk. Consider moving to where the other two functions are, or moving the other two up here.


================
Comment at: lld/ELF/Writer.cpp:2148
+
+  if (config->emachine == EM_ARM && !config->isLE && config->armBe8)
+    addArmInputSectionMappingSymbolsThenSort();
----------------
I have a mild preference for making sortArmMappingSymbols() non static and doing:
if (config->emachine == EM_ARM && !config->isLE && config->armBe8) {
  addArmInputSectionMappingSymbols();
  sortArmMappingSymbols();
}

But that isn't a strong opinion, feel free to keep this if you prefer.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150870