[SR-648] Add option to create statically linked binaries (take 2) #5394

spevans · 2016-10-21T14:46:29Z

This is a reworking of PR #4754 to provide a way of creating static binaries for use with the package manager. Based on the comments in the earlier PR this one doesn't intercept any libdl but builds its own version of libicu that doesn't need dlopen() etc

Main changes:

Adds option to build/install libicu instead of using the system one. This allows it to be configured to produce static libraries that don't use the dynamic linker
Creates a libswiftSectionDataShared.a and libswiftSectionDataStatic.a to link with libswiftCore.a depending on the executable being dynamic or static
Adds -static-executable option to swiftc to use along with -emit-executable
Also fixes SR-2280 for -static-stdlib on linux to statically link in libicu
Only works on Linux for now

spevans · 2016-10-25T14:27:00Z

/cc @jckarter could you have a nitpick of this pr if you have some time, it should be a cleaner solution than the last one

jckarter · 2016-10-25T17:25:05Z

Definitely, thanks for continuing work on this @spevans!

jckarter · 2016-10-25T17:26:00Z

@erg might also have opinions about the cmake changes here.

mominul · 2016-10-26T10:45:21Z

@spevans How about closing #5269 ?

erg · 2016-10-26T18:42:10Z

Is this something we should merge once OSX support is added or should we merge the Linux part first?

spevans · 2016-10-26T18:57:28Z

@erg osx doesnt support static binaries but it should be possible to expand it to other ELF targets eventually, its just that Linux is the only one I have to test with and I didnt want to break any of the other ELF targets at the moment

spevans · 2016-10-26T18:59:54Z

@mominul #5269 actually works slightly differently and solves the problem for when we are using the distro's libicu so I'll leave it open for now

jckarter · 2016-10-26T19:12:19Z

Apple platforms don't really support fully static binaries, so focusing on Linux support seems like the right thing to do.

spevans · 2016-11-02T20:02:38Z

rebased and retested on Linux and macOS

@jckarter is there anything I can do to help get this PR reviewed and merged? Would it help if I broke out some of the commits into their own PRs?

jckarter · 2016-11-02T21:55:47Z

Sorry, just have been busy with other things. Let me take a look now.

jckarter · 2016-11-03T16:58:50Z

stdlib/public/runtime/SectionDataStatic.cpp

+dladdr(const void* addr, Dl_info* info)
+{
+  return 0;
+}


We should not export our own definition of dladdr. Is this still necessary?

This is needed by Errors.cpp when doing a stack backtrace. I dont currently have an implementation for this but I am thinking of adding an ELF parser to inspect the binary, but for now I just made it return 0 for not found

Errors.cpp can be changed to use a different name, swift_dladdr or something similar, that returns 0 in the static implementation. We shouldn't be redefining standard POSIX symbols.

I added a commit to wrap dladdr() in _swift_dladdr() and updated the callers. I also tidied up the function visibility for exported functions using the correct macros and retested on macOS and Linux

Thanks @spevans, this is shaping up nicely! I took a shot at refactoring all the image inspection code in #5846. It should be a lot easier to cleanly slot in an ImageInspectionStatic.cpp file once that goes in; all you'd need is something like:

extern const char Conformances __asm__(".swift2_protocol_conformances_start"); extern const char TypeMetadataRecords __asm__(".swift2_type_metadata_start"); void swift::initializeProtocolConformanceLookup() { // The image is statically linked, so we can just directly reference the symbol // that signals the beginning of the section. // ld should mash all the conformance records together for us, and we don't need // to be concerned about dynamic loading. uint64_t size; memcpy(&size, Conformances, sizeof(size)); addImageProtocolConformanceBlockCallBack(Conformances + sizeof(size), size); }

and something similar for type metadata records.

@jckarter Ahh I see, you mean have a separate initialise function for each section rather than having them all share the same one, that makes it a bit easier to understand. Do you think in the future there will be many more data sections? I noticed some other ones defined in swift_sections.S but they only seemed to be used by the unit test code

jckarter · 2016-11-03T16:59:40Z

stdlib/public/runtime/SectionData.h

+#define SWIFT_TYPE_METADATA_SECTION ".swift2_type_metadata_start"
+
+#if defined(__linux__)
+#define SUPPORTS_STATIC_BINARIES


Macros should be namespaced by starting with SWIFT_.

I fixed the macros names

jckarter · 2016-11-03T17:27:13Z

stdlib/public/runtime/SectionDataStatic.cpp

+   auto blockSize = *reinterpret_cast<const uint64_t*>(blockAddr);
+   blockAddr += sizeof(blockSize);
+   inspectArgs->fnAddImageBlock(blockAddr, blockSize);
+}


