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

[weissi]

…by ICU)

Resolves SR-3819. ICU compiled in default mode (without --disable-dyload) needs to be linked with -ldl as it needs dl{open,close,sym} (even in its static version libdl.a). This adds -ldl to the linker flags for static executables.

[compnerd]

CC: @jrose-apple

This definitely doesn't feel like the right place for this. We need to sink this knowledge into the driver. libdl is a target dependent property, even beyond if (UNIX) in CMake, e.g. NetBSD doesnt use libdl.

[jrose-apple]

I don't have a better idea that doesn't involve the driver calling out to icuconfig with every static link.

cc @dabrahams, the owner of Swift's interface with ICU.

[dabrahams]

@jrose-apple not sure how I can help, here.

[weissi]

I agree with @jrose-apple , the correct thing to do would be to call out to icu-config but for the time being I'd argue that this PR is better than nothing. I'm happy to file a SR and also look into fixing it if we can agree that calling out to icu-config is the right (or only?) solution.

[alblue]

@swift-ci please smoke test

[weissi]

@jrose-apple / @dabrahams any ideas on how we can move this one forward?

[jrose-apple]

I think we're waiting for someone to look at ICU and decide what the right behavior is. I'm not opposed to this patch as long as it doesn't break any configurations.

[weissi]

@jrose-apple correct me if I'm wrong but additionally linking libdl should only break if there's no libdl.a or am I missing something? Even if the platform's static ICU does not require libdl it shouldn't break if you link it. Chances are it'll be required anyway.

[dabrahams]

@jrose-apple Waiting for someone, like who? I don't really understand what this problem is about, so I've not been weighing in here even though @weissi has @-invoked me. But it does seem like it's not clear what the next steps are. Could you try to clarify that?

Thanx

[jrose-apple]

Someone needs to

1. Clearly evaluate the problem and the conditions where it occurs.
2. Let us know what ICU's recommendation is for C libraries.
3. Translate that to Swift.
4. Convince us that's the right plan, either for now or forever.

It's possible that's what @weissi already has, but I'm not quite convinced yet. :-) I was hoping your familiarity with ICU would help with this, but I guess that's more at the API level, not the project integration parts.

@weissi: No libdl.a in the search paths is actually a possibility, right? It's easier to add extra arguments than to remove them.

[dabrahams]

@jrose-apple writ large:

I was hoping your familiarity with ICU would help with this, but I guess that's more at the API level, not the project integration parts.

Yeah, I haven't really had to think through any linking issues in years, so maybe that should be someone else. You know who the ICU and linking experts are, right?

[spevans]

The underlying issue is the ./configure options used to build the libicu package on Linux.
As part of #5394 which added the -static-executable option to swift, a --libicu option was added to build-script-impl which can optionally compile libicu using the ./configure options found @ https://github.com/apple/swift/blob/master/utils/build-script-impl#L2668. The libicu that is built can then be linked in without the use of libdl which shouldnt be used if building proper static binaries on Linux.

In fact the libdl.a file is just a stub library:

$ size /usr/lib/x86_64-linux-gnu/libdl.a
   text    data     bss     dec     hex filename
     57       0       0      57      39 dlopen.o (ex /usr/lib/x86_64-linux-gnu/libdl.a)
     53       0       0      53      35 dlclose.o (ex /usr/lib/x86_64-linux-gnu/libdl.a)
     57       0       0      57      39 dlsym.o (ex /usr/lib/x86_64-linux-gnu/libdl.a)
     57       0       0      57      39 dlvsym.o (ex /usr/lib/x86_64-linux-gnu/libdl.a)
     53       0       0      53      35 dlerror.o (ex /usr/lib/x86_64-linux-gnu/libdl.a)
     53       0       0      53      35 dladdr.o (ex /usr/lib/x86_64-linux-gnu/libdl.a)
     53       0       0      53      35 dladdr1.o (ex /usr/lib/x86_64-linux-gnu/libdl.a)
     57       0       0      57      39 dlinfo.o (ex /usr/lib/x86_64-linux-gnu/libdl.a)
     57       0       0      57      39 dlmopen.o (ex /usr/lib/x86_64-linux-gnu/libdl.a)

So I think the correct fix is to see if the upstream package can be built with different options so that the static libraries dont use the libdl functions at all.

[jrose-apple]

This is reassuring in both directions: first, that there might be a better solution; and second, that even if we do have to link libdl it won't be an undue burden on the built executable.

