[lld-macho] Set the SG_READ_ONLY flag on __DATA_CONST

[BertalanD]

Summary

This flag instructs dyld to mark the segment read-only after fixups have
been performed.

I'm not sure why this flag is needed, as on macOS 13 beta at least,
__DATA_CONST is read-only even without this flag, but ld64 sets it as
well.

[int3]

lgtm

llvm/tools/llvm-objdump/MachODump.cpp

8862

I assume by "this inconsistency" you mean the inclusion of SG_ prefix in the output? Would be nice to spell it out, took me a sec :)

[thakis]

lg

lld/MachO/OutputSegment.cpp

48

ld64 checks this:

	// but not for shared cache dylibs (because __objc_const is not read-only)
	return ( fUseDataConstSegment && (strcmp(name, "__DATA_CONST") == 0) && (!fSharedRegionEligible || (fOutputKind == Options::kDyld)) );

I think we only end up with dataConst segments if config->dataConst (the first check) (…or if users use that segment name explicitly, but then they kind of ask for this treatment, so that seems fine).

We don't implement neither -dylinker nor -add_split_seg_info / -not_for_dyld_shared_cache (not sure why the positive and negative forms of that have different names…), and it's unlikely we'll add them (seems they're only needed for the linker used to link the OS), so we don't really have to worry about the second part. But maybe this could grow a comment around the lines of "If we ever implement shared cache output support, don't set this for shared cache dylibs".

(Or not. As-is is fine too, up to you.)

llvm/tools/llvm-objdump/MachODump.cpp

8862

+1, took me a bit too.

[BertalanD]

Improved comments.

lld/MachO/OutputSegment.h

public:
   uint64_t fileOff = 0;
   uint64_t fileSize = 0;
   uint64_t addr = 0;
   uint64_t vmSize = 0;
   int inputOrder = UnspecifiedInputOrder;
   StringRef name;
   uint32_t maxProt = 0;
   uint32_t initProt = 0;
+  uint32_t flags = 0;
   uint8_t index;
 
   llvm::TinyPtrVector<Defined *> segmentStartSymbols;
   llvm::TinyPtrVector<Defined *> segmentEndSymbols;
 
 private:
   std::vector<OutputSection *> sections;
 };

lld/MachO/OutputSegment.cpp

 }
 
 static uint32_t maxProt(StringRef name) {
   assert(config->arch() != AK_i386 &&
          "TODO: i386 has different maxProt requirements");
   return initProt(name);
 }
 
