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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

A few more comments on some things that don't look right compared to the GNU ld interface.



================
Comment at: lld/ELF/Arch/ARM.cpp:1034
+
+  // If there is an unresolved definition for sym, resolve it using
+  // the definition in the import library.
----------------
I don't think this is needed. The non-secure app doesn't use `--cmse-implib` or `--in-implib` it just uses the raw object file produced by `--out-implib`. 

If we do end up matching an undefined in the importlib from the secure application then something has gone badly wrong.  


================
Comment at: lld/test/ELF/arm-cmse-secure.s:12
+/// Link the non-secure app against the import library.
+// RUN: ld.lld -e nonsecure_entry -Ttext=0x8000 --in-implib=implib.lib --cmse-implib nonsecureapp.o -o nonsecureapp
+// RUN: llvm-readelf -s implib.lib | FileCheck %s
----------------
The non-secure app doesn't use `--in-implib` and `--cmse-implib` it just uses implib.lib which is just a standard relocatable object. It doesn't actually need to know whether the definitions in the implib are CMSE related or not.


================
Comment at: lld/test/ELF/arm-cmse-secure.s:25
+// SECUREDISS-LABEL: <secure_entry>:
+// SECUREDISS-NEXT:    2000c: bl
+// SECUREDISS-NEXT:           bx lr
----------------
What is the destination of this `bl`? it should be `bl __acle_se_entry` the secure state should not use the import library. The import library is for the use of non-secure state.

Will be useful to add a test case where we take the address of `_entry` which should be crystallised as `__acle_entry`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139092