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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

I think that's all the comments I have for this round.



================
Comment at: lld/ELF/Arch/ARM.cpp:938
+    sym->size = eSym.st_size;
+
+    if (eSym.st_shndx != SHN_ABS) {
----------------
What about the binding? IIUC it is possible to have a weak definition.

Do we need to record the type? Or are these implicitly STT_FUNC so it doesn't matter?


================
Comment at: lld/ELF/Arch/ARM.cpp:1004
+      continue;
+    // If input object build attributes do not support CMSE, error and disable
+    // further scanning for <sym>, __acle_se_<sym> pairs.
----------------
One thing that I don't think this will work for is wrapped secure entry functions. For example in secure state if I have a secure entry function `foo, __acle_foo` and I make another secure entry function `__wrap_foo, `__acle___wrap_foo` then we'll only want to create one SG veneer for foo which calls `__acle_foo` where `__acle_foo` is the renamed `__acle___wrap_foo`. The original `foo, __acle_foo` are renamed to `foo, __acle_foo` are renamed to `__real_foo` and `__acle___real_foo` (in theory `__acle___real_foo` is useless is the wrapped entry function is no longer an entry function.

I think this will need quite a bit of faff to work properly. My suggestion is to search for `__acle___wrap` and error that wrapping secure entry functions is not supported. Could be worth adding support in a follow up patch.

Another thing we could check for is an attempt to wrap a secure entry function with a function that isn't a secure entry point. I.e. we have `foo, __acle_foo` but we only have `__wrap_foo` and no `__acle__wrap_foo`.


================
Comment at: lld/ELF/Arch/ARM.cpp:1060
+    return;
+  std::sort(
+      symtab.cmseSymVector.begin(), symtab.cmseSymVector.end(),
----------------
I don't understand why we are sorting cmseSymVector by name of the non acle prefixed symbol. We partitions and sort the sgSections later on so I don't think the order of cmseSymVector matters? I could be missing something though.
 


================
Comment at: lld/ELF/Arch/ARM.cpp:1068
+    addSGVeneer(acleSeSym, sym);
+}
+
----------------
In the GNU ld implementation it is a warning when:
* The `in_implib` contains an entry for `foo` but there is no `__acle_foo` defined.
* When there is an `in_implib` but no `out_implib` when there is a new entry.

In both case this is an interface change so I do think it is worth warning about.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139092