[PATCH] D113771: [ELF] Support the "read-only" memory region attribute

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

Overall looks good to me. Some small suggestions for comments based on my thoughts when trying to reason about the change. These are only suggestions though, no strong opinions.



================
Comment at: lld/ELF/LinkerScript.cpp:919
   for (auto &pair : memoryRegions) {
     MemoryRegion *m = pair.second;
+    if (((m->flags & sec->flags) || (m->invFlags & ~sec->flags)) &&
----------------
Just a thought, could it be worth adding a member function to MemoryRegion, say compatibleWith(flags) which we could move this calculation into. That would put the calculation next to the comments that explain what the fields are. We'd then have if (m->compatibleWith(sec->flags)) here.


================
Comment at: lld/ELF/LinkerScript.h:142
   Expr length;
+  // A section can be assigned to the region if any of these flags are set...
   uint32_t flags;
----------------
Could be worth saying `ELF section flags` instead of just `flags` to distinguish between Memory attibutes and Section flags. 


================
Comment at: lld/ELF/LinkerScript.h:145
+  // ... or any of these flags are not set.
+  uint32_t invFlags;
+  // A section cannot be assigned to the region if any of these flags are set...
----------------
Could be worth an example.
// ... or any of these flags are not set. For example the linker script `r` maps to ~SHF_WRITE.


================
Comment at: lld/ELF/LinkerScript.h:149
+  // ... or any of these flags are not set.
+  uint32_t negInvFlags;
   uint64_t curPos = 0;
----------------
Suggest
for example the linker script !r maps to !(~SHF_WRITE).


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

https://reviews.llvm.org/D113771