+static uint32_t flags(StringRef name) {
+  // If we ever implement shared cache output support, SG_READ_ONLY should not
+  // be used for dylibs that can be placed in it.
+  return name == segment_names::dataConst ? SG_READ_ONLY : 0;
+}
+
 size_t OutputSegment::numNonHiddenSections() const {
   size_t count = 0;
   for (const OutputSection *osec : sections)
     count += (!osec->isHidden() ? 1 : 0);
   return count;
 }
 
 void OutputSegment::addOutputSection(OutputSection *osec) {
OutputSegment *macho::getOrCreateOutputSegment(StringRef name) {
   OutputSegment *&segRef = nameToOutputSegment[name];
   if (segRef)
     return segRef;
 
   segRef = make<OutputSegment>();
   segRef->name = name;
   segRef->maxProt = maxProt(name);
   segRef->initProt = initProt(name);
+  segRef->flags = flags(name);
 
   outputSegments.push_back(segRef);
   return segRef;
 }

lld/MachO/Writer.cpp

void writeTo(uint8_t *buf) const override {
     c->fileoff = seg->fileOff;
     c->maxprot = seg->maxProt;
     c->initprot = seg->initProt;
 
     c->vmaddr = seg->addr;
     c->vmsize = seg->vmSize;
     c->filesize = seg->fileSize;
     c->nsects = seg->numNonHiddenSections();
+    c->flags = seg->flags;
 
     for (const OutputSection *osec : seg->getSections()) {
       if (osec->isHidden())
         continue;
 
       auto *sectHdr = reinterpret_cast<SectionHeader *>(buf);
       buf += sizeof(SectionHeader);
 

lld/test/MachO/builtin-rename.s

 # YDATA-DAG: __DATA_CONST,__mod_term_func __DATA__mod_term_func
 # YDATA-DAG: __DATA_CONST,__objc_classlist __DATA__objc_classlist
 # YDATA-DAG: __DATA_CONST,__objc_nlclslist __DATA__objc_nlclslist
 # YDATA-DAG: __DATA_CONST,__objc_catlist __DATA__objc_catlist
 # YDATA-DAG: __DATA_CONST,__objc_nlcatlist __DATA__objc_nlcatlist
 # YDATA-DAG: __DATA_CONST,__objc_protolist __DATA__objc_protolist
 # YDATA-DAG: __DATA_CONST,__nl_symbol_ptr __IMPORT__pointers
 
+## Check that the SG_READ_ONLY flag is set on __DATA_CONST.
+# RUN: llvm-otool -v -l %t/ydata | \
+# RUN:     FileCheck %s --check-prefix=FLAGS
+
+# FLAGS-LABEL: Load command 2
+# FLAGS-NEXT:      cmd LC_SEGMENT_64
+# FLAGS-NEXT:  cmdsize
+# FLAGS-NEXT:  segname __DATA_CONST
+# FLAGS-NEXT:   vmaddr
+# FLAGS-NEXT:   vmsize
+# FLAGS-NEXT:  fileoff
+# FLAGS-NEXT: filesize
+# FLAGS-NEXT:  maxprot rw-
+# FLAGS-NEXT: initprot rw-
+# FLAGS-NEXT:   nsects 13
+# FLAGS-NEXT:    flags SG_READ_ONLY
+
 ## LLD doesn't support defining symbols in synthetic sections, so we test them
 ## via this slightly more awkward route.
 # RUN: llvm-readobj --section-headers %t/ydata | \
 # RUN:     FileCheck %s --check-prefix=SYNTH -DSEGNAME=__DATA_CONST
 # RUN: llvm-readobj --section-headers %t/ndata | \
 # RUN:     FileCheck %s --check-prefix=SYNTH -DSEGNAME=__DATA
 # RUN: llvm-readobj --section-headers %t/nopie | \
 # RUN:     FileCheck %s --check-prefix=SYNTH -DSEGNAME=__DATA

llvm/include/llvm/BinaryFormat/MachO.h

 #undef HANDLE_LOAD_COMMAND
 
 enum : uint32_t {
   // Constant bits for the "flags" field in llvm::MachO::segment_command
   SG_HIGHVM = 0x1u,
   SG_FVMLIB = 0x2u,
   SG_NORELOC = 0x4u,
   SG_PROTECTED_VERSION_1 = 0x8u,
+  SG_READ_ONLY = 0x10u,
 
   // Constant masks for the "flags" field in llvm::MachO::section and
   // llvm::MachO::section_64
   SECTION_TYPE = 0x000000ffu,           // SECTION_TYPE
   SECTION_ATTRIBUTES = 0xffffff00u,     // SECTION_ATTRIBUTES
   SECTION_ATTRIBUTES_USR = 0xff000000u, // SECTION_ATTRIBUTES_USR
   SECTION_ATTRIBUTES_SYS = 0x00ffff00u  // SECTION_ATTRIBUTES_SYS
 };

llvm/tools/llvm-objdump/MachODump.cpp

else {
       if (flags & MachO::SG_NORELOC) {
         outs() << " NORELOC";
         flags &= ~MachO::SG_NORELOC;
       }
       if (flags & MachO::SG_PROTECTED_VERSION_1) {
         outs() << " PROTECTED_VERSION_1";
         flags &= ~MachO::SG_PROTECTED_VERSION_1;
       }
+      if (flags & MachO::SG_READ_ONLY) {
+        // Apple's otool prints the SG_ prefix for this flag, but not for the
+        // others.
+        outs() << " SG_READ_ONLY";
+        flags &= ~MachO::SG_READ_ONLY;
+      }
       if (flags)
         outs() << format(" 0x%08" PRIx32, flags) << " (unknown flags)\n";
       else
         outs() << "\n";
     }
   } else {
     outs() << "    flags " << format("0x%" PRIx32, flags) << "\n";
   }