Page MenuHomeFreeBSD
Differential D17587

Implement a BSD licensed crtbegin/crtend
ClosedPublic
Actions

Authored by andrew on Oct 16 2018, 4:31 PM.
Tags
None
Referenced Files
F110288003: D17587.id49229.diff
Sun, Feb 16, 7:10 AM
Unknown Object (File)
Sat, Jan 25, 3:48 AM
Unknown Object (File)
Fri, Jan 24, 9:36 PM
Unknown Object (File)
Fri, Jan 24, 4:54 AM
Unknown Object (File)
Fri, Jan 24, 3:37 AM
Unknown Object (File)
Thu, Jan 23, 2:13 PM
Unknown Object (File)
Sun, Jan 19, 7:28 AM
Unknown Object (File)
Sun, Jan 19, 4:03 AM
View All Files
Subscribers
arichardson
bdrewery
val_packett.cool

Details

Reviewers
emaste
imp
kib
Commits
rS352394: MFC r339738, r339744, r339770, r339773, r339864-r339866, r339907-r339908…
rS339738: Implement a BSD licensed crtbegin/crtend
Summary

These are needed for .ctors/.dtors and .jcr handling. The former needs
all the function pointers to be called in the correct order from the
.init/.fini section. The latter just needs to call a gcj specific function
if it exists with a pointer to the start of the .jcr section.

Tested on amd64 and arm64, other architectures still need to be
implemented.

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 20404
Build 19844: arc lint + arc unit

Event Timeline

andrew created this revision.Oct 16 2018, 4:31 PM
Harbormaster completed remote builds in B20247: Diff 49229.Oct 16 2018, 4:31 PM
andrew marked an inline comment as done.Oct 16 2018, 4:33 PM
andrew added inline comments.
gnu/lib/Makefile
5–6

This will be fixes before committing

andrew updated this revision to Diff 49238.Oct 16 2018, 5:44 PM

Set the first item in .ctors/.dtors to all 1's

Harbormaster completed remote builds in B20254: Diff 49238.Oct 16 2018, 5:44 PM
andrew added a reviewer: kib.Oct 16 2018, 5:46 PM
kib added inline comments.Oct 17 2018, 11:07 AM
lib/csu/common/crtbegin.c
85

It is better to use .pushsection there

87

... and .popsection there.

That said, why do you put this for amd64 at all ? _NOINIT note is put into the binaries on all arches, meaning that rtld calls init/init_array elements for the dynamic binaries (and crt1 calls them for static),

andrew marked an inline comment as done.Oct 17 2018, 11:49 AM
andrew added inline comments.
lib/csu/common/crtbegin.c
87

I have a test [1] the .ctors & .dtors are run. Without this they aren't. I included this for compatibility with old toolchains that use these sections.

[1] https://github.com/zxombie/initfini

andrew updated this revision to Diff 49250.Oct 17 2018, 12:07 PM

Use .pushsection & .popsection

Harbormaster completed remote builds in B20261: Diff 49250.Oct 17 2018, 12:07 PM
emaste added inline comments.Oct 17 2018, 3:25 PM
lib/csu/common/crtbegin.c
12–14

I think we should avoid having a binary redistribution clause in these components that are linked into every output binary by the toolchain.

andrew marked an inline comment as done.Oct 17 2018, 4:21 PM
andrew added inline comments.
lib/csu/common/crtbegin.c
12–14

We already include it in many other csu files.

andrew updated this revision to Diff 49258.Oct 17 2018, 5:23 PM

Call register_classes from .init. When called from a constructor on ppc64
it will be added to the .ctors section, but before CTOR_LIST so won't
be called.

Harbormaster completed remote builds in B20263: Diff 49258.Oct 17 2018, 5:23 PM
andrew updated this revision to Diff 49259.Oct 17 2018, 5:25 PM

Add the missing Makefile include for powerpc64

Harbormaster completed remote builds in B20264: Diff 49259.Oct 17 2018, 5:25 PM
arichardson added a subscriber: arichardson.Oct 17 2018, 9:07 PM
kib added a comment.Oct 18 2018, 11:05 AM

After some investigation, I see that crtbegin/crtend.o are shipped with compilers, at least it does with gcc. So I do not see a reason to provide the .ctor fallback for e.g. amd64 or i386: in-tree compiler does not need it, and out-of-tree gcc should cope on its own.

That said, not defining HAVE_INITFINI_ARRAY for all current architectures is highly confusing, because all arches in tree currently use init/fini arrays as the method of running user constructors. At least the name needs to be changed.

andrew added a comment.Oct 18 2018, 11:42 AM
In D17587#375744, @kib wrote:

