[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:940
+    if (eSym.st_shndx != SHN_ABS) {
+      error("CMSE symbol " + sym->getName() + " in import library " +
+            toString(this) + " is not absolute");
----------------
Surround symbol names and library filenames with single quotes


================
Comment at: lld/ELF/Arch/ARM.cpp:945
+
+    if (!(eSym.st_value & 1)) {
+      error("CMSE symbol " + sym->getName() + " in import library " +
----------------
The two conditions can be combined as done in `checkCmseSymAttributes`. The message can be unified as well.


================
Comment at: lld/ELF/Arch/ARM.cpp:978
+  if (!a)
+    return (toString(acleSeSym->file) + ": cmse special symbol '" +
+            acleSeSym->getName() + "' is not defined")
----------------
This is slightly more efficient.


================
Comment at: lld/ELF/Arch/ARM.cpp:991
+  Defined *s = dyn_cast_or_null<Defined>(sym);
+  if (!s)
+    return (toString(s->file) + ": cmse entry function '" + s->getName() +
----------------
The 3 checks can be made a local lambda to be shared by `acleSeSym` and `sym`. The lambda can have a parameter to differentiate `special` and `entry`.


================
Comment at: lld/ELF/Arch/ARM.cpp:993
+    return (toString(s->file) + ": cmse entry function '" + s->getName() +
+            "' is an undefined symbol")
+        .str();
----------------
is not defined.

Again, Not a `Defined` does not imply `Undefined`.


================
Comment at: lld/ELF/Arch/ARM.cpp:999
+        .str();
+  if (s->section == nullptr)
+    return (toString(s->file) + ": cmse entry function '" + s->getName() +
----------------



================
Comment at: lld/ELF/Arch/ARM.cpp:1022
+      continue;
+    if (!config->armCMSESupport)
+      error("cmse special symbol '" + acleSeSym->getName() +
----------------
Printing an error for every symbol is verbose. I think `armCMSESupport` should be removed.

In `updateSupportedARMFeatures`, report a single error if not v8-M and perhaps `config->cmseImplib = false;` to disable `processArmCmseSymbol`.


================
Comment at: lld/ELF/Arch/ARM.cpp:1044
+    // <sym> may be redefined later in the link in .gnu.sgstubs
+    symtab.cmseSymVector.emplace_back(std::make_pair(acleSeSym, sym));
+  }
----------------



================
Comment at: lld/ELF/Arch/ARM.cpp:1091
+  // Only secure symbols with values equal to that of it's non-secure
+  // counterpart needs to be in the .sg.stubs section.
+  ArmCmseSGVeneer *ss = nullptr;
----------------
wrong section name


================
Comment at: lld/ELF/Arch/ARM.cpp:1106
+  for (auto *s : this->sgSections) {
+    auto off = s->outSecOff - getVA();
+    s->writeTo(buf + off);
----------------
inline this used-once variable and delete braces


================
Comment at: lld/ELF/Driver.cpp:2744
 
+  // Process CMSE symbols
+  processArmCmseSymbol();
----------------



================
Comment at: lld/ELF/InputFiles.cpp:308
+void elf::parseArmCMSEImportLib(std::optional<InputFile *> file) {
+  invokeELFT(doParseArmCMSEImportLib, file);
+}
----------------
Place the `if (file)` check in the caller and use `InputFile &file` instead of `std::optional`.


================
Comment at: lld/ELF/InputFiles.cpp:1137
   ArrayRef<Elf_Sym> eSyms = this->getELFSyms<ELFT>();
+
   for (size_t i = firstGlobal, end = eSyms.size(); i != end; ++i) {
----------------
revert


================
Comment at: lld/ELF/LinkerScript.cpp:904
+void LinkerScript::diagnoseMissingSGSectionAddress() const {
+  if (!config->cmseImplib || !in.armCmseSGSection.get()->isNeeded())
+    return;
----------------



================
Comment at: lld/ELF/LinkerScript.cpp:908
+  OutputSection *sec = findByName(sectionCommands, ".gnu.sgstubs");
+  if (sec != nullptr && !sec->addrExpr &&
+      !config->sectionStartMap.count(".gnu.sgstubs"))
----------------



================
Comment at: lld/ELF/SyntheticSections.cpp:3840
   symTabShndx.reset();
+  armCmseSGSection.reset();
 }
----------------
Move this before ppc64 to match the order in the declaration.


================
Comment at: lld/ELF/SyntheticSections.h:1180
+  void addSGVeneer(Symbol *sym, Symbol *ext_sym);
+  inline void addMappingSymbol();
+  void finalizeContents() override;
----------------
The `inline` doesn't seem useful. It's only called once anyway.


================
Comment at: lld/ELF/Writer.cpp:457
 
+  in.armCmseSGSection = std::make_unique<ArmCmseSGSection>();
+  add(*in.armCmseSGSection);
----------------
Do this only if `EM_ARM`


================
Comment at: lld/ELF/Writer.cpp:598
+
+    if (config->emachine == EM_ARM)
+      writeARMCmseImportLib<ELFT>();
----------------
Move the condition `config->cmseOutputLib.empty()` and then you can remove the `EM_ARM` condition.


================
Comment at: lld/test/ELF/arm-cmse-check-startaddress.s:3
+/// Test that the address of .gnu.sgstubs can be specified via command line or linker script.
+/// Test that the linker warns when the address of .gnu.sgstubs is specified via both command line and linker script.
+/// Test that the linker errors when the address of .gnu.sgstubs is not specified using either method.
----------------



================
Comment at: lld/test/ELF/arm-cmse-check-startaddress.s:9
+// RUN: llvm-mc -arm-add-build-attributes --triple=thumbv8m.main -filetype=obj -I%S/Inputs/ asm -o t.o
+// RUN: ld.lld --cmse-implib -Ttext=0x8000 --section-start .gnu.sgstubs=0x20000 --script with_sgstubs.script t.o -o 1 2>&1 | FileCheck %s --check-prefixes=NOERROR
+// RUN: ld.lld --cmse-implib -Ttext=0x8000 --section-start .gnu.sgstubs=0x20000 --script wout_sgstubs.script t.o -o 2 2>&1 | FileCheck %s --check-prefixes=NOERROR
----------------
Use `FileCheck /dev/null --implicit-check-not=error:` in stead of `NOERROR`


================
Comment at: lld/test/ELF/arm-cmse-check-sym-attrs.s:2
+// REQUIRES: arm
+/// Test diagnostics emitted during symbol attribute checks
+
----------------



================
Comment at: lld/test/ELF/arm-cmse-cmdline-options.s:1
+# REQUIRES: arm
+## Test diagnostics when --in-implib/--out-implib are used without --cmse-implib
----------------
There are still too many test files. Consider grouping this negative test with others together.


================
Comment at: lld/test/ELF/arm-cmse-cmdline-options.s:2
+# REQUIRES: arm
+## Test diagnostics when --in-implib/--out-implib are used without --cmse-implib
+# RUN: yaml2obj %s -o %t.o
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139092