[weissi]

@spevans / @jrose-apple what are we doing with this PR then?

[spevans]

@weissi I dont think this solution is correct. Static executables shouldnt be calling any dl functions. Even glibc doesnt like being statically linked with -ldl:

$ echo 'print("Hello World")' > test.swift && ~/swift-test/usr/bin/swiftc -static-executable -Xlinker -ldl  test.swift
/usr/bin/../lib/gcc/x86_64-linux-gnu/5.4.0/../../../x86_64-linux-gnu/libicuuc.a(putil.ao):function uprv_dl_open_55: warning: Using 'dlopen' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
$ ./test
Hello World

Although it works its not really correct. The correct solution is to use a libicu that has been configured and compiled without use of dlopen. Note you will see the same issues when linking in Foundation, due to the default libcurl on linux using dlopen as well. It can be built to not use it, but then a toolchain is required that has static library versions of libcurl and a few other libraries.

Even when building with a static Foundation and static libcurl and libicu glibc will STILL issue warnings due to usage of functions that use modules eg PAM used by getpwnam etc.

[weissi]

@spevans whilst on a theoretical level I agree with you that static executables shouldn't really call out do dl*, it's not really the reality, hence my patch.

Let's go through why

• I believe what you're showing with your example is that libicuuc.a is using dlopen even though it's a static executable, we can't change that (short of providing our own version of ICU and never using the system's one)
• as you say, glibc will always warn you in a static binary if you use anything that uses NSS (DNS, anything related to passwords/users/...) because glibc does not really exist in a fully static version. In other words, glibc isn't compatible with a static-only environment (see glibc FAQ ).

The points you make don't show that what I'm doing in this PR isn't correct, they show that libicu and glibc despite having .a files will still use shared linking using libdl. Also: libdl has a static version exactly for this purpose. My patch just acknowledges this fact and makes it explicit which makes it actually work. Again, in an ideal world we wouldn't need this and we could port to Swift to musl libc which actually has a static version, drop the CURL, libxml, ... dependencies in Foundation, have our own properly statically linked version of ICU. That would be great but it's a sizeable effort. Therefore for now I propose to appreciate the decision Ubuntu has taken and just link with the static version of libdl.

Does that make sense?

Here more background info for Ubuntu 16.04:

• the static libicuuc.a uses dl* and therefore requires libdl

$ nm /usr/lib/x86_64-linux-gnu/libicuuc.a  | grep 'U dl'
                 U dlclose
                 U dlopen
                 U dlsym

• there is a static version of libdl:

$ ls /usr/lib/x86_64-linux-gnu/libdl.a 
/usr/lib/x86_64-linux-gnu/libdl.a

• the static glibc version does have plenty of dl-functionality

$ nm /usr/lib/x86_64-linux-gnu/libc.a 2> /dev/null | grep '__dl' 
0000000000000080 T __dlopen
0000000000000010 T __dlclose
0000000000000020 T __dlsym
0000000000000020 T __dlvsym
                 U __dladdr
                 U __dladdr1
                 U __dlclose
0000000000000050 T __dlerror
                 U __dlinfo
                 U __dlmopen
                 U __dlopen
                 U __dlsym
                 U __dlvsym
0000000000000000 T __dladdr
0000000000000000 T __dladdr1
00000000000000d0 T __dlinfo
0000000000000070 T __dlmopen
0000000000000000 T __dl_iterate_phdr

[spevans]

@weissi Yes from a technically correct POV this fix is wrong however in reality the following are true so I dont have any strong objections:

1. -ldl wont be linked in to executables that dont need it, so its not going to do any harm to binaries that dont use dlopen etc.

2. At least glibc prints it own warning.

I wonder what would happen if a static executable using dl didnt have access to the underlying glibc version as the warning says it needs.

However just bear in mind that if using Foundation there will be more issues than this since there arent even .a versions of some of the libraries.

Also see #9958 which make -static-stdlib work a lot better on Linux.

[alblue]

Note that we're already passing in -ldl when we build the static linked version of the arguments anyway:

https://github.com/apple/swift/blob/d87cb5bc98d973493af735578111d4857ea6f410/utils/gen-static-stdlib-link-args#L62

So all we're doing is harmonising what we're building the statically linked library with the executables that result from using that library.

[jrose-apple]

Okay, this latest discussion makes me feel better too. Let's re-run the tests just to be sure (it's been months), but then we can take this.

@swift-ci Please test Linux