[lld][ELF] support nested special directives in output sections

[bluca]

Summary

bfd recently added support for nesting special directives as:
(FOO (BAR = BAZ)). Support this syntax in lld too for source input
compatibility, and simply skip over unknown directives.

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

[bluca]

One last attempt at making lld input-compatible with documented bfd features. I am willing to add (TYPE=SHT_NOTE) to the package notes metadata script even if it's only for the benefit of llvm and is not useful anywhere else, but only if the syntax is fully compatible with documented bfd syntax.

[MaskRay]

In D120045#3329242, @bluca wrote:

Please answer why (READONLY) has to be used: https://sourceware.org/pipermail/binutils/2022-February/119599.html
It's not only my complaint, but also Machael's. (I would say Alan would likely disagree as well but he usually avoids that if Nick does something.)

One last attempt at making lld input-compatible with documented bfd features. I am willing to add (TYPE=SHT_NOTE) to the package notes metadata script even if it's only for the benefit of llvm and is not useful anywhere else, but only if the syntax is fully compatible with documented bfd syntax.

I complained about READONLY (https://sourceware.org/pipermail/binutils/2022-February/119796.html)

---

And a question which may render all these discussions unnecessary, why an assembler produced object file cannot be used: https://sourceware.org/pipermail/binutils/2022-February/119592.html
My feeling is that these questions were just dismissed with "Compiled object do not work and cannot possibly work sanely in this context." without a concrete example....

[bluca]

All of that has been answered multiple times already, and I have no intention of wasting any more time on it given you are not willing to listen, nor to understand what it means to ship a distribution. I made a final good faith attempt, and put my unpaid time to work for the benefit of google, by testing and providing this patch, and by showing I am willing to adapt the setup to work around these compatibility bugs of LLVM. The ball now is your court - either fix the problems, using the provided patch or an equivalent (not looking for credit or anything), and then we can make it work, or don't, and keep LLVM incompatible with the default setup of any project which uses the package note specification. The choice is yours.

[MaskRay]

In D120045#3330497, @bluca wrote:

All of that has been answered multiple times already, and I have no intention of wasting any more time on it given you are not willing to listen, nor to understand what it means to ship a distribution. I made a final good faith attempt, and put my unpaid time to work for the benefit of google, by testing and providing this patch, and by showing I am willing to adapt the setup to work around these compatibility bugs of LLVM. The ball now is your court - either fix the problems, using the provided patch or an equivalent (not looking for credit or anything), and then we can make it work, or don't, and keep LLVM incompatible with the default setup of any project which uses the package note specification. The choice is yours.

When you said "If you have an alternative solution to make a linker script create a read-only note section to be inserted after another section we'd love to hear it. So far we haven't found any, but it's possible we missed something, of course.",
i'd love to hear which package doesn't work this way.
Rui Ueyama feels strange (a linker script solution won't work with mold). Cary Coutant may be annoyed by (the .note* = SHT_NOTE thing) (gold doesn't have it, like ld.lld).
Adhemerval Zanella and Andrew Kelly may feel strange, too.

And why READONLY is needed is never explained. You said:

If you have an alternative solution to make a linker script create a read-only note section

Well, just don't use (READONLY). The section is readonly even without (READONLY). Alan's 2020 binutils fix is sufficient. If not, open a new bug on binutils. Don't add more stuff to READONLY.

"work for the benefit of google"

Google doesn't use the feature. I added the (TYPE=...) syntax for the sole purpose that Fedora or systemd (https://github.com/systemd/package-notes) has a proper way to mark .note* sections SHT_NOTE.
If one llvm-project commit of mine can benefit my company, I can argue it's my work; otherwise it's not. Maintaining lld/ELF is unpaid.
I have a ton of internal work. Perhaps 90+% of my llvm-project commits are entirely unrelated.

[bluca]

In D120045#3330562, @MaskRay wrote:

I added the (TYPE=...) syntax for the sole purpose that Fedora or systemd (https://github.com/systemd/package-notes) has a proper way to mark .note* sections SHT_NOTE.

And unless this fix or something along those lines is included too, it will not be used and it will serve no purpose whatsoever. Note how this patch doesn't mention _any specific directive_ but it simply skips any unknown ones. See it as future proofing, and compatibility with the documented bfd format. It's not going to get any closer to what you want - take it or leave it.

[MaskRay]

In D120045#3330873, @bluca wrote:

In D120045#3330562, @MaskRay wrote:

I added the (TYPE=...) syntax for the sole purpose that Fedora or systemd (https://github.com/systemd/package-notes) has a proper way to mark .note* sections SHT_NOTE.

And unless this fix or something along those lines is included too, it will not be used and it will serve no purpose whatsoever. Note how this patch doesn't mention _any specific directive_ but it simply skips any unknown ones. See it as future proofing, and compatibility with the documented bfd format. It's not going to get any closer to what you want - take it or leave it.

Some keywords have been supported. A new keyword likely introduces an interesting enough semantic difference (e.g. changing sh_flags, sh_link, etc) we should not silently ignore.
Ignore the unknown keyword like FOOBAR may cause a silently broken binary.
Catching the error is more important than the compatibility with older linkers.

In addition, the (type) syntax is actually ambiguous: it is overloaded with the output section address syntax. Ignoring the keyword will cause trouble if the user intends to use a non-keyword variable in a pair of parentheses for the address.

---

I filed https://github.com/systemd/package-notes/issues/28 to remove (READONLY).

[bluca]

In D120045#3330921, @MaskRay wrote:

In D120045#3330873, @bluca wrote:

In D120045#3330562, @MaskRay wrote:

I added the (TYPE=...) syntax for the sole purpose that Fedora or systemd (https://github.com/systemd/package-notes) has a proper way to mark .note* sections SHT_NOTE.

And unless this fix or something along those lines is included too, it will not be used and it will serve no purpose whatsoever. Note how this patch doesn't mention _any specific directive_ but it simply skips any unknown ones. See it as future proofing, and compatibility with the documented bfd format. It's not going to get any closer to what you want - take it or leave it.

Some keywords have been supported. A new keyword likely introduces an interesting enough semantic difference (e.g. changing sh_flags, sh_link, etc) we should not silently ignore.
Ignore the unknown keyword like FOOBAR may cause a silently broken binary.
Catching the error is more important than the compatibility with older linkers.

In addition, the (type) syntax is actually ambiguous: it is overloaded with the output section address syntax. Ignoring the keyword will cause trouble if the user intends to use a non-keyword variable in a pair of parentheses for the address.

How would something that is not supported and was never supported before produce a broken binary? Anyway, I don't really care _how_ you fix it, feel free to come up with a different solution to fix the same incompatibility if you don't like this one

[MaskRay]

In D120045#3330924, @bluca wrote:

In D120045#3330921, @MaskRay wrote:

In D120045#3330873, @bluca wrote:

In D120045#3330562, @MaskRay wrote:

I added the (TYPE=...) syntax for the sole purpose that Fedora or systemd (https://github.com/systemd/package-notes) has a proper way to mark .note* sections SHT_NOTE.

And unless this fix or something along those lines is included too, it will not be used and it will serve no purpose whatsoever. Note how this patch doesn't mention _any specific directive_ but it simply skips any unknown ones. See it as future proofing, and compatibility with the documented bfd format. It's not going to get any closer to what you want - take it or leave it.

Some keywords have been supported. A new keyword likely introduces an interesting enough semantic difference (e.g. changing sh_flags, sh_link, etc) we should not silently ignore.
Ignore the unknown keyword like FOOBAR may cause a silently broken binary.
Catching the error is more important than the compatibility with older linkers.

In addition, the (type) syntax is actually ambiguous: it is overloaded with the output section address syntax. Ignoring the keyword will cause trouble if the user intends to use a non-keyword variable in a pair of parentheses for the address.

How would something that is not supported and was never supported before produce a broken binary? Anyway, I don't really care _how_ you fix it, feel free to come up with a different solution to fix the same incompatibility if you don't like this one

For example, if we introduce new syntax to change sh_flags. Since dynamic loaders don't really read the section header table, mistakes there will not be detected.
If the program does introspection or inspects a relocatable output, the wrong sh_flags may cause a problem.
We have syntax for SHT_NOBITS and arbitrary sh_type now, I am comfortably to state that all new syntax will make interesting semantic differences we should not silently ignore.

The solution Michael Matz, I (likely also Cary Coutant (gold)/Rui Ueyama (mold)) prefer most is to use an assembler. binutils ships GNU assembler and let your tool link the relocatable object file.
I am willing to help you if you can point me to a package which has a problem.

.section .note.package,"a",%note
....

If it really doesn't work (then gold and mold will be excluded):

.note.package (TYPE=SHT_NOTE) : { BYTE(...) BYTE(...) ... }

The READONLY thing is unneeded to drop the SHF_WRITE flag.

[bluca]

In D120045#3330944, @MaskRay wrote:

In D120045#3330924, @bluca wrote:

In D120045#3330921, @MaskRay wrote:

In D120045#3330873, @bluca wrote:

In D120045#3330562, @MaskRay wrote:

I added the (TYPE=...) syntax for the sole purpose that Fedora or systemd (https://github.com/systemd/package-notes) has a proper way to mark .note* sections SHT_NOTE.

And unless this fix or something along those lines is included too, it will not be used and it will serve no purpose whatsoever. Note how this patch doesn't mention _any specific directive_ but it simply skips any unknown ones. See it as future proofing, and compatibility with the documented bfd format. It's not going to get any closer to what you want - take it or leave it.

Some keywords have been supported. A new keyword likely introduces an interesting enough semantic difference (e.g. changing sh_flags, sh_link, etc) we should not silently ignore.
Ignore the unknown keyword like FOOBAR may cause a silently broken binary.
Catching the error is more important than the compatibility with older linkers.

In addition, the (type) syntax is actually ambiguous: it is overloaded with the output section address syntax. Ignoring the keyword will cause trouble if the user intends to use a non-keyword variable in a pair of parentheses for the address.

How would something that is not supported and was never supported before produce a broken binary? Anyway, I don't really care _how_ you fix it, feel free to come up with a different solution to fix the same incompatibility if you don't like this one

For example, if we introduce new syntax to change sh_flags.

There is no such thing though, so that sounds like an excuse to me. But anyway, feel free to come up with a different way to fix this incompatibility in llvm, I don't mind how it looks like or who implements it. Or not, and let llvm be incompatible with the default setup. Regardless, please stop wasting my time with proposals that do not work and that nobody asked you to provide.

lld/ELF/ScriptParser.cpp

 // Tries to read the special directive for an output section definition which
 // can be one of following: "(NOLOAD)", "(COPY)", "(INFO)", "(OVERLAY)", and
 // "(TYPE=<value>)".
+// Also supports nesting special directives as (FOO (BAR)), as ld.bfd does.
+// Unknown directives are simply skipped over.
 // 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" && tok2 != "TYPE")
-    return false;
 
   expect("(");
   if (consume("NOLOAD")) {

       cmd->type = readExpr()().getValue();
     }
     cmd->typeIsSet = true;
-  } else {
-    skip(); // This is "COPY", "INFO" or "OVERLAY".
+  } else if (consume("COPY") || consume("INFO") || consume("OVERLAY")) {
     cmd->nonAlloc = true;
+  } else {
+    skip();
   }
+
+  if (peek() == "(" && !readSectionDirective(cmd, "(", peek2()))
+    return false;
+
   expect(")");
   return true;
 }

lld/test/ELF/linkerscript/custom-section-type.s

 #--- a.lds
 SECTIONS {
   progbits (TYPE=SHT_PROGBITS) : { BYTE(1) }
-  note (TYPE = SHT_NOTE) : { BYTE(7) *(note) }
+  note (FOOBAR (TYPE = SHT_NOTE)) : { BYTE(7) *(note) }
   nobits ( TYPE=SHT_NOBITS) : { BYTE(8) }
   init_array (TYPE=SHT_INIT_ARRAY ) : { QUAD(myinit) }
   fini_array (TYPE=SHT_FINI_ARRAY) : { QUAD(15) }