This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by grimar on Jan 20 2017, 7:25 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Jan 20 2017, 7:25 AM
ruiu edited edge metadata.Jan 20 2017, 8:03 AM

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.

ruiu added a comment.Jan 20 2017, 8:17 AM

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.)

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 added a comment.Jan 20 2017, 8:31 AM

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 added a subscriber: emaste.Jan 20 2017, 8:41 AM

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 updated this revision to Diff 85152.Jan 20 2017, 9:53 AM
  • 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 added a subscriber: silvas.Jan 20 2017, 12:39 PM

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 added a subscriber: davide.Jan 20 2017, 12:48 PM

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

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)

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 accepted this revision.Jan 21 2017, 2:54 PM

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.
This revision is now accepted and ready to land.Jan 21 2017, 2:54 PM
grimar edited the summary of this revision. (Show Details)Jan 23 2017, 1:46 AM
This revision was automatically updated to reflect the committed changes.