There really shouldn't need to be any work done here, since there's only ever one section of interest in a static binary.

Theres currently two sections (protocol, metadata) although the metadata isn't usually loaded from what I saw in my tests. I added a struct to define the sections which eliminated the casts and simplified it

jckarter · 2016-11-03T17:30:35Z

stdlib/public/runtime/SectionData.h

+  void _swift_initializeCallbacksForSectionData(InspectArgs *inspectArgs);
+}
+
+#endif /* SWIFT_RUNTIME_SECTIONDATA_H */


I'm not sure any of the abstractions here really hold their weight, given how platform-specific the behavior is at nearly every level. The code is getting really confusing to follow with how conditionalized it. At a high level, we need _swift_initializeCallbacksForSectionData to perform platform-specific registration of existing sections and observation of new dylibs getting loaded. I don't think that operation benefits from having any InspectArgs passed down—the implementation of _swift_initializeCallbacksForSectionData() can probably take no arguments and be factored into a per-platform implementation that knows the specific lookup operations to perform for a general platform (Mach-O, Windows, dynamic ELF, static ELF).

I had a go at changing the interface to remove the extra callback used just for MachO however I think that I may have introduced more complexity into the mach-o handling to provide a simpler interface.

I couldn't find a way to eliminate the InspectArgs because the 2 sections are initialised lazily
unless we explicitly want to setup callbacks for both sections at startup - Im not sure if thats acceptable though. Unless you mean when one of the sections is lazily loaded by calling _swift_initializeCallbacksForSectionData() it loads the other one at that point as well, I think that should be easy to accomodate

I did however manage to come up with a solution for loading sections in ELF objects that are dlopen'd after the initial initialise, which I think fixes rdar://problem/19045112

Yeah, injecting a static constructor is probably the best we can do for ELF, since ld.so doesn't appear to have any other on-image-load hooks. I might not be explaining the refactoring I had in mind well—let me take a look at cleaning this code up a bit.

Can the static constructor for introducing dlopen-ed ELF images be factored into a separate PR? I don't want to block this PR on that functionality.

yes, it shouldnt be a problem to back out the constructor and add it as a separate PR

jckarter · 2016-11-03T17:31:35Z

I made some comments on the runtime implementation. The CMake changes look vaguely reasonable, but that's not my wheelhouse. @shahmishal, @erg, or @zisko, any comments on that?

jckarter · 2016-11-18T17:10:17Z

@spevans I merged #5846, so you may want to rebase and update for those changes.

jckarter · 2016-11-18T21:58:41Z

FYI, Apple's Thanksgiving break is next week, so I won't have a chance to look at this again till the 28th. Thanks for continuing to work on it!

spevans · 2016-11-27T23:44:54Z

@jckarter I've rebased wrt your changes. I ended up adding a _swfit_dladdr() wrapper around dladdr() in each platform's ImageInspection*.cpp which isnt ideal but if Win32 needs it's own implentation at some point its not too bad.

After this PR is merged I have two changes lined up, one to add a full dladdr() implementation for ELF static binaries and another to address the ELF dynamically loaded images metadata lookup which is dependant on some changes in this PR

erg · 2016-11-28T18:03:33Z

@swift-ci Please smoke test

jckarter · 2016-11-28T18:40:13Z

Great! I'll try to look at this tomorrow.

jckarter · 2016-11-28T18:40:56Z

@spevans It looks like there's a new conflict with Greg's ProtocolConformance.cpp changes, if you don't mind merging that in.

spevans · 2016-11-29T00:47:20Z

@jckarter I opened #5969 which should be merged first if it looks ok as the recent windows fixes make rebasing this a bit harder otherwise due to the _swift_dladdr() wrappers

- Add --libicu option to compile icu from source. This allows the configuration to be controlled so that it is enabled for shared and static building and the static files dont require the use of the dynamic loader - Add --install-libicu option to install the shared libraries into $prefix/lib/swift and the static ones into $prefix/lib/swift_static. This avoids interference with system installed versions - Dont use find_package if building ICU from source

