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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

I've not finished checking yet, but thought it worth sending the comments that I have so far. Only thing that I can spot that looks wrong is that CMSE is M-profile only so the attributes check looks like it needs adjusting. The remainder are nit level comments.



================
Comment at: lld/ELF/Arch/ARM.cpp:916
 
+// Initialize symbols. symbols is a parallel array as its corresponding ELF
+// symbol table.
----------------
I know the language is taken from InputFiles.cpp, but I think it would be worth correcting here. I suggest
`symbols is a parall array to the corresponding Elf symbol table.`


================
Comment at: lld/ELF/Arch/ARM.cpp:995
+// Both these symbols are Thumb function symbols with external linkage.
+// <sym> may be redefined in .gnu.sgstubs.
+void elf::processArmCmseSymbol() {
----------------
The function is called process, but it is not clear without reading the code what processing is done. I think it is doing two things:
* Record the `[__acle_se_<sym>, <sym>]` pair for later processing.
* Resolve any undefined symbols against the definitions in the CMSE Import Lib.

Would be good to make that explicit in the comment. 


================
Comment at: lld/ELF/Arch/ARM.cpp:996
+// <sym> may be redefined in .gnu.sgstubs.
+void elf::processArmCmseSymbol() {
+  if (!config->cmseImplib)
----------------
Should be plural `processArmCmseSymbols()` as this processes more than one symbol.


================
Comment at: lld/ELF/Driver.cpp:2746
+  // Process CMSE symbols.
+  processArmCmseSymbol();
+
----------------
Should be plural `processArmCmseSymbols()`


================
Comment at: lld/ELF/InputFiles.cpp:181
+
+  // Only ARMv8-M or later architectures have CMSE support.
+  if (arch >= ARMBuildAttrs::CPUArch::v8_M_Base)
----------------
The check below is a bit too loose as it will also include the A profile CPU arch > v8_M_Base as they are in the same tag. I think you'll need (pseudo code) && CPU_arch_profile = M 


================
Comment at: lld/ELF/SyntheticSections.h:1183
+  void exportEntries(SymbolTableBaseSection *symTab);
+  uint64_t impLibMinAddr = -1;
+  uint64_t impLibMaxAddr = 0;
----------------
Is impLibMinAddr used? I can see it being assigned to, but can't see it being used for anything.


================
Comment at: lld/test/ELF/arm-cmse-diagnostics.s:36
+
+// RUN: llvm-mc -arm-add-build-attributes -filetype=obj -triple=thumbv9a app -I %S/Inputs -o app4.o
+// RUN: ld.lld --cmse-implib -Ttext=0x8000 --section-start .gnu.sgstubs=0x20000 -o app4 app4.o 2>&1 | FileCheck /dev/null --implicit-check-not=error:
----------------
Thumbv9a doesn't have CMSE, only M-profile has CMSE.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139092