[PATCH] D139092: [LLD][ELF] Cortex-M Security Extensions (CMSE) Support
Fangrui Song via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Feb 23 13:18:37 PST 2023
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
More information about the llvm-commits
mailing list