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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

Some suggestions for simplification.



================
Comment at: lld/ELF/Arch/ARM.cpp:1335
+
+  for (ArmCmseSGVeneer *s : sgSections) {
+    if (!s->getAddr().has_value())
----------------
IIUC at this point we have two partions:
P1
ordered by address existing implib veneers
P2
reverse ordered new veneers

I think you could simplify the code by using `std::reverse(it, sgSections.end());` on the second partition rather than traversing it via llvm::reverse()

Then I think you would only need one much simpler loop. 
```
for (size_t i; i < sgSections.size() ++i) {
  ArmCmseSGVeneer* s = sgSections[i];
  s->offset = i * s->size();
  Defined(file, StringRef(), s->sym->binding, s->sym->stOther, s->sym->type,
            s->offset | 1, s->size(), this)
        .overwrite(*s->sym);
}
```


================
Comment at: lld/ELF/SyntheticSections.h:1151
+
+class ArmCmseSGVeneer {
+public:
----------------
Can the definition be moved into ARM.cpp? In ArmCMSESGVeneerSection the type is only used by name so we should be able to forward declare it here?


================
Comment at: lld/ELF/SyntheticSections.h:1156
+      : sym(sym), acleSeSym(acleSeSym), entAddr{addr} {}
+  size_t size() const { return ACLESESYM_SIZE; };
+  const std::optional<uint64_t> getAddr() const { return entAddr; };
----------------
If this is no longer a SyntheticSection you can make it a static const size_t as all SG veneers are the same size.


================
Comment at: lld/ELF/SyntheticSections.h:1181
+  SmallVector<std::pair<Symbol *, Symbol *>, 0> entries;
+  SmallVector<ArmCmseSGVeneer *, 0> sgSections;
+  uint64_t newEntries = 0;
----------------
As these are no longer sections perhaps worth calling them sgVeneers?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139092