After some investigation, I see that crtbegin/crtend.o are shipped with compilers, at least it does with gcc. So I do not see a reason to provide the .ctor fallback for e.g. amd64 or i386: in-tree compiler does not need it, and out-of-tree gcc should cope on its own.

I don't think it's shipped with clang. Not supporting .ctors could be a problem when linking objects created with different compilers, e.g. a static library built with gcc, but linked through clang.

That said, not defining HAVE_INITFINI_ARRAY for all current architectures is highly confusing, because all arches in tree currently use init/fini arrays as the method of running user constructors. At least the name needs to be changed.

I can change it to a combination of HAVE_CTORS and CTORS_CONSTRUCTORS. The former is if we need to support .ctors, the latter is if the constructor attribute may be using .ctors.

emaste added a comment.Oct 18 2018, 1:31 PM

Not supporting .ctors could be a problem when linking objects created with different compilers, e.g. a static library built with gcc, but linked through clang.

This is precisely the reason for maintaining .ctors/.dtors support; even if GCC uses .init_array/.fini_array on all platforms on FreeBSD now users may have existing .o files which do not.

kib added a comment.Oct 18 2018, 9:40 PM
In D17587#375788, @emaste wrote:

Not supporting .ctors could be a problem when linking objects created with different compilers, e.g. a static library built with gcc, but linked through clang.

This is precisely the reason for maintaining .ctors/.dtors support; even if GCC uses .init_array/.fini_array on all platforms on FreeBSD now users may have existing .o files which do not.

I do not think that it is _reasonable_ to maintain ABI for .o's. and in fact we already do not do that. E.g. we change signatures of the versioned functions, which can be only handled for the completely linked targets.

Linux also does not guarantee ABI stability for the .o, you must recompile if you changed glibc version and intend to relink.

But I do not object against providing the call to .ctors, I am confused by the platforms selection for such compatibility, and by the names (Andrew promised to fix the later).

andrew added a comment.Oct 18 2018, 9:58 PM
In D17587#375979, @kib wrote:

But I do not object against providing the call to .ctors, I am confused by the platforms selection for such compatibility, and by the names (Andrew promised to fix the later).

I've only included the platforms I can test. For the .init section we need the asm for each architecture that uses it, however I don't know this for each, and am unable to test on all platforms. The current set is based on what machines I have access to. I expect to add arm, i386, and some mips, but after that it's difficult for me to test.

emaste added inline comments.Oct 20 2018, 6:37 PM
lib/csu/common/crtbegin.c
12–14

Aha, we already have 1-clause BSD in the tree, we ought to use that.

SPDX link: https://spdx.org/licenses/BSD-1-Clause.html
Their reference: https://svnweb.freebsd.org/base/head/include/ifaddrs.h?revision=326823
Tag: SPDX-License-Identifier: BSD-1-Clause

andrew updated this revision to Diff 49509.Oct 23 2018, 3:40 PM
  • Update the license
  • Add i386 support
  • Add a build option to enable

The build option is enabled on arm64 and powerpc64 as these are lower risc.
I would like more testing on amd64 and i386 (e.g. a ports exp run) before enabling by default there.

Herald added a subscriber: bdrewery. · View Herald TranscriptOct 23 2018, 3:40 PM
Harbormaster completed remote builds in B20355: Diff 49509.Oct 23 2018, 3:40 PM
andrew updated this revision to Diff 49512.Oct 23 2018, 4:18 PM
  • Add $FreeBSD$ to header files
  • Add WITH/WITHOUT descriptions
  • Remove HAVE_INITFINI_ARRAY and replace with HAVE_CTORS and CTORS_CONSTRUCTORS
Harbormaster completed remote builds in B20358: Diff 49512.Oct 23 2018, 4:18 PM
kib accepted this revision.Oct 23 2018, 4:20 PM
kib added inline comments.
lib/csu/common/crtbegin.c
87

There should be \t before instruction/asm pseudo-ops.

This revision is now accepted and ready to land.Oct 23 2018, 4:20 PM
andrew updated this revision to Diff 49513.Oct 23 2018, 4:31 PM
  • Add a tab before instructions in the .init section
  • Fix a logic inversion in an ifdef
This revision now requires review to proceed.Oct 23 2018, 4:31 PM
Harbormaster completed remote builds in B20359: Diff 49513.Oct 23 2018, 4:31 PM
arichardson added a comment.Oct 23 2018, 6:23 PM

Could you add your initfini test as a (ATF?) test here?

emaste added inline comments.Oct 24 2018, 2:33 PM
lib/csu/Makefile.inc
20

We'll want to make sure we test dynamically/statically linked binaries with ctors and exceptions.

andrew updated this revision to Diff 49555.Oct 24 2018, 3:40 PM

Add simple crtbegin/crtend tests