- Add ImageInspectionStatic.cpp to lookup protocol conformance and metadata sections in static binaries - For Linux, build libswiftImageInspectionShared.a and libswiftImageInspectionStatic.a for linking with libswiftCore.a. This allows static binaries to be built without linking to libdl. libswiftImageInspectionShared (ImageInspectionELF.cpp) is automatically compiled into libswiftCore.so - Adds -static-executable option to swiftc to use along with -emit-executable that uses linker arguments in static-executable-args.lnk. This also requires a libicu to be compiled using the --libicu which has configure options that dont require libdl for accessing ICU datafiles - Static binaries only work on Linux at this time

- Create a file of linker arguments instead of a hardcoded list in ToolChains.cpp for use by -static-stdlib option

spevans · 2016-11-29T17:34:11Z

Rebased on top of #5969

jckarter

Looking good! Just a few small comments.

jckarter · 2016-11-29T20:42:26Z

stdlib/public/runtime/CMakeLists.txt

@@ -62,13 +62,57 @@ set(LLVM_OPTIONAL_SOURCES
    MutexPThread.cpp
    MutexWin32.cpp
    CygwinPort.cpp
+    ImageInspectionELF.cpp


Isn't ImageInspectionELF.cpp included in the core swift_runtime_sources above?

When compiling for Linux it is removed from swift_runtime_sources so that it is not added to libswiftCore.a but instead used in libswiftImageInspectionShared.a
For other ELF targets it remains in swift_runtime_source - this may change as this static option is tested on other ELF targets

jckarter · 2016-11-29T20:43:05Z

stdlib/public/runtime/ImageInspectionELF.cpp

@@ -78,6 +79,7 @@ static int iteratePHDRCallback(struct dl_phdr_info *info,
  return 0;
 }

+SWIFT_RUNTIME_EXPORT


These functions shouldn't need to be exported outside the runtime AFAICS.

This is used because the functions are in libswiftImageInspectionShared.a but called from libswiftCore.a

Its not needed for the ImageInspectionStatic.cpp functions as this file is not compiled with
-fvisibility=hidden

They still shouldn't need to be exported since all of the .a files are ultimately being linked into the same binary. Visibility only affects an executable or dylib, not .o files. In a static binary, ideally, everything should be hidden besides main, in order to maximize the power of LTO and dead symbol stripping.

I just commented them out and you are correct, it does still link, but it fails when building the tests. I was mistaken, the issue is actually with linking the tests with libswiftCore.so not with the .a libs (its been a while since I first looked at it!). It looks like it is fixable in the CMake files so I will add a fix for that and remove the SWIFT_RUNTIME_EXPORT

jckarter · 2016-11-29T20:44:42Z

stdlib/public/runtime/ImageInspectionStatic.cpp

+  uint64_t size;
+  const uint8_t data[0];
+};
+extern const SectionInfo __swift2_protocol_conformances_start;


You ought to be able to preserve the existing dotted symbol names by declaring these with an asm attribute, e.g. extern const SectionInfo ProtocolConformancesStart asm(".swift2_protocol_conformances_start");, which would also give the declarations a slightly friendlier source name. I'm a bit concerned about the potential of other users outside of the compiler depending on the existing symbol names.

jckarter · 2016-11-29T20:58:54Z

stdlib/public/runtime/ImageInspectionStatic.cpp

