[Reflection] Implement Runtime Attributes #63168
Conversation
|
@swift-ci please test |
Azoy
force-pushed
the
reflection-runtime-attributes
branch
from
c628fea
to
727ff39
Compare
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
Azoy
force-pushed
the
reflection-runtime-attributes
branch
from
727ff39
to
42b5cdc
Compare
|
@swift-ci please test |
| } | ||
|
|
||
| @available(SwiftStdlib 5.9, *) | ||
| @_cdecl("registerAttributes") |
There was a problem hiding this comment.
This puts the function in the global C namespace with the name "registerAttributes". Did you want "swift_registerAttributes" instead?
There was a problem hiding this comment.
Internally, yes, but these symbols are not exported. This is only an issue if a dependency of this library declares some symbol named this. I can change this if you want.
There was a problem hiding this comment.
See my other comments. I think we can refactor this code to not need this function at all.
I'd be happy to offer an additional commit to this PR, or to open a separate PR, to show what I mean. :)
| //===----------------------------------------------------------------------===// | ||
|
|
||
| @available(SwiftStdlib 5.9, *) | ||
| var attributeCache: Lock<[ContextDescriptor: [UnsafeRawPointer]]> = .create( |
There was a problem hiding this comment.
Is this cache actually needed at this point? The cost of walking all the images can't be that much, can it? Could snapshot() just return a copy of the cache (on Darwin) or a copy of the output of swift_enumerateAllMetadataSections() (elsewhere)?
| } | ||
|
|
||
| extern "C" SWIFT_CC(swift) | ||
| void snapshot() { |
There was a problem hiding this comment.
This function is in the global C namespace as "snapshot". Did you want "swift_snapshot" or something like that?
Or perhaps:
void swift_enumerateRuntimeAttributesSections(
void (* body)(const void *start, size_t length, void *context),
void *context
);And have it call directly through to swift_enumerateAllMetadataSections() on Linux, and walk the current snapshot on Darwin? That way, you don't need to expose registerAttributes across language boundaries.
| } | ||
|
|
||
| void imageFunc(const struct mach_header *header, intptr_t size) { | ||
| lookupSection(header, "__TEXT", "__swift5_rattrs", registerAttributes); |
There was a problem hiding this comment.
I think the right thing to do here is have a ConcurrentReadableArray<std::pair<const void *, size_t>> that you add to each time _dyld_register_func_for_add_image() calls its callback. Then, when snapshot() is called (or rather, my enumerating equivalent I described in another comment), you get a snapshot of that, and just walk it.
There was a problem hiding this comment.
This sounds like a better approach to me as well.
Azoy
force-pushed
the
reflection-runtime-attributes
branch
from
42b5cdc
to
886e68b
Compare
|
@swift-ci please test |
Azoy
force-pushed
the
reflection-runtime-attributes
branch
from
886e68b
to
2e92a89
Compare
|
@swift-ci please test |
Azoy
force-pushed
the
reflection-runtime-attributes
branch
from
2e92a89
to
e1824ae
Compare
|
@swift-ci please test |
|
@swift-ci please build toolchain macOS platform |
| } | ||
|
|
||
| auto runtimeAttrs = sections->swift5_runtime_attributes; | ||
| registerAttributes(reinterpret_cast<const void *>(runtimeAttrs.start), |
There was a problem hiding this comment.
I believe this usage will cause sections to be re-added every time Attribute.allInstances() is called.
There was a problem hiding this comment.
Not if I empty the cache before each invocation of snapshot 😉
Azoy
force-pushed
the
reflection-runtime-attributes
branch
from
e1824ae
to
985aafa
Compare
|
@swift-ci please test |
Azoy
force-pushed
the
reflection-runtime-attributes
branch
from
985aafa
to
80c8e94
Compare
|
@swift-ci please test |
add docs fix error Fix linux and windows build initialize the lock value fix metadata section version reverse booleans Update Linux and Windows implementation to correctly cache add include fix linux
Azoy
force-pushed
the
reflection-runtime-attributes
branch
from
80c8e94
to
d5b4853
Compare
|
@swift-ci please test |
| @@ -168,6 +168,12 @@ void swift::initializeDynamicReplacementLookup() { | |||
| void swift::initializeAccessibleFunctionsLookup() { | |||
| } | |||
|
|
|||
| SWIFT_RUNTIME_EXPORT | |||
There was a problem hiding this comment.
Is this change accidental?
| // If our cached image count is less than what's being reported by the | ||
| // Swift runtime, then we need to reset our cache and enumerate all of | ||
| // the images again for updated attributes. | ||
| let currentImageCount = swift_getMetadataSectionCount() |
There was a problem hiding this comment.
This call is unsafe in production—it can race with enumerateSections(). It should only be used for tests where the set of loaded libraries is controlled. :(
| void initializeDyldLookup() { | ||
| static swift::once_t token; | ||
| swift::once(token, []{ | ||
| _dyld_register_func_for_add_image(imageFunc); |
There was a problem hiding this comment.
You should use objc_addLoadImageFunc() here, and only use _dyld_register_func_for_add_image() if the former is not available. The dyld function's callback may be called before the Objective-C runtime has finished initializing its own data structures. See ImageInspectionMachO.cpp for similar code.
Implement the runtime and reflection portion of runtime attributes.