Harbormaster completed remote builds in B20388: Diff 49555.Oct 24 2018, 3:40 PM
emaste added inline comments.Oct 24 2018, 4:32 PM
lib/csu/Makefile
9–10

Is this just the set of hosts where you've been able to validate the tests? Would be nice to enable on all archs (eventually).

lib/csu/common/crtend.c
34

ctors?

tools/build/options/WITHOUT_BSD_CRTBEGIN
3

Should end with a .

andrew updated this revision to Diff 49567.Oct 24 2018, 5:02 PM

Update based on feedback from emaste
Rework how the tests are built

Harbormaster completed remote builds in B20398: Diff 49567.Oct 24 2018, 5:02 PM
emaste added inline comments.Oct 24 2018, 5:05 PM
tools/build/options/WITHOUT_BSD_CRTBEGIN
3

Now I see why you might have omitted the trailing . :)

Perhaps this?

licensed
.Pa crtbegin.o
and
.Pa crtend .o .
andrew updated this revision to Diff 49569.Oct 24 2018, 5:14 PM

Use .Pa

Harbormaster completed remote builds in B20400: Diff 49569.Oct 24 2018, 5:15 PM
andrew marked an inline comment as done.Oct 24 2018, 5:20 PM
andrew added inline comments.
lib/csu/Makefile
9–10

This is the list of platforms with crt.h. I expect the others will have it after we commit this so we can enable the tests everwhere.

andrew updated this revision to Diff 49573.Oct 24 2018, 5:59 PM
  • Add a C++ test
  • Add missing break statements to the default case in the fini switches
Harbormaster completed remote builds in B20404: Diff 49573.Oct 24 2018, 5:59 PM
emaste accepted this revision.Oct 25 2018, 1:13 PM
emaste added inline comments.
lib/csu/aarch64/crt.h
1

Perhaps put a comment here that arm64 does not use .ctors/.dtors or such? To explain why there's a seemingly-useless header.

This revision is now accepted and ready to land.Oct 25 2018, 1:13 PM
Closed by commit rS339738: Implement a BSD licensed crtbegin/crtend (authored by andrew). · Explain WhyOct 25 2018, 5:40 PM
This revision was automatically updated to reflect the committed changes.
val_packett.cool added a subscriber: val_packett.cool.Oct 25 2018, 9:05 PM
val_packett.cool added inline comments.
lib/csu/aarch64/crt.h
1

Huh, why doesn't arm64 use them?!

I've been porting ldc, the LLVM D Compiler, to FreeBSD/aarch64 — and wondering why the hell aren't ctors/dtros running…

andrew marked an inline comment as done.Oct 25 2018, 9:10 PM
andrew added inline comments.
lib/csu/aarch64/crt.h
1

On arm64 we use .init_array for constructors as it's defined in the ABI.

andrew added a commit: rS352394: MFC r339738, r339744, r339770, r339773, r339864-r339866, r339907-r339908….Sep 16 2019, 1:41 PM

Revision Contents
Changeset List

PathSize
gnu/
lib/
6 lines
lib/
csu/
6 lines
23 lines
aarch64/
1 line
amd64/
32 lines
common/
99 lines
64 lines
i386/
32 lines
powerpc64/
33 lines
tests/
6 lines
3 lines
11 lines
92 lines
dynamic/
7 lines
129 lines
137 lines
static/
8 lines
share/
mk/
8 lines
tools/
build/
options/
5 lines
5 lines

Diff 49573

View Options

gnu/lib/Makefile

Loading...
View Options

lib/csu/Makefile

Loading...
View Options

lib/csu/Makefile.inc

Loading...
View Options

lib/csu/aarch64/crt.h

Loading...
View Options

lib/csu/amd64/crt.h

Loading...
View Options

lib/csu/common/crtbegin.c

Loading...
View Options

lib/csu/common/crtend.c

Loading...
View Options

lib/csu/i386/crt.h

Loading...
View Options

lib/csu/powerpc64/crt.h

Loading...
View Options

lib/csu/tests/Makefile

Loading...
View Options

lib/csu/tests/Makefile.inc

Loading...
View Options

lib/csu/tests/Makefile.tests

Loading...
View Options

lib/csu/tests/cxx_constructors.cc

Loading...
View Options

lib/csu/tests/dynamic/Makefile

Loading...
View Options

lib/csu/tests/fini_test.c

Loading...
View Options

lib/csu/tests/init_test.c

Loading...
View Options

lib/csu/tests/static/Makefile

Loading...
View Options

share/mk/src.opts.mk

Loading...
View Options

tools/build/options/WITHOUT_BSD_CRTBEGIN

Loading...
View Options

tools/build/options/WITH_BSD_CRTBEGIN

Loading...