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

[ikudrin]

Summary

The attribute r allows (or disallows for the negative case) read-only sections, i.e. ones without the SHF_WRITE flag, to be assigned to the memory region. Before the patch, lld could put a section in the wrong region or fail with "error: no memory region specified for section".

[peter.smith]

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.

lld/ELF/LinkerScript.cpp

979

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.

lld/ELF/LinkerScript.h

142

Could be worth saying ELF section flags instead of just flags to distinguish between Memory attibutes and Section flags.

146

Could be worth an example.
// ... or any of these flags are not set. For example the linker script r maps to ~SHF_WRITE.

152

Suggest
for example the linker script !r maps to !(~SHF_WRITE).

[ikudrin]

Thanks for the suggestions!

• Added method MemoryRegion::compatibleWith()
• Updated comments

[peter.smith]

Thanks for the update and sorry for the delay in responding. LGTM. Will be worth waiting a few days before committing to see if @MaskRay has any comments.

[MaskRay]

The attribute allows (or disallows for the negative case) read-only sections, i.e. ones without the SHF_WRITE flag, to be assigned to the memory region.

- i.e. ones without the SHF_WRITE flag
+ i.e. ones without the SHF_WRITE flag (`r`)

lld/test/ELF/linkerscript/memory-readonly.test

1 ↗

