[ELF] - Linkerscripts: ignore CONSTRUCTORS in output section declaration.

[grimar]

Summary

It is used in linux kernel script:
http://lxr.free-electrons.com/source/arch/x86/kernel/vmlinux.lds.S#L140

Though CONSTRUCTORS is ignored for ELF.

[ruiu]

Can you give me a pointer to verify that the directive is ignored?

ELF/LinkerScript.cpp

1453–1454 ↗

(On Diff #85134)

We have a lot of things we silently ignore, and I don't find this is special, so no need to print out a message for that.

[grimar]

In D28951#651581, @ruiu wrote:

Can you give me a pointer to verify that the directive is ignored?

http://code.metager.de/source/xref/gnu/src/gold/yyscript.y#523

https://sourceware.org/binutils/docs-2.26/ld/Output-Section-Keywords.html

[ruiu]

As I said, we need to write readable code so that even the first-time readers can understand it. If you were a stranger who happens to read this piece of code, you would wonder what this directive is and why it is ignored. Please write comments to describe why you are doing this. (I'm not saying that this needs a lengthy comment, but writing zero comments is not ok.)

[grimar]

In D28951#651624, @ruiu wrote:

As I said, we need to write readable code so that even the first-time readers can understand it. If you were a stranger who happens to read this piece of code, you would wonder what this directive is and why it is ignored. Please write comments to describe why you are doing this. (I'm not saying that this needs a lengthy comment, but writing zero comments is not ok.)

Original patch did not require comments because had elf::log with clear message. I'll add comment and remove elf::log.

[ruiu]

That error message was not a replacement for comments because it didn't answer what CONSTRUCTOR is. Is it a new feature that our linker does not support yet, or is it for a bug compatibility, or for compatibility with other object file formats? Don't you wonder that kind of things if you just read your code as a third person?

You should mention that CONSTRUCTORS is for the a.out format which died out in '90s in major Unix systems but we need to accept it for compatibility.

[emaste]

FYI the FreeBSD fix for this is here: https://svnweb.freebsd.org/changeset/base/303442

The linker script CONSTRUCTORS keyword is only meaningful "when linking
object file formats which do not support arbitrary sections, such as
ECOFF and XCOFF"[1] and is ignored for other object file formats.

LLVM's lld does not yet accept (and ignore) CONSTRUCTORS, so just remove
CONSTRUCTORS from the linker scripts as it has no effect.

[1] https://sourceware.org/binutils/docs/ld/Output-Section-Keywords.html

[grimar]

• Addressed review comments.

ELF/LinkerScript.cpp

1453–1454 ↗

(On Diff #85134)

If honestly I would emit warning for this and other depricated things if any. I know we probably never will. Though I believe world requires some cleanup. I would be happy to cleanup my 25 years old scripts If I would have them :)
Otherwise one day somebody can end up with some NewSuperLLD and had to add tons of things it should ignore because some people did not restrict to have trash in scripts hundred years ago.

[silvas]

Are you sure that this CONSTRUCTORS is trying to be a linker keyword? The nearby DATA_DATA is actually a macro expanded from http://src.illumos.org/source/xref/linux-master/include/asm-generic/vmlinux.lds.h#205
Is the CONSTRUCTORS still in file after it has been preprocessed? (I'm guessing so since otherwise this patch wouldn't fix the build, but just wanted to be sure).
Remember to keep in mind that these linker scripts all get preprocessed by the C preprocessor, so remember to look at the preprocessed linker script instead of the original source.

[davide]

In D28951#651895, @silvas wrote:

Are you sure that this CONSTRUCTORS is trying to be a linker keyword? The nearby DATA_DATA is actually a macro expanded from http://src.illumos.org/source/xref/linux-master/include/asm-generic/vmlinux.lds.h#205
Is the CONSTRUCTORS still in file after it has been preprocessed? (I'm guessing so since otherwise this patch wouldn't fix the build, but just wanted to be sure).
Remember to keep in mind that these linker scripts all get preprocessed by the C preprocessor, so remember to look at the preprocessed linker script instead of the original source.

Yeah, CONSTRUCTORS is a linker command https://www.sourceware.org/binutils/docs-2.10/ld_3.html#SEC29

[davide]

Also funny they still have that in their linker script as it seems that the semantic action associated for that on ELF is none (and I'll be surprised if linux runs on non-ELF, FWIW)

[grimar]

In D28951#651895, @silvas wrote:

Are you sure that this CONSTRUCTORS is trying to be a linker keyword? The nearby DATA_DATA is actually a macro expanded from http://src.illumos.org/source/xref/linux-master/include/asm-generic/vmlinux.lds.h#205
Is the CONSTRUCTORS still in file after it has been preprocessed? (I'm guessing so since otherwise this patch wouldn't fix the build, but just wanted to be sure).
Remember to keep in mind that these linker scripts all get preprocessed by the C preprocessor, so remember to look at the preprocessed linker script instead of the original source.

Yes, I am aware this could be macros, but it is not in this case. I verified this when wrote the patch.
Here is a part of script from description after processing I had locally:
(arch/x86/kernel/vmlinux.lds for me)

.data : AT(ADDR(.data) - 0xffffffff80000000) {
 /* Start of data section */
 _sdata = .;
 /* init_task */
 . = ALIGN(((1 << 12) << (2 + 0))); __start_init_task = .; *(.data..init_task) __end_init_task = .;
 . = ALIGN((1 << 12)); *(.data..page_aligned)
 . = ALIGN((1 << (6))); *(.data..cacheline_aligned)
 *(.data .data.[0-9a-zA-Z_]*) *(.ref.data) *(.data..shared_aligned) *(.meminit.data) *(.memexit.data) *(.data.unlikely) . = ALIGN(32); *(__tracepoints) . = ALIGN(8); __start___jump_table = .; KEEP(*(__jump_table)) __stop___jump_table = .; . = ALIGN(8); __start___verbose = .; KEEP(*(__verbose)) __stop___verbose = .; __start___trace_bprintk_fmt = .; KEEP(*(__trace_printk_fmt)) __stop___trace_bprintk_fmt = .; __start___tracepoint_str = .; KEEP(*(__tracepoint_str)) __stop___tracepoint_str = .;
 CONSTRUCTORS
 /* rarely changed data like cpu maps */
 . = ALIGN((1 << 6)); *(.data..read_mostly) . = ALIGN((1 << 6));
 /* End of data section */
 _edata = .;
} :data

[ruiu]

LGTM

ELF/LinkerScript.cpp

1453–1454 ↗

(On Diff #85152)

// CONSTRUCTORS is a keyword to make the linker recognize C++ ctors/dtors
// by name. This is for very old file formats such as ECOFF/XCOFF.
// For ELF, we should ignore.

lld/trunk/ELF/LinkerScript.cpp

while (!Error && !consume("}")) {
     StringRef Tok = next();
     if (SymbolAssignment *Assignment = readProvideOrAssignment(Tok)) {
       Cmd->Commands.emplace_back(Assignment);
     } else if (BytesDataCommand *Data = readBytesDataCommand(Tok)) {
       Cmd->Commands.emplace_back(Data);
     } else if (Tok == "ASSERT") {
       Cmd->Commands.emplace_back(new AssertCommand(readAssert()));
       expect(";");
+    } else if (Tok == "CONSTRUCTORS") {
+      // CONSTRUCTORS is a keyword to make the linker recognize C++ ctors/dtors
+      // by name. This is for very old file formats such as ECOFF/XCOFF.
+      // For ELF, we should ignore.
     } else if (Tok == "FILL") {
       Cmd->Filler = readFill();
     } else if (Tok == "SORT") {
       readSort();
     } else if (peek() == "(") {
       Cmd->Commands.emplace_back(readInputSectionDescription(Tok));
     } else {
       setError("unknown command " + Tok);

lld/trunk/test/ELF/linkerscript/constructor.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
+# RUN: echo "SECTIONS { foo : { *(.foo) CONSTRUCTORS } }" > %t.script
+
+# RUN: llvm-objdump -section-headers %t1 | FileCheck %s
+# CHECK:      Sections:
+# CHECK-NEXT: Idx Name          Size
+# CHECK-NEXT:   0               00000000
+# CHECK-NEXT:   1 foo           00000001
+
+.section foo, "a"
+.byte 0