[PATCH] D133010: [lld-macho] Set the SG_READ_ONLY flag on __DATA_CONST

[Nico Weber via Phabricator via llvm-commits]

thakis accepted this revision.
thakis added a comment.

lg



================
Comment at: lld/MachO/OutputSegment.cpp:48
+static uint32_t flags(StringRef name) {
+  return name == segment_names::dataConst ? SG_READ_ONLY : 0;
+}
----------------
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.)


================
Comment at: llvm/tools/llvm-objdump/MachODump.cpp:8862
+      if (flags & MachO::SG_READ_ONLY) {
+        // Note: Apple's otool has this inconsistency as well.
+        outs() << " SG_READ_ONLY";
----------------
int3 wrote:
> 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 :)
+1, took me a bit too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133010