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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

About tests: the addresses and raw bytes just make test updates difficult in case we want to change the default image base. The existing `arm-*` may not the best references in `lld/test/ELF`.

It's sufficient to have addresses only on the line below the label line, e.g.

  // RUN: llvm-objdump -d --no-show-raw-bytes %tsecureapp | FileCheck %s --check-prefix=SECUREDIS
  ...
  
  // SECUREDIS-LABEL: <entry>:
  // SECUREDIS-NEXT:    20000:   sg
  // SECUREDIS-NEXT:             b.w {{.*}} <__acle_se_entry>

When `-d` sufficies, don't use `-D`.

You can find some of my test cleanup changes: `git log -S no-show-raw-insn --author=maskray -- lld/test/ELF`



================
Comment at: lld/ELF/Arch/ARM.cpp:1038
+    Symbol *sym = symtab.find(name);
+
+    if (!sym) {
----------------
delete blank line


================
Comment at: lld/ELF/Driver.cpp:1680
+      if (!armCmseImpLib.empty())
+        error("multiple CMSE import libraries not supported");
+      else if (std::optional<MemoryBufferRef> mb = readFile(arg->getValue()))
----------------
If only one is supported, use `std::optional<` instead of SmallVector/std::vector.

`SmallVector<*, 0>` is generally preferred in a struct/class.


================
Comment at: lld/test/ELF/arm-cmse-check-startaddress.s:6
+// REQUIRES: arm
+// RUN: split-file %s %t
+
----------------
`// RUN: rm -rf %t && split-file %s %t && cd %t`

Then you may replace `%txxx.o` with `xxx.o` below, omitting the `%t` prefix.


================
Comment at: lld/test/ELF/arm-cmse-secure.s:1
+// REQUIRES: arm
+// RUN: split-file %s %t
----------------
Add a file-level comment immediately below `REUIQRES: arm` using `///`. It's unclear what the file tests.


================
Comment at: lld/test/ELF/arm-cmse-secure.s:15
+// SECUREDISS:      00020000 <entry>:
+// SECUREDISS-NEXT:    20000: e97f e97f      sg
+// SECUREDISS-NEXT:    20004: f000 b800      b.w  0x20008 <__acle_se_entry>
----------------
The addresses and raw bytes just make test updates difficult in case we want to change the default image base.

It's sufficient to have addresses only on the line below the label line, e.g.

```
// RUN: llvm-objdump -d --no-show-raw-bytes %tsecureapp | FileCheck %s --check-prefix=SECUREDIS
...

// SECUREDIS-LABEL: <entry>:
// SECUREDIS-NEXT:    20000:   sg
// SECUREDIS-NEXT:             b.w {{.*}} <__acle_se_entry>
```

You can find some of my test cleanup changes: `git log -S no-show-raw-insn --author=maskray -- lld/test/ELF`


================
Comment at: lld/test/ELF/arm-cmse-secure.s:62
+
+// CHECK:      Symbol table '.symtab' contains 2 entries:
+// CHECK-NEXT:    Num:    Value  Size Type    Bind   Vis       Ndx Name
----------------
Move the CHECK lines below `NONSECUREDISS`. It's awkward to have the comments part of the file `cmse-non-secure-app.s`


================
Comment at: lld/test/ELF/arm-cmse-veneers.s:1
+// REQUIRES: arm
+// RUN: llvm-mc -arm-add-build-attributes -filetype=obj --triple=thumbv8m.main %s -I %S/Inputs -o %t.o
----------------
Add a file-level comment immediately below `REUIQRES: arm` using `///`. It's unclear what the file tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139092