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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Arch/ARM.cpp:970
+static std::string checkCmseSymAttributes(Symbol *acleSeSym, Symbol *sym) {
+  auto isNotDefined = [](Symbol *s,
+                         StringRef type) -> std::optional<std::string> {
----------------
Merge isNotDefined/isNotThumbFunc/isAbsolute into one lambda.


================
Comment at: lld/ELF/Arch/ARM.cpp:1004
+
+  for (const auto &[sym, type] :
+       {std::make_pair(acleSeSym, "special"), std::make_pair(sym, "entry")})
----------------



================
Comment at: lld/ELF/Arch/ARM.cpp:1095
+void ArmCmseSGSection::addSGVeneer(Symbol *acleSeSym, Symbol *sym) {
+  entries.push_back(std::make_pair(acleSeSym, sym));
+  if (acleSeSym->file != sym->file ||
----------------
Use `emplace_back`


================
Comment at: lld/ELF/Arch/ARM.cpp:1098
+      cast<Defined>(*acleSeSym).value != cast<Defined>(*sym).value) {
+    // Symbol addresses different, nothing to do.
+    return;
----------------
Move the comment before `if (acleSeSym->file != sym->file ||` and delete braces


================
Comment at: lld/ELF/Arch/ARM.cpp:1127
+    return (impLibMaxAddr ? impLibMaxAddr - getVA() : 0) +
+           (newEntries * entsize);
+
----------------



================
Comment at: lld/ELF/Arch/ARM.cpp:1133
+void ArmCmseSGSection::finalizeContents() {
+  if (this->sgSections.empty())
+    return;
----------------
`this->` can be removed. Applies elsewhere.


================
Comment at: lld/ELF/Arch/ARM.cpp:1138
+      this->sgSections.begin(), this->sgSections.end(),
+      [](auto *i) { return i->getAddr() != std::nullopt; });
+
----------------
Use `.has_value()` if you want to emphasize it is a `std::optional`.


================
Comment at: lld/ELF/Arch/ARM.cpp:1139
+      [](auto *i) { return i->getAddr() != std::nullopt; });
+
+  std::sort(this->sgSections.begin(), it, [](auto *a, auto *b) {
----------------
Delete blank line


================
Comment at: lld/ELF/Arch/ARM.cpp:1183
+  uint64_t s = acleSeSym->getVA();
+  uint64_t p = getVA() + 4;
+  target->relocateNoSym(buf + 4, R_ARM_THM_JUMP24, s - p - 4);
----------------
It's unclear what the `+4` for `p` is and what the 4 in `s - p - 4` is.

Perhaps just combine them into `s - getVA() - 8` below.


================
Comment at: lld/ELF/Arch/ARM.cpp:1231
+
+  for (auto &[osec, isec] : osIsPairs) {
+    osec->size = isec->getSize();
----------------
Why not merge the two `osIsPairs` loops?


================
Comment at: lld/ELF/Arch/ARM.cpp:1243
+
+  uint64_t sectionHeaderOff = alignToPowerOf2(off, config->wordsize);
+  auto shnum = osIsPairs.size() + 1;
----------------
Remove used-once variable or add `const` if the use site(s) are far away.


================
Comment at: lld/ELF/Arch/ARM.cpp:1248
+  unlinkAsync(config->cmseOutputLib);
+  unsigned flags = 0;
+  if (!config->mmapOutputFile)
----------------
`unsigned flags = config->mmapOutputFile ? 0 : (unsigned)FileOutputBuffer::F_no_mmap;`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139092