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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Arch/ARM.cpp:23
 
+enum class CodeState { Data = 0, Thumb = 2, Arm = 4 };
+
----------------
Move `CodeState` into the end of the anonymous namespace 

Make sectionMap `static` after the anonymous namespace.

https://llvm.org/docs/CodingStandards.html#anonymous-namespaces


================
Comment at: lld/ELF/Arch/ARM.cpp:25
+
+DenseMap<InputSection *, std::vector<const Defined *>> sectionMap;
+
----------------
`std::vector => SmallVector<const Defined *, 0>`


================
Comment at: lld/ELF/Arch/ARM.cpp:939
+  // Collect mapping symbols for every executable InputSection.
+  for (const SymbolTableEntry &entry : in.symTab->getSymbols()) {
+    auto *def = dyn_cast<Defined>(entry.sym);
----------------
`in.symTab` will be null with `--strip-all`


================
Comment at: lld/ELF/Arch/ARM.cpp:945
+      continue;
+    if (auto *sec = dyn_cast_or_null<InputSection>(def->section))
+      if (sec->flags & SHF_EXECINSTR)
----------------
`cast_if_present`


================
Comment at: lld/ELF/Arch/ARM.cpp:985
+  CodeState curState = CodeState::Data;
+
+  uint64_t start = 0, width = 0, length = sec->getSize();
----------------
Don't add blank line between local variable declarations


================
Comment at: lld/ELF/Arch/ARM.cpp:987
+  uint64_t start = 0, width = 0, length = sec->getSize();
+
+  for (auto &msym : mapSyms) {
----------------
Delete this blank line.


================
Comment at: lld/ELF/Arch/ARM.cpp:995
+    else if (isDataMapSymbol(msym))
+      newState = CodeState::Data;
+
----------------
`if (isDataMapSymbol(msym))` is unneeded.


================
Comment at: lld/ELF/Arch/ARM.cpp:1012
+    width = static_cast<uint64_t>(curState);
+    toLittleEndianInstructions(buf, start, length, width);
+  }
----------------
`length` is uncommon in lld. Use `size`.


================
Comment at: lld/ELF/Config.h:204
   bool asNeeded = false;
+  bool be8 = false;
   BsymbolicKind bsymbolic = BsymbolicKind::None;
----------------
`bool armBe8 = false;`


================
Comment at: lld/ELF/Options.td:39
 
+def be8: F<"be8">, HelpText<"Big endian with byte invariant">;
+
----------------
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)`


================
Comment at: lld/ELF/Target.h:222
 void mergeRISCVAttributesSections();
+void getArmMappingSymbolList();
 
----------------
`...List` is uncommon in lld code. use the plural form `Symbols`


================
Comment at: lld/docs/ld.lld.1:92
+.It Fl --be8
+Write a Big Endian ELF File using BE8 format
 .It Fl -build-id Ns = Ns Ar value
----------------
` (AArch32 only)`


================
Comment at: lld/test/ELF/arm-plt-reloc.s:260
+// CHECKLONG-EB-NEXT: <func1>:
+// CHECKLONG-EB-NEXT:     1000:       bx      lr
+// CHECKLONG-EB: <func2>:
----------------
Omit addresses that are not significant. These addresses make test updating difficult if text section addresses change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150870