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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

I have performed the second round of checking. I think there is still lots of needed work to make this ready for review.



================
Comment at: lld/ELF/Driver.cpp:346
+      error("--cmse-implib is only supported on ARM targets");
+
+    if (!config->cmseInputLib.empty())
----------------
delete blank line


================
Comment at: lld/ELF/Driver.cpp:349
+      error("--in-implib is only supported on ARM targets");
+
+    if (!config->cmseOutputLib.empty())
----------------
delete blank line


================
Comment at: lld/ELF/Driver.cpp:1675
+    case OPT_in_implib:
+      if (std::optional<MemoryBufferRef> mb = readFile(arg->getValue()))
+        files.push_back(createObjFile(*mb));
----------------
This parses the `--in-implib=` file as a regular file and requires a condition in the generic parsing code which I have mentioned is improper.

If the parse order does not matter, record this in a vector and perform parsing in the end of `llvm::TimeTraceScope timeScope("Parse input files");`


================
Comment at: lld/ELF/Driver.cpp:2355
+// Both symbols should be global/weak Thumb code symbol definitions.
+static bool checkCmseSymAttributes(Symbol *acle_se_sym, Symbol *sym) {
+  Defined *a = dyn_cast_or_null<Defined>(acle_se_sym);
----------------
This function should be moved to ELF/Arch/ARM.cpp to not pollute the generic code.

This function can be changed to `std::string checkCmseSymAttributes`. The caller can check whether the return value is empty; if yes, call `error`.


================
Comment at: lld/ELF/Driver.cpp:2359
+    error(toString(acle_se_sym->file) + ": cmse special symbol '" +
+          acle_se_sym->getName() + "'; undefined symbol");
+    return false;
----------------
`...' is not defined`. `undefined symbol` has a different meaning (not a defined/shared/lazy symbol).


================
Comment at: lld/ELF/Driver.cpp:2399
+// for the non-secure world.
+#define ACLESESYM_PREFIX "__acle_se_"
+
----------------
`const char ...[] = "__acle_se_";`


================
Comment at: lld/ELF/Driver.cpp:2444
+  // If there is an unresolved definition for sym, resolve it using
+  // the definition in the import library
+  for (auto &p : symtab.cmseImportLib) {
----------------



================
Comment at: lld/ELF/InputFiles.cpp:1084
 
+// Initialize this->Symbols. this->Symbols is a parallel array as
+// its corresponding ELF symbol table.
----------------



================
Comment at: lld/ELF/InputFiles.cpp:1099
+  if (firstGlobal == eSyms.size())
+    warn("CMSE import library " + toString(this) +
+         " does not define any symbols");
----------------
This warning doesn't seem useful. Consider removing it to reduce complexity.


================
Comment at: lld/ELF/InputFiles.cpp:1202
     uint8_t binding = eSym.getBinding();
+
     if (LLVM_UNLIKELY(binding != STB_GLOBAL && binding != STB_WEAK &&
----------------
delete blank line


================
Comment at: lld/ELF/LinkerScript.cpp:907
+
+  auto inSectionStart = [&]() {
+    return config->sectionStartMap.find(".gnu.sgstubs") !=
----------------
Why define the immediately-invoked used-once lambda?


================
Comment at: lld/ELF/LinkerScript.cpp:922
+  if (cmdline && linkerscript)
+    warn("addresses assigned to the veneers output section .gnu.sgstubs via "
+         "--section-start and linker script may potentially clash");
----------------
This check isn't right. Just check whether `addrExpr` of the OutputSection is set.


================
Comment at: lld/ELF/SymbolTable.h:63
 
+  void addCmseSymPair(Symbol *acle_se_sym, Symbol *sym);
+  // The list of __acle_se_<sym>, <sym> pairs found in the input objects.
----------------
`acleSeSym`


================
Comment at: lld/ELF/SyntheticSections.h:1223
 
+class ArmCmseSGVeneer : public SyntheticSection {
+public:
----------------
This class can likely be moved to Arch/ARM.cpp


================
Comment at: lld/test/ELF/arm-cmse-check-startaddress.s:1
+// REQUIRES: arm
+// RUN: llvm-mc -arm-add-build-attributes --triple=thumbv8m.main -filetype=obj -I%S/Inputs/ %s -o %t.o
----------------
Add a file-level comment what the test does.


================
Comment at: lld/test/ELF/arm-cmse-check-startaddress.s:3
+// RUN: llvm-mc -arm-add-build-attributes --triple=thumbv8m.main -filetype=obj -I%S/Inputs/ %s -o %t.o
+// RUN: echo "SECTIONS { \
+// RUN:   .text : { *(.text) } \
----------------
See `split-file` in newly added tests or how my recent commits refactored existing tests.


================
Comment at: lld/test/ELF/arm-cmse-check-startaddress.s:11
+// RUN: ld.lld --cmse-implib -Ttext=0x8000 --section-start .gnu.sgstubs=0x20000 --script %twithsgstubs.script %t.o -o %t1 2>&1 | FileCheck %s --check-prefix=WARN
+// RUN: ld.lld --cmse-implib -Ttext=0x8000 --section-start .gnu.sgstubs=0x20000 --script %twoutsgstubs.script %t.o -o %t2 2>&1 | FileCheck %s --check-prefix=NOWARN --check-prefix=NOERROR
+// RUN: ld.lld --cmse-implib -Ttext=0x8000 --section-start .gnu.sgstubs=0x20000 %t.o -o %t3 2>&1 | FileCheck %s --check-prefix=NOWARN --check-prefix=NOERROR
----------------
Use `--check-prefixes=`


================
Comment at: lld/test/ELF/arm-cmse-check-startaddress.s:26
+
+// WARN: warning: addresses assigned to the veneers output section .gnu.sgstubs via --section-start and linker script may potentially clash
+// NOWARN-NOT: warning: addresses assigned to the veneers output section .gnu.sgstubs via --section-start and linker script may potentially clash
----------------
Newer tests tend to place CHECK prefixes before the assembly.


================
Comment at: lld/test/ELF/arm-cmse-check-startaddress.s:30
+// ERROR: error: no address assigned to the veneers output section .gnu.sgstubs
+// NOERROR-NOT: error: no address assigned to the veneers output section .gnu.sgstubs
+
----------------
`NOERROR-NOT: error:` is stronger than this check prefix which checks that there is no other type of errors.


================
Comment at: lld/test/ELF/arm-cmse-check-startaddress.s:32
+
+// ADDRCMDLINE:      Section {
+// ADDRCMDLINE:        Name: .gnu.sgstubs
----------------
Prefer `llvm-readelf -S` to `llvm-readobj -S` for new tests.


================
Comment at: lld/test/ELF/arm-cmse-implib.s:8
+// RUN: llvm-mc -arm-add-build-attributes -filetype=obj --triple=thumbv8m.base %S/Inputs/arm-cmse-implib-2.s -I %S/Inputs -o %t2.o
+// RUN: ld.lld -Ttext=0x8000 --section-start .gnu.sgstubs=0x20000 -o %t2 --out-implib=%t2.lib --in-implib=%t1.lib --cmse-implib %t2.o
+// RUN: llvm-readelf -s %t2 %t2.lib | FileCheck %s --check-prefix=CHECK2
----------------
There is a warning: `CMSE symbol no_veneer1 in import library a1.lib does not have correct size of 8 bytes`

For the main functionality test, avoid such warnings.


================
Comment at: lld/test/ELF/arm-cmse-warn-empty-implib.s:1
+// REQUIRES: arm
+// RUN: llvm-mc -arm-add-build-attributes -filetype=obj --triple=thumbv8m.main %s -o %t.o
----------------
Many `test/ELF/arm-*` tests are old and may not be a good reference. In newer tests we prefer merging such error/warning tests into the main functional test file. Use `split-file`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139092