[lld][ELF] support READONLY in linker script section

[bluca]

Summary

bfd recently added a READONLY attribute for sections.
Support the keyword in lld too, but note that it's a no-op here,
as sections are already read-only by default, but it's needed to
allow the same linker scripts to be used.

https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=6b86da53d5ee2022b9065f445d23356190380746

[MaskRay]

bfd recently added a READONLY attribute for sections, as they are created writable by default.

I think saying it writable is incorrect (see https://sourceware.org/bugzilla/show_bug.cgi?id=26378). See https://sourceware.org/pipermail/binutils/2021-May/116579.html

I mentioned that READONLY is not needed but it seems that my comment has been ignored.

[bluca]

In D118491#3280927, @MaskRay wrote:

bfd recently added a READONLY attribute for sections, as they are created writable by default.

I think saying it writable is incorrect (see https://sourceware.org/bugzilla/show_bug.cgi?id=26378). See https://sourceware.org/pipermail/binutils/2021-May/116579.html

I mentioned that READONLY is not needed but it seems that my comment has been ignored.

There might be cases/combination where that is not the case, but with the provided linker script, before that patch, the note section was definitely created as writable by bfd, and there was no way to mark it read-only from the script.

[bluca]

In D118491#3280965, @bluca wrote:

In D118491#3280927, @MaskRay wrote:

bfd recently added a READONLY attribute for sections, as they are created writable by default.

I think saying it writable is incorrect (see https://sourceware.org/bugzilla/show_bug.cgi?id=26378). See https://sourceware.org/pipermail/binutils/2021-May/116579.html

I mentioned that READONLY is not needed but it seems that my comment has been ignored.

There might be cases/combination where that is not the case, but with the provided linker script, before that patch, the note section was definitely created as writable by bfd, and there was no way to mark it read-only from the script.

But I don't really mind what the commit message says, so if you want that changed, that's fine: removed that sentence in the edit.

[MaskRay]

In D118491#3280965, @bluca wrote:

In D118491#3280927, @MaskRay wrote:

bfd recently added a READONLY attribute for sections, as they are created writable by default.

I think saying it writable is incorrect (see https://sourceware.org/bugzilla/show_bug.cgi?id=26378). See https://sourceware.org/pipermail/binutils/2021-May/116579.html

I mentioned that READONLY is not needed but it seems that my comment has been ignored.

There might be cases/combination where that is not the case, but with the provided linker script, before that patch, the note section was definitely created as writable by bfd, and there was no way to mark it read-only from the script.

Sorry, I do not follow. .note.package : ALIGN(4) { ... } does not produce a writable output section in GNU ld.
That's the bug fixed by https://sourceware.org/bugzilla/show_bug.cgi?id=26378 (2020-09), before READONLY was added.

If we want to customize both sh_type and sh_flags, I think the proper way is to add syntax to specify both.

(I know that ld.lld does not set any flag if you don't have a regular input section. There are quite a few cases in GNU ld. It's just overwhelming to support every one.)

[bluca]

In D118491#3281023, @MaskRay wrote:

In D118491#3280965, @bluca wrote:

In D118491#3280927, @MaskRay wrote:

bfd recently added a READONLY attribute for sections, as they are created writable by default.

I think saying it writable is incorrect (see https://sourceware.org/bugzilla/show_bug.cgi?id=26378). See https://sourceware.org/pipermail/binutils/2021-May/116579.html

I mentioned that READONLY is not needed but it seems that my comment has been ignored.

There might be cases/combination where that is not the case, but with the provided linker script, before that patch, the note section was definitely created as writable by bfd, and there was no way to mark it read-only from the script.

Sorry, I do not follow. .note.package : ALIGN(4) { ... } does not produce a writable output section in GNU ld.
That's the bug fixed by https://sourceware.org/bugzilla/show_bug.cgi?id=26378 (2020-09), before READONLY was added.

Without readonly:

[ 3] .note.package     NOTE             00000000000002e8  000002e8
     0000000000000030  0000000000000000  WA       0     0     4

If we want to customize both sh_type and sh_flags, I think the proper way is to add syntax to specify both.

(I know that ld.lld does not set any flag if you don't have a regular input section. There are quite a few cases in GNU ld. It's just overwhelming to support every one.)

That's what this patch does, no? It adds support for the same option.

[MaskRay]

In D118491#3281073, @bluca wrote:

In D118491#3281023, @MaskRay wrote:

In D118491#3280965, @bluca wrote:

In D118491#3280927, @MaskRay wrote:

bfd recently added a READONLY attribute for sections, as they are created writable by default.

I think saying it writable is incorrect (see https://sourceware.org/bugzilla/show_bug.cgi?id=26378). See https://sourceware.org/pipermail/binutils/2021-May/116579.html

I mentioned that READONLY is not needed but it seems that my comment has been ignored.

There might be cases/combination where that is not the case, but with the provided linker script, before that patch, the note section was definitely created as writable by bfd, and there was no way to mark it read-only from the script.

Sorry, I do not follow. .note.package : ALIGN(4) { ... } does not produce a writable output section in GNU ld.
That's the bug fixed by https://sourceware.org/bugzilla/show_bug.cgi?id=26378 (2020-09), before READONLY was added.

Without readonly:

[ 3] .note.package     NOTE             00000000000002e8  000002e8
     0000000000000030  0000000000000000  WA       0     0     4

If we want to customize both sh_type and sh_flags, I think the proper way is to add syntax to specify both.

(I know that ld.lld does not set any flag if you don't have a regular input section. There are quite a few cases in GNU ld. It's just overwhelming to support every one.)

That's what this patch does, no? It adds support for the same option.

The main request is whether READONLY is necessary.

I can send a patch to add SHF_ALLOC for an empty output section but I'd like to observe GNU ld's behavior more closely first.

[bluca]

In D118491#3281093, @MaskRay wrote:

In D118491#3281073, @bluca wrote:

In D118491#3281023, @MaskRay wrote:

In D118491#3280965, @bluca wrote:

In D118491#3280927, @MaskRay wrote:

bfd recently added a READONLY attribute for sections, as they are created writable by default.

I think saying it writable is incorrect (see https://sourceware.org/bugzilla/show_bug.cgi?id=26378). See https://sourceware.org/pipermail/binutils/2021-May/116579.html

I mentioned that READONLY is not needed but it seems that my comment has been ignored.

There might be cases/combination where that is not the case, but with the provided linker script, before that patch, the note section was definitely created as writable by bfd, and there was no way to mark it read-only from the script.

Sorry, I do not follow. .note.package : ALIGN(4) { ... } does not produce a writable output section in GNU ld.
That's the bug fixed by https://sourceware.org/bugzilla/show_bug.cgi?id=26378 (2020-09), before READONLY was added.

Without readonly:

[ 3] .note.package     NOTE             00000000000002e8  000002e8
     0000000000000030  0000000000000000  WA       0     0     4

If we want to customize both sh_type and sh_flags, I think the proper way is to add syntax to specify both.

(I know that ld.lld does not set any flag if you don't have a regular input section. There are quite a few cases in GNU ld. It's just overwhelming to support every one.)

That's what this patch does, no? It adds support for the same option.

The main request is whether READONLY is necessary.

I can send a patch to add SHF_ALLOC for an empty output section but I'd like to observe GNU ld's behavior more closely first.

It is necessary because currently lld aborts with an error if it finds it in a script. This patch fixes the problem.

[MaskRay]

In D118491#3281099, @bluca wrote:

The main request is whether READONLY is necessary.

I can send a patch to add SHF_ALLOC for an empty output section but I'd like to observe GNU ld's behavior more closely first.

It is necessary because currently lld aborts with an error if it finds it in a script. This patch fixes the problem.

READONLY papers over the problem and works for your case, but I don't think that is appropriate place to change.

The problem is that with INSERT [BEFORE|AFTER], the section order is fuzzy in LinkerScript::adjustSectionsBeforeSorting if (isEmpty).
I'll need to think how to support the case more appropriately.

The high-level problem is whether READONLY should be added.
In https://sourceware.org/pipermail/binutils/2021-May/116579.html I mentioned that it is not needed.
I keep my opinion that this is not needed to achieve your goal, even with the linker script I tend to grumble about.

In addition, READONLY should be orthogonal to SHF_ALLOC. I don't think READONLY should make a section SHF_ALLOC.

If you maintain the relevant systemd package, I'd hope that you re-check whether it is needed. If it is needed, it more likely exposes a GNU ld issue which should be reported instead.

[bluca]

In D118491#3281374, @MaskRay wrote:

In D118491#3281099, @bluca wrote:

The main request is whether READONLY is necessary.

I can send a patch to add SHF_ALLOC for an empty output section but I'd like to observe GNU ld's behavior more closely first.

It is necessary because currently lld aborts with an error if it finds it in a script. This patch fixes the problem.

READONLY papers over the problem and works for your case, but I don't think that is appropriate place to change.

The problem is that with INSERT [BEFORE|AFTER], the section order is fuzzy in LinkerScript::adjustSectionsBeforeSorting if (isEmpty).
I'll need to think how to support the case more appropriately.

The high-level problem is whether READONLY should be added.
In https://sourceware.org/pipermail/binutils/2021-May/116579.html I mentioned that it is not needed.
I keep my opinion that this is not needed to achieve your goal, even with the linker script I tend to grumble about.

It is not needed to make the section read-only in lld (it is needed for source-level compatibility of scripts, which is the reason I sent this patch, as lld curently aborts with an error when specifying a script that is valid for bfd), but is is needed in bfd. Without it the section is writable:

[ 3] .note.package NOTE 00000000000002e8 000002e8

0000000000000030  0000000000000000  WA       0     0     4

In addition, READONLY should be orthogonal to SHF_ALLOC. I don't think READONLY should make a section SHF_ALLOC.

Sure, one doesn't imply the other, and my patches for lld don't do that, so it should be all good.

If you maintain the relevant systemd package, I'd hope that you re-check whether it is needed. If it is needed, it more likely exposes a GNU ld issue which should be reported instead.

I do maintain them, and yes, it is needed. The build-id note gets the read-only flag as expected before/after the changes.

[MaskRay]

https://sourceware.org/pipermail/binutils/2022-February/119627.html

Michael: That's what I meant with ill-advised: a feature was added to work-around
a bug, instead of the bug being fixed. The bug now is fixed and the
feature is useless but needs to be kept for backward comp, except maybe
it could be removed again if the few users could be convinced to just not
use it anymore; so could you be convinced? :)

lld/ELF/ScriptParser.cpp

 }
 
 // Tries to read the special directive for an output section definition which
-// can be one of following: "(NOLOAD)", "(COPY)", "(INFO)" or "(OVERLAY)".
-// Tok1 and Tok2 are next 2 tokens peeked. See comment for readSectionAddressType below.
+// can be one of following: "(NOLOAD)", "(COPY)", "(INFO)", "(OVERLAY)" or
+// "(READONLY)". Tok1 and Tok2 are next 2 tokens peeked. See comment for
+// readSectionAddressType below.
 bool ScriptParser::readSectionDirective(OutputSection *cmd, StringRef tok1, StringRef tok2) {
   if (tok1 != "(")
     return false;
-  if (tok2 != "NOLOAD" && tok2 != "COPY" && tok2 != "INFO" && tok2 != "OVERLAY")
+  if (tok2 != "NOLOAD" && tok2 != "COPY" && tok2 != "INFO" &&
+      tok2 != "OVERLAY" && tok2 != "READONLY")
     return false;
 
   expect("(");
   if (consume("NOLOAD")) {
     cmd->noload = true;
     cmd->type = SHT_NOBITS;
-  } else {
+  } else if (!consume("READONLY")) {
+    // READONLY is supported by bfd, but we add sections as RO by default, so
+    // it's a no-op needed to let users run the same linker scripts
     skip(); // This is "COPY", "INFO" or "OVERLAY".
     cmd->nonAlloc = true;
   }

lld/test/ELF/linkerscript/note-section.test

 
 SECTIONS
 {
-    .note.package : ALIGN(4) {
+    .note.package (READONLY) : ALIGN(4) {
         BYTE(0x04) BYTE(0x00) BYTE(0x00) BYTE(0x00) /* Length of Owner including NUL */
         BYTE(0x0e) BYTE(0x00) BYTE(0x00) BYTE(0x00) /* Length of Value including NUL */
         BYTE(0xaa) BYTE(0xaa) BYTE(0xaa) BYTE(0xaa) /* Note ID */