+// Used in swift_sections.S to mark the start of a section
+struct SectionInfo {
+  uint64_t size;
+  const uint8_t data[0];


Zero-sized array fields aren't standard C++, and reaching outside the bounds of an array field through a struct pointer is undefined behavior. It'd fit a bit more within the standard to leave SectionInfo opaque and do char* pointer manipulation:

struct alignas(uint64_t) SectionInfo; uint64_t getSectionSize(const SectionInfo *section) { uint64_t result; memcpy(&result, section, sizeof(uint64_t)); return result; } const void *getSectionData(const SectionInfo *section) { return reinterpret_cast<const char*>(section) + sizeof(uint64_t); }

Is the following ok? I wanted to keep SectionInfo a struct as it will also be used in the PR which fixes the issue with libs that are dlopen()'d after startup.

extern const uint64_t protocolConformancesStart asm(".swift2_protocol_conformances_start"); extern const uint64_t typeMetadataStart asm(".swift2_type_metadata_start"); struct SectionInfo { uint64_t size; const char *data; }; static SectionInfo _getSection(const uint64_t *section) { SectionInfo info; info.size = *section; info.data = reinterpret_cast<const char *>(section + 1); return info; } void swift::initializeProtocolConformanceLookup() { auto protocolConformances = _getSection(&protocolConformancesStart); addImageProtocolConformanceBlockCallback(protocolConformances.data, protocolConformances.size); }

Keeping a SectionInfo struct around seems reasonable. I think it's still best to declare the section references as an opaque type, for type safety, and to avoid possible optimizations assuming protocolConformancesStart only points to a uint64_t's worth of data:

struct alignas(uint64_t) Section; extern const Section protocolConformancesStart asm(".swift2_protocol_conformances_start"); extern const Section typeMetadataStart asm(".swift2_type_metadata_start"); struct SectionInfo { uint64_t size; const char *data; }; static SectionInfo getSectionInfo(const Section *section) { SectionInfo info; memcpy(&info.size, section, sizeof(uint64_t)); info.data = reinterpret_cast<const char *>(section) + sizeof(uint64_t); return info; }

jckarter · 2016-11-29T21:04:41Z

@swift-ci Please smoke test

jckarter · 2016-11-29T23:17:37Z

@swift-ci Please smoke test Linux

jckarter · 2016-11-30T01:10:06Z

@ddunbar The swift-build bootstrap appears to be failing without any errors emitted to the console. Any suggestion as to what that might indicate?

https://ci.swift.org/job/swift-PR-Linux-smoke-test/2858/consoleFull#127721033ee1a197b-acac-4b17-83cf-a53b95139a76

ddunbar · 2016-11-30T17:46:53Z

Based on the exit status I suspect what is happening is that swift-build-tool is crashing (with SIGILL, maybe?)? I'm not sure how to diagnose... does this reproduce locally for anyone?

ddunbar · 2016-11-30T17:48:23Z

Oh wait... I misread -- this is the self-hosted build that is failing, so the chances are good that swift-build itself was miscompiled and is crashing. That would explain the lack of any other diagnostics.

- Revert the use of SWIFT_RUNTIME_EXPORT in ImageInspectionELF.cpp and fix the unittests by explicitly adding the file to the list - Revert the change of section data names

spevans · 2016-11-30T19:21:54Z

@ddunbar Ive not seen any issues when testing and Ive been building on both ubuntu16.04 and debian9

@jckarter The commit I just added should fix the outstanding issues with the SWIFT_RUNTIME_EXPORT, section renaming and extracting the section info

ddunbar · 2016-11-30T19:25:28Z

@spevans are you also building the package manager as part of your Linux builds?

jckarter · 2016-11-30T19:47:07Z

Runtime changes look great now, thanks for bearing with me! Are you able to look into the swift-build bootstrapping problem?

jckarter · 2016-11-30T19:47:52Z

@swift-ci Please smoke test

spevans · 2016-11-30T19:49:07Z

@ddunbar I hadnt been testing with --swiftpm but Im running a test now and it seems to be working so far. I think that last commit may have fixed something as I think I stopped testing with the package manager a while ago when I saw some breakage but though it was due to something else

jckarter · 2016-11-30T20:34:51Z

Looks like swiftpm bootstraps successfully now.

alblue · 2016-12-01T12:56:25Z

When I update to master and build with: swift/utils/build-script -R -T --libdispatch -- --reconfigure

I get this test failure, which may be related to this merge:

--
Exit Code: 1

Command Output (stderr):
--
LLVM ERROR: /swift-build/build/Ninja-ReleaseAssert/swift-linux-x86_64/lib/swift_static/linux/static-stdlib-args.lnk not found

--

********************
Testing Time: 201.96s
********************
Failing Tests (1):
    Swift(linux-x86_64) :: Driver/static-stdlib-linux.swift

spevans · 2016-12-01T14:40:19Z

@alblue Possibly an error where the static-stdlib test is running but the --build-swift-static-stdlib was not specificied. Can you add that option and try again. I will see if that test should be running without it

jckarter · 2016-12-01T16:30:34Z

If that test is only intended to be run for static-executable targets, we may need to add a REQUIRES: variable to lit.cfg that the test can look for.

spevans · 2016-12-01T16:38:33Z

That test is the preexisting one and currently has:

// REQUIRES: OS=linux-gnu
// REQUIRES: static_stdlib

Thats why I thought it might be a config error as I think it should only be run if --build-swift-static-stdlib (the preexisting option) was used.

It may have worked in the past by accident if the lib/swift_static/linux files were there from a previous build but fails now because of the missing static-stdlib-link-args

It may also be an issue the --reconfigure doesnt work fully and may need CMakeCache.txt to be removed, I cant replicate the exact issue but Im trying to see if its possible to get stale values left in CMakeCache.txt

jckarter · 2016-12-02T21:23:38Z

@spevans I wanted to check whether you had started a PR to add the static constructor for registering dlopen-ed images with the runtime. I checked our open PRs and didn't see it right away.

[pull] swiftwasm from main

spevans force-pushed the pr_libicu_with_static_executable branch from 6783268 to f975871 Compare October 23, 2016 19:20

jckarter self-assigned this Oct 25, 2016

spevans force-pushed the pr_libicu_with_static_executable branch 2 times, most recently from 468a4a8 to f4378c1 Compare November 2, 2016 12:30

jckarter requested changes Nov 3, 2016

View reviewed changes

spevans force-pushed the pr_libicu_with_static_executable branch from 0b76e48 to 68830ab Compare November 17, 2016 17:19

spevans force-pushed the pr_libicu_with_static_executable branch from 530dc22 to 254cc73 Compare November 21, 2016 23:57

spevans mentioned this pull request Nov 29, 2016

[runtime] Add platform independent version of dladdr() / Dl_info #5969

Merged

spevans added 3 commits November 29, 2016 13:36

[SR-648] Update -static-stdlib option on Linux with libicu changes

f6866e7

- Create a file of linker arguments instead of a hardcoded list in ToolChains.cpp for use by -static-stdlib option

spevans force-pushed the pr_libicu_with_static_executable branch from 254cc73 to f6866e7 Compare November 29, 2016 16:58

jckarter reviewed Nov 29, 2016

View reviewed changes

[SR-648] ELF static binary fixes

707588c

- Revert the use of SWIFT_RUNTIME_EXPORT in ImageInspectionELF.cpp and fix the unittests by explicitly adding the file to the list - Revert the change of section data names

jckarter approved these changes Nov 30, 2016

View reviewed changes

jckarter merged commit 1f6a8ff into swiftlang:master Nov 30, 2016

spevans mentioned this pull request Jan 11, 2017

SR-2280: swiftc -static-stdlib option fails on Linux #5269

Merged

spevans mentioned this pull request Mar 22, 2017

fix SR-3819: add -ldl to static executables (almost certainly needed … #7188

Merged

spevans mentioned this pull request Jan 16, 2020

Restore -static-executable on Linux. #29039

Closed

finagolfin mentioned this pull request Apr 1, 2020

[build] rewrite #21707 so that it doesn't break cross-compilation #30170

Open

spevans mentioned this pull request Sep 14, 2016

[SR-648] swift package manager should have the option to produce statically linked binaries swiftlang/swift-package-manager#5432

Open

MaxDesiatov pushed a commit that referenced this pull request Sep 7, 2023

Merge pull request #5394 from swiftwasm/main

547439e

[pull] swiftwasm from main

[SR-648] Add option to create statically linked binaries (take 2) #5394

[SR-648] Add option to create statically linked binaries (take 2) #5394

Conversation

spevans commented Oct 21, 2016

spevans commented Oct 25, 2016

jckarter commented Oct 25, 2016

jckarter commented Oct 25, 2016

mominul commented Oct 26, 2016

erg commented Oct 26, 2016

spevans commented Oct 26, 2016

spevans commented Oct 26, 2016

jckarter commented Oct 26, 2016

spevans commented Nov 2, 2016

jckarter commented Nov 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spevans Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jckarter commented Nov 3, 2016

jckarter commented Nov 18, 2016

jckarter commented Nov 18, 2016

spevans commented Nov 27, 2016

erg commented Nov 28, 2016

jckarter commented Nov 28, 2016

jckarter commented Nov 28, 2016

spevans commented Nov 29, 2016

spevans commented Nov 29, 2016

jckarter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jckarter commented Nov 29, 2016

jckarter commented Nov 29, 2016

jckarter commented Nov 30, 2016

ddunbar commented Nov 30, 2016

ddunbar commented Nov 30, 2016

spevans commented Nov 30, 2016

ddunbar commented Nov 30, 2016

jckarter commented Nov 30, 2016

jckarter commented Nov 30, 2016

spevans commented Nov 30, 2016

jckarter commented Nov 30, 2016

alblue commented Dec 1, 2016

spevans commented Dec 1, 2016

jckarter commented Dec 1, 2016

spevans commented Dec 1, 2016

jckarter commented Dec 2, 2016

spevans Nov 8, 2016 •

edited

Loading