Remove swiftImageInspectionShared

[lanza]

Remove the swiftImageInspectionShared since #12758 removed the need for it's existence.

[lanza]

@compnerd

[compnerd]

@lanza did you try building a static binary with this toolchain?

[compnerd]

@swift-ci please test

[compnerd]

CC: @spevans - seems that static binaries work with this change (as expected). This is generally simpler than the approach that you had taken in #15183. Any reason to prefer that over this?

[spevans]

@compnerd I haven't actually tested this but it appears to remove ImageInspectionELF.cpp so I dont see how it would work for extra libraries that get loaded. It would also need to implement a version of lookupSymbol that doesn't use dladdr() which is unavailable in static binaries.

[lanza]

Huh? This commit does not remove that file.

[spevans]

Sorry, I mis-read the diff and didn't see the line

-  list(REMOVE_ITEM swift_runtime_sources ImageInspectionELF.cpp)

So thought that it was not getting compiled in.

The point of the libswiftImageInspectionShared is that it contains just the parts only needed in a dynamic executable, there should be a libswiftImageInspectionStatic for static executables (which is restored in #15183). So the two are used as follows:

Normal dynamic binary and binary built with -static-stdlib: libswiftCore + libswiftImageInspectionShared (ImageInspectionELF.cpp).

Statically linked binary built with -static-executable: libswiftCore + libswiftImageInspectionStatic (StaticBinaryELF.cpp).

Once #15183 is applied it will make more sense.

[compnerd]

@spevans I'm really not sure I follow the logic here. ImageInspectionELF.cpp contains everything needed for static or shared libraries. The only bit that causes issues in the first place is the swift::lookupSymbol which we could easily conditionalize on the build type (static or shared) and it is included in the runtime. The duplication that #15183 causes seems unnecessary since the functions are identical.

swift_addNewDSOImage will register the current image into the registered list of images, and then register the metadata into the runtime. The loops that you have removed in the duplication in the mentioned PR will execute once and have the same effect, so you have duplicated the code for no reason.

Why not just reuse the existing code path and remove the single function in the case of the static runtime build? This is basically the same issue that I raised on #15183. I'd like to understand what exactly the motivation is to duplicate the runtime into a second build when you are simply trying to remove a single reference (i.e. dladdr) from an unused function.

[spevans]

I wasn't clear enough in my previous answer, but libswiftCore.a is actually used in 2 separate ways when building an executable.

1. Normal dynamic executable: libswiftCore.so + libswiftImageInspectionShared.so (lookupSymbol uses dladdr)

2. Fully static executable (-static-executable): libswiftCore.a + libswiftImageInspectionStatic.a (lookupSymbol directly reads the symbols using StaticBinaryELF.cpp

3. Dynamic executable with static swift stdlib (-static-stdlib): libSwiftCore.a + libswiftImageInspectionShared.a: (lookupSymbol uses dladdr)

Its case 3. which causes issues due to its mixture of static libraries in a dynamic executable.

If we wanted to eliminate the ImageInspection stub libraries we could by explicitly producing 2 libswiftCore.a and end up with:

libswiftCore.so, libswiftCoreForUseInFullyStaticBinaries.a, and libswitCoreForUseInSharedBinariesIncludingWithStaticStdlib.a which may be simpler in the end assuming we can come up with some better names 😀

As to your query about swift_addNewDSOImage you are correct in that it can be reused and in shared libraries (in combination with the initialise*Lookup() functions) it maintains a list of metadata sections from swift libraries as they are loaded and then adds them in using the initialise*Lookup() functions.

For static binaries this can be short circuited to simply add the section in the initialise*Lookup() functions since there is only one metadata section for the whole binary and its data is available immediately. This eliminates the extra call overhead of the swift_image_constructor in SwiftRT-ELF.cpp and saves maintaining the linked list.

[compnerd]

(responding in reverse order since the latter is more involved)

I agree that short-circuiting avoids the linked list maintenance, but that really is 4 writes, which is really not that expensive and avoids a bunch of code duplication. I think that the benefits of a single codepath there outweighs the benefits of the tiny optimization that you get. The "overhead" in the swift_image_constructor is some memory writes and a function call, which is again quite cheap IMO. Do you have a particular environment in mind where the ~20 memory writes and a function call are so expensive to make this worth the possibility of divergence worth it?

As to the point of the merged static runtimes, I think that it might be nicer to have the separate builds of the runtime. I'd like to get @bob-wilson's and @jrose-apple's opinion on this as well. I think that longer term, having a static runtime with a dynamic executable would probably be deprecated? This really is only needed right now due to the unstable ABI, and as discussed offline with @bob-wilson, I think that it makes sense to switch to a resilient ABI across the board in one go. That would really obviate the need for the static runtime + dynamic executable case. In such a case, with the merged constructor, I think that the difference really would become which runtime library you link against (libswiftCore.so vs libswiftCore.a). This greatly reduces the overall complexity in the system. Of course this leaves us with the difficult problem of naming.

For the naming, I think that we should go for something confusing rather than clear if my assumption about future obsolescence is correct. We could do something like:

libswiftCore.so: dynamic runtime (dynamic executable)
libswiftCore.a: static runtime (static executable)
libswiftCoreFragile.a: static runtime (dynamic executable)

This indicates that the runtime itself is built with the fragile (Apple parlance, I don't think that Linux/Windows have a nice term for this) runtime. The benefit of this slightly obscure naming is that we would simply drop libswiftCoreFragile.a eventually, and people wouldn't notice any changes in the command line.