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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

Thanks for posting this I've got about half way through. I'll need to re-read the specification as well. Will comment more as I have them.

Will be worth a motivating example in the description, for example a simple secure state offering a single function, with the non-secure state accessing it via the import library. Will also be useful to show the extension of the secure-state API with another function while maintining the address of the previous function. This might help people reading the description.



================
Comment at: lld/ELF/Arch/ARM.cpp:916
 
+template <typename ELFT> void elf::writeARMCmseImportLib() {
+  if (config->emachine != EM_ARM)
----------------
Would be good to have a summary comment of what a CMSE import lib is, and what the function has to do. Possibly worth a link to the specification as well.


================
Comment at: lld/ELF/Arch/ARM.cpp:940
+  OutputSection *elfHeader1 = make<OutputSection>(symTab->name, 0, 0);
+  OutputSection *elfHeader2 = make<OutputSection>(strTab->name, 0, 0);
+  OutputSection *elfHeader3 = make<OutputSection>(shStrTab->name, 0, 0);
----------------
IIUC we are writing an ELF file with just 4 sections, each with one input section of the same name. Perhaps use a prefix or suffix of `Is` and `os` to distinguish the two?


================
Comment at: lld/ELF/Arch/ARM.cpp:1011
+  //
+  // The ELF header can only store numbers up to SHN_LORESERVE in the e_shnum
+  // and e_shstrndx fields. When the value of one of these fields exceeds
----------------
In this case it looks like we are only ever writing 4 sections, the import lib is really just a symbol table. We'll not need the SHN_XINDEX part.


================
Comment at: lld/ELF/Arch/ARM.cpp:1019
+  auto *sHdrs = reinterpret_cast<typename ELFT::Shdr *>(buf + eHdr->e_shoff);
+  // TODO: eHdr->e_shnum = outputSections.size() + 1 gives warning;
+  // unable to get the associated symbol table for SHT_SYMTAB_SHNDX section with
----------------
I don't think you'll ever need a SHT_SYMTAB_SHNDX section for the import lib as we know ahead of time that the number of sections is fewer than the limit. Can you remove it? 


================
Comment at: lld/ELF/Arch/ARM.cpp:1029
+
+  // Write section contents to a mmap'ed file.
+  parallel::TaskGroup tg;
----------------
Would this be simpler if done in series? In other words is it worth multithreading for just 4 relatively small sections?


================
Comment at: lld/ELF/Config.h:169
   llvm::StringRef whyExtract;
+  llvm::StringRef in_implib;
+  llvm::StringRef out_implib;
----------------
LLD tends not to use underscores in names. I recommend using CMSE in the name as implib is quite general and CMSE is very specific.


================
Comment at: lld/ELF/InputFiles.cpp:1044
+#define ACLESESYM_PREFIX_LEN 10
+bool isACLESESymbolName(llvm::StringRef name) {
+  return name.startswith(ACLESESYM_PREFIX);
----------------
These look like they are only used within this source file. Can they be made static?


================
Comment at: lld/ELF/InputFiles.cpp:1183
 
+static void processArmCmseSymbol(Symbol &sym, uint32_t secIdx) {
+  if (!(config->emachine == EM_ARM))
----------------
Will be worth a brief comment saying what this function is supposed to do. It saves having to look up the specification.


================
Comment at: lld/ELF/InputFiles.cpp:1184
+static void processArmCmseSymbol(Symbol &sym, uint32_t secIdx) {
+  if (!(config->emachine == EM_ARM))
+    return;
----------------
Is this sufficient? For example should we check from the BuildAttributes that the architecture is at least v8-M?


================
Comment at: lld/ELF/InputFiles.cpp:1191
+  StringRef ext_name = getNameFromACLESE(sym.getName());
+  // Try to find the associated symbol. Search only in global symbols
+  // because the specification requires to produce output library with
----------------
Suggest `Try to find the associated symbol definition. Symbol must have external linkage.`

Symbol visibility has a specific meaning that is best not to confuse with binding.


================
Comment at: lld/ELF/InputFiles.cpp:1217
+
+  // Validate both symbols.
+  if (!(d_sym.isFunc() && (d_sym.value & 0x1))) {
----------------
Suggest `// Check that both symbols are Thumb symbols of type STT_FUNC.`


================
Comment at: lld/ELF/SymbolTable.h:63
 
+  bool addCmseSymPair(Symbol *acle_sg_sym, Symbol *sym);
+  llvm::MapVector<Symbol *, Symbol *> cmseSymMap;
----------------
Comment to describe these CMSE specific containers.


================
Comment at: lld/ELF/SyntheticSections.h:1241
+// specified in the CMSE import library.
+// The symbol table in the import library should have a size
+// specified
----------------
The symbol table entry, or the symbol from the import library


================
Comment at: lld/ELF/SyntheticSections.h:1243
+// specified
+class ArmCmseFixedAddressSGVeneer : public ArmCmseSGVeneer {
+public:
----------------
You might be able to forwared declar ArmCmseSGVeneer (you only use it by name in the Vector), and move the class definitions to the .cpp file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139092