Wrap (de)mangling namespaces in an inline namespace. #30733
Conversation
lib/Demangling/CMakeLists.txt
Outdated
| # that someone might want to use, and because it does not appear in source | ||
| # code. | ||
| target_compile_definitions(swiftDemangling PUBLIC | ||
| SWIFT_INLINE_NAMESPACE=cins) |
There was a problem hiding this comment.
This is meant to be embedded within the swift namespace so I don't think that the collision is something to worry about. We could also just use __compiler.
There was a problem hiding this comment.
I'm not attached to these specific names, but I guess I wanted to make sure that if someone did something reasonable down the line, like namespace swift { namespace compiler {, it was clear that that was distinct from this macro-hidden namespace.
I pushed an amended commit with _compiler, since __compiler would technically be a reserved word IIRC.
There was a problem hiding this comment.
Yeah, _compiler should be fine. Ah, okay, I see what you mean - its merely about cognitive costs.
| @@ -45,7 +45,8 @@ if(("${SWIFT_HOST_VARIANT_SDK}" STREQUAL "${SWIFT_PRIMARY_VARIANT_SDK}") AND | |||
| # interfaces. | |||
| target_compile_definitions(SwiftRuntimeLongTests | |||
| PRIVATE | |||
| swiftCore_EXPORTS) | |||
| swiftCore_EXPORTS | |||
| SWIFT_INLINE_NAMESPACE=rtins) | |||
There was a problem hiding this comment.
Similar for the runtime, runtime or __runtime is preferable IMO.
There was a problem hiding this comment.
Used _runtime for the same reasons mentioned above. PTAL.
allevato
force-pushed
the
rt-inline-ns
branch
2 times, most recently
from
9d2694e to
f1f009c
Compare
|
This probably should be rebased. |
|
This is rebased on master now that #30732 is merged, and as we discussed offline, I pushed the inline namespace down into |
|
@swift-ci please test |
|
Build failed |
|
Build failed |
It's a combination of the I imagine if there was a common "choke point" that all runtime targets depended on libDemangling through, we could reduce the cognitive overhead here, but I don't know CMake well enough to know the best way to compose that. If you have any ideas, I'd be happy to try to simplify it. |
|
Hmm, Im not a fan of this, but, I think that it is safe to put that into |
|
|
It looks like lldb may be including Demangling without having the dependency declared, so it's not inheriting the compile definition. I'll verify and prep a PR for that. For the other change, what do you think about just creating two separate |
|
I would say that is the correct approach ... if its not Swift :-(. The separate library should be an |
swiftlang/swift#30733 will begin requiring this in a change that allows the swiftDemangling symbols to be conditionally renamed using an inline namespace so that compiler and runtime symbols don't collide if statically linked in the same binary. The namespace `compiler` was chosen for lldb since that's what would have been used if its build was properly inheriting the public definition from the Swift CMake target.
swiftlang/swift#30733 will begin requiring this in a change that allows the swiftDemangling symbols to be conditionally renamed using an inline namespace so that compiler and runtime symbols don't collide if statically linked in the same binary. The namespace `compiler` was chosen for lldb since that's what would have been used if its build was properly inheriting the public definition from the Swift CMake target.
allevato
force-pushed
the
rt-inline-ns
branch
2 times, most recently
from
7caa63f to
5f7952b
Compare
|
Ok, I've pushed an update that pulls the compile definition up into The only place where the runtime namespace is repeated now is in the unit test targets that link to target Demangling. It's possible we might be able to conditionalize that somehow, but since the same |
|
@compnerd I've fixed up recent merge conflicts; now that the LLDB changes have been merged, can you kick off CI again please? |
|
@swift-ci please test |
|
Build failed |
|
Build failed |
|
Now that lldb is forward-declaring symbols in |
|
@swift-ci please test |
|
Build failed |
|
@swift-ci please clean test |
|
Build failed |
|
@swift-ci please clean test |
|
@swift-ci please test Windows platform |
|
@swift-ci please clean test macOS platform |
|
Build failed |
|
@swift-ci please clean test |
|
Build failed |
Since libDemangling is included in the Swift standard library, ODR violations can occur on platforms that allow statically linking stdlib if Swift code is linked with other compiler libraries that also transitively pull in libDemangling, and if the stdlib version and compiler version do not match exactly (even down to commit drift between releases). This lets the runtime conditionally segregate its copies of the libDemangling symbols from those in the compiler using an inline namespace without affecting usage throughout source.
|
@swift-ci please clean test |
|
@swift-ci please test Windows platform |
|
CC: @mikeash @slavapestov |
This matches the behavior introduced with swiftlang#30733. Addresses rdar://142550635
This matches the behavior introduced with swiftlang#30733 -- quoting the explanation here for the sake of convenience: > Since libDemangling is included in the Swift standard library, > ODR violations can occur on platforms that allow statically > linking stdlib if Swift code is linked with other compiler > libraries that also transitively pull in libDemangling, and if > the stdlib version and compiler version do not match exactly > (even down to commit drift between releases). This lets the > runtime conditionally segregate its copies of the libDemangling > symbols from those in the compiler using an inline namespace > without affecting usage throughout source. Addresses rdar://142550635
Since libDemangling is included in the Swift standard library,
ODR violations can occur on platforms that allow statically
linking stdlib if Swift code is linked with other compiler
libraries that also transitively pull in libDemangling, and if
the stdlib version and compiler version do not match exactly
(even down to commit drift between releases). This lets the
runtime conditionally segregate its copies of the libDemangling
symbols from those in the compiler using an inline namespace
without affecting usage throughout source.
cc: @compnerd