ELF registration Restructuring #12758

compnerd · 2017-11-04T00:54:59Z

Restructure the ELF handling to be completely agnostic to the OS.
Rather than usng the loader to query the section information, use the
linker to construct linker tables and synthetic markers for the
beginning and of the table. Save off the values of these pointers and
pass them along through the constructor to the runtime for registration.

This removes the need for the begin/end objects. Remove the special
construction of the begin/end objects through the special assembly
constructs, preferring to do this in C with a bit of inline assembly to
ensure that the section is always allocated.

Remove the special handling for the various targets, the empty object
file can be linked on all the targets.

The new object file has no requirements on the ordering. It needs to
simply be injected into the link.

Name the replacement file srt.o mirroring crt.o from libc. Merge
the constructor and the definition into a single object file.

This approach is generally more portable, overall simpler to implement,
and more robust.

Replace this paragraph with a description of your changes and rationale. Provide links to external references/discussions if appropriate.

Resolves SR-NNNN.

compnerd · 2017-11-04T00:57:02Z

CC: @dcci @modocache @hpux735

@jrose-apple not sure if you remember anyone else who may be interested in this reworking

hpux735 · 2017-11-04T01:04:36Z

perhaps @tienex ?

hpux735 · 2017-11-04T01:07:34Z

After a quick glance, I think this is great. To be perfectly honest, it's a little over my head (this was Tienex's innovation). To perform robust regression testing on arm, however, we have work to do to get Swift 4+ working there.

jrose-apple · 2017-11-06T20:10:21Z

I'd want a @jckarter or @gparker42 to comment on it as well.

jckarter · 2017-11-06T20:30:01Z

lib/Driver/ToolChains.cpp

+  SmallString<128> srtPath = SharedRuntimeLibPath;
+  llvm::sys::path::append(srtPath,
+                          swift::getMajorArchitectureName(getTriple()));
+  llvm::sys::path::append(srtPath, "srt.o");


Is there a more descriptive name we can use than srt?

"swiftrt.o" would be fine.

Or even just "swiftRuntime.o", we don't have to pay for bandwidth by the bit anymore!

I'm not tied to the name. swiftrt.o sounds good to me.

As an outsider I wouldn't have immediately guessed that srt/swiftrt concerns the runtime (even if it seems obvious in hindsight). There is value in spelling things out and the cost is low.

jckarter · 2017-11-06T20:30:51Z

lib/IRGen/GenDecl.cpp