(On Diff #387602)

Suggest renaming this to memory-neg-attr.test

We can use this file to test all negative (!) attributes, not just readonly.

30 ↗

(On Diff #387602)

Optional: can you figure out another memory region to test an attribute other than !r in the same test?

[MaskRay]

lld/ELF/LinkerScript.h

146

The ~ notation in ~SHF_WRITE may be ambiguous. It is not the bitwise operation in regular C++ programs.

Since the comment applies to invFlags, I think ~ can be omitted.

Ditto for negInvFlags below.

lld/test/ELF/linkerscript/memory-readonly.test

11 ↗

(On Diff #387602)

12 ↗

(On Diff #387602)

13 ↗

(On Diff #387602)

14 ↗

(On Diff #387602)

[ikudrin]

Thanks for the suggestions!

• Updated comments
• memory-readonly.test -> memory-attr.test
• Added tests for all supported attributes

lld/test/ELF/linkerscript/memory-readonly.test

1 ↗

(On Diff #387602)

The test checks both negative and positive cases, so I guess memory-attr.test will fit it better.

30 ↗

(On Diff #387602)

I'll check all four supported attributes in the updated test.

lld/ELF/LinkerScript.h

 // with ONLY_IF_RW is created if all input sections are RW.
 enum class ConstraintKind { NoConstraint, ReadOnly, ReadWrite };
 
 // This struct is used to represent the location and size of regions of
 // target memory. Instances of the struct are created by parsing the
 // MEMORY command.
 struct MemoryRegion {
   MemoryRegion(StringRef name, Expr origin, Expr length, uint32_t flags,
-               uint32_t negFlags)
+               uint32_t invFlags, uint32_t negFlags, uint32_t negInvFlags)
       : name(std::string(name)), origin(origin), length(length), flags(flags),
-        negFlags(negFlags) {}
+        invFlags(invFlags), negFlags(negFlags), negInvFlags(negInvFlags) {}
 
   std::string name;
   Expr origin;
   Expr length;
+  // A section can be assigned to the region if any of these ELF section flags
+  // are set...
   uint32_t flags;
+  // ... or any of these flags are not set.
+  // For example, the memory region attribute "r" maps to SHF_WRITE.
+  uint32_t invFlags;
+  // A section cannot be assigned to the region if any of these ELF section
+  // flags are set...
   uint32_t negFlags;
+  // ... or any of these flags are not set.
+  // For example, the memory region attribute "!r" maps to SHF_WRITE.
+  uint32_t negInvFlags;
   uint64_t curPos = 0;
+
+  bool compatibleWith(uint32_t secFlags) const {
+    if ((secFlags & negFlags) || (~secFlags & negInvFlags))
+      return false;
+    return (secFlags & flags) || (~secFlags & invFlags);
+  }
 };
 
 // This struct represents one section match pattern in SECTIONS() command.
 // It can optionally have negative match pattern for EXCLUDED_FILE command.
 // Also it may be surrounded with SORT() command, so contains sorting rules.
 class SectionPattern {
   StringMatcher excludedFilePat;
 

lld/ELF/LinkerScript.cpp

if (memoryRegions.empty())
     return {nullptr, nullptr};
 
   // An orphan section should continue the previous memory region.
   if (sec->sectionIndex == UINT32_MAX && hint)
     return {hint, hint};
 
   // See if a region can be found by matching section flags.
   for (auto &pair : memoryRegions) {
     MemoryRegion *m = pair.second;
-    if ((m->flags & sec->flags) && (m->negFlags & sec->flags) == 0)
+    if (m->compatibleWith(sec->flags))
       return {m, nullptr};
   }
 
   // Otherwise, no suitable region was found.
   error("no memory region specified for section '" + sec->name + "'");
   return {nullptr, nullptr};
 }
 

lld/ELF/ScriptParser.cpp

private:
   SymbolAssignment *readProvideHidden(bool provide, bool hidden);
   SymbolAssignment *readAssignment(StringRef tok);
   void readSort();
   Expr readAssert();
   Expr readConstant();
   Expr getPageSize();
 
   Expr readMemoryAssignment(StringRef, StringRef, StringRef);
-  std::pair<uint32_t, uint32_t> readMemoryAttributes();
+  void readMemoryAttributes(uint32_t &flags, uint32_t &invFlags,
+                            uint32_t &negFlags, uint32_t &negInvFlags);
 
   Expr combine(StringRef op, Expr l, Expr r);
   Expr readExpr();
   Expr readExpr1(Expr lhs, int minPrec);
   StringRef readParenLiteral();
   Expr readPrimary();
   Expr readTernary(Expr cond);
   Expr readParenExpr();
void ScriptParser::readMemory() {
   while (!errorCount() && !consume("}")) {
     StringRef tok = next();
     if (tok == "INCLUDE") {
       readInclude();
       continue;
     }
 
     uint32_t flags = 0;
+    uint32_t invFlags = 0;
     uint32_t negFlags = 0;
+    uint32_t negInvFlags = 0;
     if (consume("(")) {
-      std::tie(flags, negFlags) = readMemoryAttributes();
+      readMemoryAttributes(flags, invFlags, negFlags, negInvFlags);
       expect(")");
     }
     expect(":");
 
     Expr origin = readMemoryAssignment("ORIGIN", "org", "o");
     expect(",");
     Expr length = readMemoryAssignment("LENGTH", "len", "l");
 
     // Add the memory region to the region map.
-    MemoryRegion *mr = make<MemoryRegion>(tok, origin, length, flags, negFlags);
+    MemoryRegion *mr = make<MemoryRegion>(tok, origin, length, flags, invFlags,
+                                          negFlags, negInvFlags);
     if (!script->memoryRegions.insert({tok, mr}).second)
       setError("region '" + tok + "' already defined");
   }
 }
 
 // This function parses the attributes used to match against section
 // flags when placing output sections in a memory region. These flags
 // are only used when an explicit memory region name is not used.
-std::pair<uint32_t, uint32_t> ScriptParser::readMemoryAttributes() {
-  uint32_t flags = 0;
-  uint32_t negFlags = 0;
+void ScriptParser::readMemoryAttributes(uint32_t &flags, uint32_t &invFlags,
+                                        uint32_t &negFlags,
+                                        uint32_t &negInvFlags) {
   bool invert = false;
 
   for (char c : next().lower()) {
-    uint32_t flag = 0;
-    if (c == '!')
+    if (c == '!') {
       invert = !invert;
-    else if (c == 'w')
-      flag = SHF_WRITE;
+      std::swap(flags, negFlags);
+      std::swap(invFlags, negInvFlags);
+      continue;
+    }
+    if (c == 'w')
+      flags |= SHF_WRITE;
     else if (c == 'x')
-      flag = SHF_EXECINSTR;
+      flags |= SHF_EXECINSTR;
     else if (c == 'a')
-      flag = SHF_ALLOC;
-    else if (c != 'r')
+      flags |= SHF_ALLOC;
+    else if (c == 'r')
+      invFlags |= SHF_WRITE;
+    else
       setError("invalid memory region attribute");
+  }
 
-    if (invert)
-      negFlags |= flag;
-    else
-      flags |= flag;
+  if (invert) {
+    std::swap(flags, negFlags);
+    std::swap(invFlags, negInvFlags);
   }
-  return {flags, negFlags};
 }
 
 void elf::readLinkerScript(MemoryBufferRef mb) {
   llvm::TimeTraceScope timeScope("Read linker script",
                                  mb.getBufferIdentifier());
   ScriptParser(mb).readLinkerScript();
 }
 

lld/test/ELF/linkerscript/memory-attr.test

+REQUIRES: x86
+
+RUN: split-file %s %ts
+RUN: llvm-mc -filetype=obj -triple=x86_64 %ts/s -o %t.o
+
+## Check assigning sections to memory regions by attributes.
+
+#--- s
+  .text
+  .zero 8
+
+  .rodata
+  .zero 8
+
+  .data
+  .zero 8
+
+#--- t1
+## Check memory region attribute 'a'.
+
+# RUN: ld.lld -o %t1 -T %ts/t1 %t.o
+# RUN: llvm-readelf -S %t1 | FileCheck %s --check-prefix=TEST1
+
+# TEST1:      Name     Type      Address
+# TEST1:      .text    PROGBITS  0000000000002000
+# TEST1-NEXT: .rodata  PROGBITS  0000000000002008
+# TEST1-NEXT: .data    PROGBITS  0000000000002010
+
+MEMORY
+{
+  ## All sections have SHF_ALLOC attribute, so no one can be added here.
+  NOMEM (rwx!a) : org = 0x1000, l = 1K
+  ## All sections are assigned to this memory region.
+  MEM   (a)     : org = 0x2000, l = 1K
+}
+
+SECTIONS
+{
+  .text   : { *(.text) }
+  .rodata : { *(.rodata) }
+  .data   : { *(.data) }
+}
+
+#--- t2
+## Check that memory region attributes 'r', 'w', and 'x' are supported both in
+## positive and negative forms.
+
+# RUN: ld.lld -o %t2 -T %ts/t2 %t.o
+# RUN: llvm-readelf -S %t2 | FileCheck %s --check-prefix=TEST2
+
+# TEST2:      Name     Type      Address
+# TEST2:      .text    PROGBITS  0000000000004000
+# TEST2-NEXT: .rodata  PROGBITS  0000000000003000
+# TEST2-NEXT: .data    PROGBITS  0000000000002000
+
+MEMORY
+{
+  ## While all sections are allocatable, '.text' and '.rodata' are read-only and
+  ## '.data' is writable, so no sections should be assigned to this region.
+  NOMEM (a!rw) : org = 0x1000, l = 1K
+  ## Only writable section '.data' is allowed here.
+  RAM   (w)    : org = 0x2000, l = 1K
+  ## Executable sections are not allowed here, so only '.rodata' should be
+  ## assigned to the region.
+  ROM   (r!x)  : org = 0x3000, l = 1K
+  ## An executable section '.text' comes here.
+  EXEC  (x)    : org = 0x4000, l = 1K
+}
+
+SECTIONS
+{
+  .text   : { *(.text) }
+  .rodata : { *(.rodata) }
+  .data   : { *(.data) }
+}