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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Arch/ARM.cpp:925
+  for (size_t i = 1, end = firstGlobal; i != end; ++i) {
+    error("CMSE symbol " + CHECK(eSyms[i].getName(stringTable), this) +
+          " in import library " + toString(this) + " is not global");
----------------
Prefer `errorOrWarn`


================
Comment at: lld/ELF/Arch/ARM.cpp:945
+
+    if (!(eSym.st_value & 0x1)) {
+      error("CMSE symbol " + sym->getName() + " in import library " +
----------------
Just use 1. Below you use `(a->value & 1) != 1` anyway.


================
Comment at: lld/ELF/Arch/ARM.cpp:957
+
+    if (symtab.cmseImportLib.find(sym->getName()) !=
+        symtab.cmseImportLib.end()) {
----------------
Use `.count(...)`


================
Comment at: lld/ELF/Arch/ARM.cpp:982
+        .str();
+
+  if (!a->isFunc() || (a->value & 1) != 1)
----------------
Drop blank lines for all the tests for `acleSeSym`. Only add a blank line separator when you start to test `sym`.


================
Comment at: lld/ELF/Arch/ARM.cpp:985
+    return (toString(a->file) + ": cmse special symbol '" + a->getName() +
+            "'; is not a Thumb function definition")
+        .str();
----------------
Drop the semicolon. It's an improper use.


================
Comment at: lld/ELF/Arch/ARM.cpp:995
+  if (!s)
+    return (toString(s->file) + ": cmse entry function '" + s->getName() +
+            "'; is an undefined symbol")
----------------
"Not a Defined" does not mean Undefined. You can drop this diagnostic and combine the condition with the below: `if (!(s && s->isFunc() && (s->value & 1)))`


================
Comment at: lld/ELF/Arch/ARM.cpp:1035
+    // Symbol must have external linkage.
+    StringRef name = acleSeSym->getName();
+    name.consume_front(ACLESESYM_PREFIX);
----------------
`StringRef name = acleSeSym->getName().substr(ACLESESYM_PREFIX.size());`


================
Comment at: lld/ELF/Arch/ARM.cpp:1255
+      FileOutputBuffer::create(config->cmseOutputLib, fileSize, flags);
+
+  if (!bufferOrErr) {
----------------
delete blank line


================
Comment at: lld/ELF/Arch/ARM.cpp:1303
+
+template void elf::writeARMCmseImportLib<ELF32LE>();
+template void elf::writeARMCmseImportLib<ELF32BE>();
----------------
We usually place explicit template instantiations in the end of the file.


================
Comment at: lld/ELF/Config.h:125
   std::vector<InputFile *> files;
+  std::vector<InputFile *> armCmseImpLib;
 
----------------
std::optional


================
Comment at: lld/ELF/Driver.cpp:349
+      error("--in-implib is only supported on ARM targets");
+
+    if (!config->cmseOutputLib.empty())
----------------
MaskRay wrote:
> delete blank line
Do we need an error when `--in-implib`/`--out-implib` is used without `--cmse-implib`?


================
Comment at: lld/ELF/InputFiles.cpp:181
+
+  // Only ARMv8-M or later architectures have CMSE support.
+  if (arch >= ARMBuildAttrs::CPUArch::v8_M_Base)
----------------
or => and   if my English understanding is correct :)


================
Comment at: lld/ELF/LinkerScript.cpp:932
+  auto linkerscript = inLinkerScript();
+  if (inCmdLine && linkerscript)
+    warn("addresses assigned to the veneers output section .gnu.sgstubs via "
----------------
As previously mentioned, the diagnostic only applying to .gnu.sgstubs is strange. Just test whether the OutputSection has a set `addrExpr`


================
Comment at: lld/ELF/SymbolTable.cpp:102
 
+void SymbolTable::addCmseSymPair(Symbol *acleSeSym, Symbol *sym) {
+  cmseSymVector.push_back(std::make_pair(acleSeSym, sym));
----------------
Remove this used-once helper and let the call site do the `emplace_back(acleSeSym, sym)`.


================
Comment at: lld/ELF/SyntheticSections.h:1150
+const int ACLESESYM_SIZE = 8;
+class ArmCmseSGVeneer : public SyntheticSection {
+public:
----------------
blank line before `class`


================
Comment at: lld/test/ELF/arm-cmse-implib-error.s:24
+    /// Symbol not absolute.
+    .global entry1_not_absolute
+    .type entry1_not_absolute, STT_FUNC
----------------
The 1/2/3/4 indexes are unneeded.


================
Comment at: lld/test/ELF/arm-cmse-implib-error.s:66
+
+// CHECK: error: CMSE symbol entry2_not_external in import library {{.*}} is not global
+// CHECK: error: CMSE symbol entry1_not_absolute in import library {{.*}} is not absolute
----------------
You can use `FileCheck -DIMPLIB=%t.lib` to test the import library path in the error message. Grep `FileCheck.*-D`


================
Comment at: lld/test/ELF/arm-cmse-secure.s:7
+/// Create the secure app and import library.
+// RUN: ld.lld --entry=secure_entry --section-start .gnu.sgstubs=0x20000 --cmse-implib %timplib.o %tsecureapp.o --out-implib=%timplib.lib -o %tsecureapp
+/// Link the non-secure app against the import library
----------------
If the line starts to look long, you can change `--entry=` to `-e `. `-e ` is commonly used so the abbreviation is fine.


================
Comment at: lld/test/ELF/arm-cmse-secure.s:8
+// RUN: ld.lld --entry=secure_entry --section-start .gnu.sgstubs=0x20000 --cmse-implib %timplib.o %tsecureapp.o --out-implib=%timplib.lib -o %tsecureapp
+/// Link the non-secure app against the import library
+// RUN: ld.lld --entry=nonsecure_entry -Ttext=0x8000 -o %t1 --in-implib=%timplib.lib --cmse-implib %tnonsecureapp.o -o %tnonsecureapp
----------------



================
Comment at: lld/test/ELF/arm-cmse-secure.s:9
+/// Link the non-secure app against the import library
+// RUN: ld.lld --entry=nonsecure_entry -Ttext=0x8000 -o %t1 --in-implib=%timplib.lib --cmse-implib %tnonsecureapp.o -o %tnonsecureapp
+// RUN: llvm-readelf -s %timplib.lib | FileCheck %s
----------------
Redundant `-o %t1`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139092