@@ -2335,7 +2335,7 @@ llvm::Constant *IRGenModule::emitProtocolConformances() {
    sectionName = "__TEXT, __swift2_proto, regular, no_dead_strip";
    break;
  case llvm::Triple::ELF:
-    sectionName = ".swift2_protocol_conformances";
+    sectionName = "swift2_protocol_conformances";


Can we keep the existing symbol names to avoid unnecessary ABI thrash? I don't know what other tools might be looking for these symbol names.

No, that is part of the magic. The reason for changing the section names here is to permit the linker to generate the start and stop markers.

Can the reflection metadata symbols still be found out-of-process by the remote reflection library?

No, that is what Im seeing now. The protocol conformances and the type metadata works, but the reflection metadata is causing some problems. The section exists and it can find the section (as it is not coalesced into .data). The problem is that there is a section load offset that needs to be accounted for (as otherwise the relative pointer is now pointing to the wrong location) or swift-reflection-dump for example gets things wrong.

jckarter · 2017-11-06T20:39:38Z

stdlib/public/runtime/SwiftRT-ELF.cpp

+
+  swift_addNewDSOImage(&sections);
+
+#undef SWIFT_SECTION_RANGE


This is doing quite a bit more work and hardcoding more ABI liability into the static constructor than before. The static constructor should ideally do nothing more than make a runtime call, so that the runtime can be upgraded with additional capabilities without recompiling dynamic libraries. Since the static constructor runs eagerly at load time for libraries the executable links against, it's also important that it do as little work as possible if no lazily-instantiated runtime mechanisms have been initialized that require runtime instantiation. I think the previous approach of having the runtime inspect the image is preferable for these reasons.

I disagree. The new static constructor is doing negligible work: it merely collects a small number of image-local addresses and calls the runtime function. This arrangement avoids the vastly more expensive cost of calling dlsym() to find the sections later.

The version field in the MetadataSections provides sufficient ABI flexibility. New fields can be added to it; new runtimes can recognize calls from old libraries that don't have those fields.

OTOH, this design means that future runtimes only have access to the information we have the forethought to feed them through this MetadataSections record, instead of potentially any information we can glean from the image. (One could argue the difference is marginal in practice, since we'd still need to consciously export a symbol to reliably discover anything dynamically in the old model anyway.)

It seems to me like we still shouldn't have to do any work in the static constructor here, since the data structure here is nothing but a bunch of local references. We should be able to make the MetadataSections record here a global constant, full of relative references, so the only load-time work the constructor has to do is call the runtime with a pointer to the global constant. Using a global constant could still give us the flexibility to dladdr the pointer and get back to a general dso handle we can do deeper inspection on if we wanted to in the future.

@jckarter I'd love to make that a global constant. I just could not figure out how to make that a global constant (at least within C). I suppose that a C++ constructor could be emitted to construct the global structure.

IMO, this is doing a very small amount of work, which would ideally be just the runtime registration call, but I could not make the structure a global constant. That would be my preference as well. But, yes, the should is exactly what I think is ideal: the structure is a constant global, the constructor is just to tie to into the runtime.

Also note that this vastly simplifies the linker support needed. It obviates the need to place objects at a particular location as well as the special symbol construction and placement. This now relies solely on the standard required behavior for inspecting the linker tables.

Yeah, neither C's nor C++'s model for constant expressions is expressive enough here. You might be able to express it as LLVM IR, though, or as global inline assembly. Since it's just data it should be portable.

Calling dladdr() on any of the values passed in MetadataSections ought to recover the dso handle. We could add a field to MetadataSections that is simply a non-nil pointer somewhere into the image, to ensure that dladdr() has a target even if the image happens to have none of the other data.

@gparker42 that is one of the tricks that I'm playing here. The reason for the global inline assembly, as gross as it may be, is that it forces that the section will be constructed, which will force the emission of the start/stop symbols. These are referenced by the table thus preventing the dead strip from kicking in. As a result, we are guaranteed that the markers will always be present, though the section may be 0-sized. The symbols are image specific, so we are guaranteed to have a local symbol to reference to get the DSO handle.

compnerd · 2017-11-17T18:19:40Z

CC: @zbowling this is the change that I was referring to (there is something odd going on with the actual reading of the registration data that I need to figure out, and then this is ready to go I think)

zbowling · 2017-11-17T18:28:07Z

Awesome, yeah, this should work on Fuchsia.

compnerd · 2017-11-17T18:46:46Z

@zbowling yeah, and should be faster/smaller as well (this is ELF specific rather than OS specific now)

compnerd · 2017-11-17T20:06:35Z

@jckarter @rjmccall @gparker42 any suggestions as to where to start digging to figure out why strings are being printed as:

String(_core: Swift._StringCore(_baseAddress: Swift.Optional<Swift.UnsafeMutableRawPointer>.some(Swift.UnsafeMutableRawPointer(_rawValue: (Opaque Value))), _countAndFlags: Swift.UInt(_value: (Opaque Value)), _owner: Swift.Optional<Swift.AnyObject>.none))

Adding some tracing shows that the metadata is being identified and tabled correctly.

jckarter · 2017-11-17T21:04:50Z

That probably indicates that the protocol conformances aren't getting indexed properly, so the lookup for String's CustomStringConvertible conformance is failing.

compnerd · 2017-11-17T22:18:46Z

Ugh, I had forgot to populate two of the functions sigh. This should be relatively easy to fix and finish up.

compnerd · 2017-11-18T17:08:01Z

@swift-ci please test linux platform

swift-ci · 2017-11-18T17:09:16Z

Build failed
Swift Test Linux Platform
Git Sha - fd82c922b846adcd7ecc77d65623fac45e7ad7f1

compnerd · 2017-11-18T17:53:06Z

@swift-ci please test linux platform

swift-ci · 2017-11-18T17:53:34Z

Build failed
Swift Test Linux Platform
Git Sha - 594e60969d7bc6a93c27008dd12b4dadfa9bc66b

hpux735 · 2017-11-19T16:59:15Z

The Swift/Arm team (specifically @uraimo ) tested this patch, and it improved Swift 4+ on arm. This patch seems to have fixed regressions in the REPL that have come up since Swift 3.1.1.

jckarter · 2017-11-19T17:45:54Z

stdlib/public/runtime/SwiftRT-ELF.cpp

+    SWIFT_SECTION_RANGE(swift3_reflstr),
+    SWIFT_SECTION_RANGE(swift3_fieldmd),
+    SWIFT_SECTION_RANGE(swift3_assocty),
+};


Does Clang manage to compile this into a global constant, despite not formally being constexpr by C++ rules?

No :-(. I think that I would rather do that as a follow up. We can try out a few different things to see if we can do something more clever.

jckarter · 2017-11-19T17:47:13Z

stdlib/public/runtime/ImageInspectionELF.cpp

-  initializeSectionLookup(&ProtocolConformanceArgs);
+  const char *conformances = &__start_swift2_protocol_conformances;
+  if (size_t length = &__stop_swift2_protocol_conformances - conformances)
+    addImageProtocolConformanceBlockCallback(conformances, length);
 }


Shouldn't the standard library conformances have already been registered by libswiftCore.so's own static constructor by this point?

Yeah, thats what I expected, but interestingly enough, that was the tweak that fixed everything (minus a copy-pasta error).

compnerd · 2017-11-28T01:02:45Z

Please test with following pull request:
swiftlang/swift-corelibs-foundation#1330

@swift-ci please test

swift-ci · 2017-11-28T01:03:09Z

Build failed
Swift Test Linux Platform
Git Sha - 3a14c81af09b0ebe62cede2dfc051e522aebbc95

swift-ci · 2017-11-28T01:03:15Z

Build failed
Swift Test OS X Platform
Git Sha - 75253f427b5c8f868801556dd339dc4a02616d6d

Restructure the ELF handling to be completely agnostic to the OS. Rather than usng the loader to query the section information, use the linker to construct linker tables and synthetic markers for the beginning and of the table. Save off the values of these pointers and pass them along through the constructor to the runtime for registration. This removes the need for the begin/end objects. Remove the special construction of the begin/end objects through the special assembly constructs, preferring to do this in C with a bit of inline assembly to ensure that the section is always allocated. Remove the special handling for the various targets, the empty object file can be linked on all the targets. The new object file has no requirements on the ordering. It needs to simply be injected into the link. Name the replacement file `swiftrt.o` mirroring `crt.o` from libc. Merge the constructor and the definition into a single object file. This approach is generally more portable, overall simpler to implement, and more robust. Thanks to Orlando Bassotto for help analyzing some of the odd behaviours when switching over.

compnerd · 2017-11-28T18:04:47Z

Please test with following pull request:
swiftlang/swift-corelibs-foundation#1330

@swift-ci please test Linux platform

swift-ci · 2017-11-28T18:06:14Z

Build failed
Swift Test Linux Platform
Git Sha - 2ab930bc9e9bde8f88727fac60ca7d440065a903

compnerd · 2017-11-28T19:25:12Z

@swift-ci please test macOS platform

swift-ci · 2017-11-28T19:29:19Z

Build failed
Swift Test OS X Platform
Git Sha - 2ab930bc9e9bde8f88727fac60ca7d440065a903

compnerd · 2017-11-28T22:13:00Z

Please test with following pull request:
swiftlang/swift-corelibs-foundation#1330
#13118

@swift-ci please test Linux platform

swift-ci · 2017-11-28T22:49:21Z

Build failed
Swift Test Linux Platform
Git Sha - 098b44f7021e1f1f17e09d32031ee63a15795b3c

compnerd · 2017-11-29T02:18:43Z

Please test with following pull request:
swiftlang/swift-corelibs-foundation#1330

@swift-ci please test Linux platform

swift-ci · 2017-11-29T02:45:02Z

Build failed
Swift Test Linux Platform
Git Sha - 098b44f7021e1f1f17e09d32031ee63a15795b3c

compnerd · 2017-11-29T19:37:14Z

Please test with following pull request:
swiftlang/swift-corelibs-foundation#1330

@swift-ci please test macOS platform

swift-ci · 2017-11-29T20:46:59Z

Build failed
Swift Test OS X Platform
Git Sha - 098b44f7021e1f1f17e09d32031ee63a15795b3c

ELF is segment mapped, where the segment which contains a particular section may be mapped to an address which does not correspond to the address on disk. Since the reflection dumper does not use the loader to load the image into memory, we must manually account for any section offsets. Calculate this value when we query the mmap'ed image and wire it through to the relative direct pointer accesses. When switching to the linker table approach for the ELF metadata introspection, this was uncovered as the segment containing the orphaned sections was coalesced into a separate PT_LOAD header which had a non-0 offset for the mapping.

This test fails with LSAN. It is unclear why this is failing with the ELF metadata change. This looks like a latent issue. XFAIL it until we can properly look into this.

compnerd · 2017-11-30T01:57:02Z

@swift-ci please test

swift-ci · 2017-11-30T01:57:27Z

Build failed
Swift Test OS X Platform
Git Sha - 098b44f7021e1f1f17e09d32031ee63a15795b3c

swift-ci · 2017-11-30T02:09:15Z

Build failed
Swift Test Linux Platform
Git Sha - 098b44f7021e1f1f17e09d32031ee63a15795b3c

compnerd · 2017-11-30T06:32:29Z

Please test with following pull request:
swiftlang/swift-corelibs-foundation#1330

@swift-ci please smoke test Linux platform

compnerd · 2017-11-30T16:24:05Z

Please test with following pull request:
swiftlang/swift-corelibs-foundation#1330

@swift-ci please test

aschwaighofer · 2017-11-30T20:24:36Z

Did this break: https://ci.swift.org/job/oss-swift-incremental-RA-linux-ubuntu-14_04/1815 ?

compnerd · 2017-12-01T00:43:46Z

Apparently. For the record, #13180 addressed that failure that the testing didnt catch.

compnerd force-pushed the ELF-registration branch 4 times, most recently from ea3f8e6 to ea16d85 Compare November 6, 2017 16:57

jckarter reviewed Nov 6, 2017

View reviewed changes

jckarter requested changes Nov 6, 2017

View reviewed changes

compnerd force-pushed the ELF-registration branch from ea16d85 to fd82c92 Compare November 16, 2017 22:28

compnerd force-pushed the ELF-registration branch 3 times, most recently from f5d4bb8 to 594e609 Compare November 18, 2017 17:07

compnerd changed the title ~~[DO NOT MERGE] ELF registration~~ ELF registration Restructuring Nov 18, 2017

compnerd force-pushed the ELF-registration branch from 594e609 to 415f552 Compare November 18, 2017 17:52

jckarter reviewed Nov 19, 2017

View reviewed changes

compnerd force-pushed the ELF-registration branch from 5f319c5 to 2ab930b Compare November 28, 2017 01:02

compnerd force-pushed the ELF-registration branch from 2ab930b to 098b44f Compare November 28, 2017 18:04

compnerd added 2 commits November 29, 2017 17:56

test: disable sanitizer_coverage on Linux for now

28dffe7

This test fails with LSAN. It is unclear why this is failing with the ELF metadata change. This looks like a latent issue. XFAIL it until we can properly look into this.

compnerd force-pushed the ELF-registration branch from 098b44f to 28dffe7 Compare November 30, 2017 01:56

compnerd merged commit 9ea988f into swiftlang:master Nov 30, 2017

compnerd deleted the ELF-registration branch November 30, 2017 19:20

jrose-apple mentioned this pull request Dec 20, 2017

[4.1] Revert "reflection: handle LMA != VMA for ELF" #13563

Closed

lanza mentioned this pull request Apr 3, 2018

Remove swiftImageInspectionShared #15692

Closed

ELF registration Restructuring #12758

ELF registration Restructuring #12758

Uh oh!

Conversation

compnerd commented Nov 4, 2017

Uh oh!

compnerd commented Nov 4, 2017

Uh oh!

hpux735 commented Nov 4, 2017

Uh oh!

hpux735 commented Nov 4, 2017

Uh oh!

jrose-apple commented Nov 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compnerd Nov 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

compnerd commented Nov 17, 2017

Uh oh!

zbowling commented Nov 17, 2017

Uh oh!

compnerd commented Nov 17, 2017

Uh oh!

compnerd commented Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jckarter commented Nov 17, 2017

Uh oh!

compnerd commented Nov 17, 2017

Uh oh!

compnerd commented Nov 18, 2017

Uh oh!

swift-ci commented Nov 18, 2017

Uh oh!

compnerd commented Nov 18, 2017

Uh oh!

swift-ci commented Nov 18, 2017

Uh oh!

hpux735 commented Nov 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

compnerd Nov 6, 2017 •

edited

Loading

compnerd commented Nov 17, 2017 •

edited

Loading