[PATCH] D139092: [RFC][LLD][ELF] Cortex-M Security Extensions (CMSE) Support

[Fangrui Song via Phabricator via llvm-commits]

MaskRay requested changes to this revision.
MaskRay added inline comments.
This revision now requires changes to proceed.


================
Comment at: lld/ELF/Arch/ARM.cpp:917
+template <typename ELFT> void elf::writeARMCmseImportLib() {
+  if (config->emachine != EM_ARM)
+    return;
----------------
delete


================
Comment at: lld/ELF/Arch/ARM.cpp:920
+
+  if (in.armCmseSGSection->getSize() == 0)
+    return;
----------------
combine two `if`


================
Comment at: lld/ELF/Arch/ARM.cpp:934
+
+  inputSections.push_back(symTab);
+  inputSections.push_back(symTabShndx);
----------------
tschuett wrote:
> MaskRay wrote:
> > amilendra wrote:
> > > tschuett wrote:
> > > > I have literally no idea what you are doing, but you create a `SmallVector` `inputSections` of size 0 and then immediately add 4 elements.
> > > Thanks for pointing it out. I'll improve it.
> > `SmallVector<SyntheticSection *, ４>` instantiates some code which bloats the lld executable. `SmallVector<SyntheticSection *, 0>` makes sense if `SmallVector<SyntheticSection *, ４>` hasn't been used globally.
> Then it is binary size vs. malloc.
I'll optimize for binary size and code size. `Inline elements>0` compiles to more code and optimizes for this very niche extension which is only called once. It's not a good tradeoff to optimize away a few malloc.


================
Comment at: lld/ELF/Arch/ARM.cpp:939
+
+  OutputSection *elfHeader1 = make<OutputSection>(symTab->name, 0, 0);
+  OutputSection *elfHeader2 = make<OutputSection>(strTab->name, 0, 0);
----------------
These are not `ELF header`. `elfheader*` are not good names.


================
Comment at: lld/ELF/Arch/ARM.cpp:948
+  };
+  recordInputSections(elfHeader1, symTab);
+  recordInputSections(elfHeader2, strTab);
----------------
Several functions are repeatedly called. Use an array for the 4 `OutputSection`s and use loops.


================
Comment at: lld/ELF/Arch/ARM.cpp:1030
+  // Write section contents to a mmap'ed file.
+  parallel::TaskGroup tg;
+
----------------
TaskGroup's destructor ensures the tasks are done. It must finish before `buffer->commit` is called.

```
{
  parallel::TaskGroup tg;
  for (...)
    xxx->writeTo<ELFT>(...);
}
```


================
Comment at: lld/ELF/Arch/ARM.cpp:1037
+
+  errorToErrorCode(buffer->commit());
+}
----------------
Use the `if (auto e = buffer->commit()) fatal(..)`  pattern in Writer.cpp


================
Comment at: lld/ELF/InputFiles.cpp:1121
+  ArrayRef<Elf_Sym> eSyms = this->getELFSyms<ELFT>();
+  if (numSymbols == 0) {
+    numSymbols = eSyms.size();
----------------
delete `numSymbols == 0` which is always true


================
Comment at: lld/ELF/InputFiles.cpp:1126
+
+  // Some entries have been filled by LazyObjFile.
+  for (size_t i = firstGlobal, end = eSyms.size(); i != end; ++i) {
----------------
The comment is incorrect. There is no lazy state.


================
Comment at: lld/ELF/InputFiles.cpp:1190
+
+  StringRef ext_name = getNameFromACLESE(sym.getName());
+  // Try to find the associated symbol. Search only in global symbols
----------------
Use `consume_front`


================
Comment at: lld/ELF/InputFiles.cpp:1198
+          toString(sym.file) + " but no associated global symbol definition " +
+          ext_name + " found.");
+    return;
----------------
Diagnostics are not capitalized and don't have a trailing period.


================
Comment at: lld/ELF/InputFiles.cpp:1237
   ArrayRef<Elf_Sym> eSyms = this->getELFSyms<ELFT>();
+  if (toString(this) == config->in_implib)
+    return;
----------------
postParse is performance critical and parallel. `symtab.addCmseSymPair(&sym, ext_sym);` is racy.

Move cmse-specific code here and `processArmCmseSymbol(sym, secIdx);` to a separate pass. Consider just iterating `symtab` when `--cmse-implib` is used so that `symtab.find(ext_name)` and `cmseSymMap` can be avoided (`symtab` doesn't have duplicate entries).


================
Comment at: lld/ELF/Options.td:83
 
+def cmse_implib: F<"cmse-implib">, HelpText<"Make import library to be a secure gateway import">;
+
----------------
We prefer `FF` and `EEq` for new options which only support the double-dash form.


================
Comment at: lld/ELF/SymbolTable.cpp:106
+  if (p.second) {
+    cmseSymVector.push_back(std::make_pair(acle_sg_sym, sym));
+  }
----------------
delete braces around a simple single statement.


================
Comment at: lld/ELF/SyntheticSections.cpp:2194
+    Defined *sym = p.second;
+    if (impLibMinAddr > sym->value)
+      impLibMinAddr = sym->value;
----------------
This order is more common.


================
Comment at: lld/ELF/SyntheticSections.cpp:2197
+    if (impLibMaxAddr <= sym->value) {
+      impLibMaxAddr = sym->value + this->entsize;
+    }
----------------
delete braces around simple single statement

The `if` for `impLibMaxAddr` looks strange. Move `impLibMaxAddr += this->entsize` after the loop.


================
Comment at: lld/ELF/SyntheticSections.cpp:2200
+  }
+  if (config->emachine == EM_ARM && !symtab.cmseSymVector.empty()) {
+    addMappingSymbol();
----------------
delete `config->emachine == EM_ARM`


================
Comment at: lld/ELF/SyntheticSections.cpp:2202
+    addMappingSymbol();
+
+    for (const auto &[acle_sg_sym, sym] : symtab.cmseSymVector)
----------------
delete blank line


================
Comment at: lld/ELF/SyntheticSections.cpp:2218
+  auto SI = symtab.cmseImportLib.find(sym->getName());
+  if (SI == symtab.cmseImportLib.end()) {
+    ArmCmseVariableAddressSGVeneer *ss =
----------------
Use `try_emplace` and the pair result to know whether the value has been inserted. Avoid code duplication.


================
Comment at: lld/ELF/SyntheticSections.cpp:2251
+
+void ArmCmseSGSection::addMappingSymbol() {
+  addSyntheticLocal("$t", STT_NOTYPE, /* off */ 0, /* size */ 0, *this);
----------------
inline this called-only-once function.


================
Comment at: lld/ELF/SyntheticSections.cpp:2252
+void ArmCmseSGSection::addMappingSymbol() {
+  addSyntheticLocal("$t", STT_NOTYPE, /* off */ 0, /* size */ 0, *this);
+}
----------------
The preferred syntax is `/*off=*/0`


================
Comment at: lld/ELF/SyntheticSections.h:1222
 };
+class ArmCmseSGVeneer : public SyntheticSection {
+public:
----------------
blank line above

use final


================
Comment at: lld/ELF/SyntheticSections.h:1242
+// The symbol table in the import library should have a size
+// specified
+class ArmCmseFixedAddressSGVeneer : public ArmCmseSGVeneer {
----------------
`a specified size.`


================
Comment at: lld/ELF/SyntheticSections.h:1247
+      : ArmCmseSGVeneer(sym, acle_sg_sym, ArmCmseSGVeneer::FixedAddress) {
+
+    entsize = 8;
----------------
delete blank line


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139092