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

[Amilendra Kodithuwakku via Phabricator via llvm-commits]

amilendra marked 44 inline comments as done.
amilendra added inline comments.


================
Comment at: lld/ELF/Arch/ARM.cpp:940
+  OutputSection *elfHeader1 = make<OutputSection>(symTab->name, 0, 0);
+  OutputSection *elfHeader2 = make<OutputSection>(strTab->name, 0, 0);
+  OutputSection *elfHeader3 = make<OutputSection>(shStrTab->name, 0, 0);
----------------
peter.smith wrote:
> IIUC we are writing an ELF file with just 4 sections, each with one input section of the same name. Perhaps use a prefix or suffix of `Is` and `os` to distinguish the two?
I've refactored to use OutputSection, InputSection pairs which hopefully makes the intentions clearer.


================
Comment at: lld/ELF/Arch/ARM.cpp:1029
+
+  // Write section contents to a mmap'ed file.
+  parallel::TaskGroup tg;
----------------
peter.smith wrote:
> Would this be simpler if done in series? In other words is it worth multithreading for just 4 relatively small sections?
IIUC the `OutputSection::writeTo(uint8_t *buf, parallel::TaskGroup &tg)` interface only supports multi-threaded calls?


================
Comment at: lld/ELF/Arch/ARM.cpp:1030
+  // Write section contents to a mmap'ed file.
+  parallel::TaskGroup tg;
+
----------------
MaskRay wrote:
> TaskGroup's destructor ensures the tasks are done. It must finish before `buffer->commit` is called.
> 
> ```
> {
>   parallel::TaskGroup tg;
>   for (...)
>     xxx->writeTo<ELFT>(...);
> }
> ```
Thanks. This was the reason my tests were segfaulting during multi-threaded runs.


================
Comment at: lld/ELF/InputFiles.cpp:1184
+static void processArmCmseSymbol(Symbol &sym, uint32_t secIdx) {
+  if (!(config->emachine == EM_ARM))
+    return;
----------------
peter.smith wrote:
> Is this sufficient? For example should we check from the BuildAttributes that the architecture is at least v8-M?
Thanks. Added checks for BuildAttributes.


================
Comment at: lld/ELF/SyntheticSections.cpp:2218
+  auto SI = symtab.cmseImportLib.find(sym->getName());
+  if (SI == symtab.cmseImportLib.end()) {
+    ArmCmseVariableAddressSGVeneer *ss =
----------------
MaskRay wrote:
> Use `try_emplace` and the pair result to know whether the value has been inserted. Avoid code duplication.
IIUC, `try_emplace` inserts to `symtab.cmseImportLib` if it the key not already present?


================
Comment at: lld/ELF/SyntheticSections.h:1222
 };
+class ArmCmseSGVeneer : public SyntheticSection {
+public:
----------------
MaskRay wrote:
> blank line above
> 
> use final
`ArmCmseSGVeneer` is sub-classed further as `ArmCmseFixedAddressSGVeneer` and `ArmCmseVariableAddressSGVeneer`. Added `final` to those classes.


================
Comment at: lld/ELF/SyntheticSections.h:1243
+// specified
+class ArmCmseFixedAddressSGVeneer : public ArmCmseSGVeneer {
+public:
----------------
peter.smith wrote:
> You might be able to forwared declar ArmCmseSGVeneer (you only use it by name in the Vector), and move the class definitions to the .cpp file.
I was not able to do this because the subclasses refer to `enum Kind` defined in `ArmCmseSGVeneer`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139092