stdlib: fix the problem swept under the rug in 58a97f1603 #78895
Conversation
|
@swift-ci please test |
|
I'm not sure I'm sold on this; I appreciate that the code you're replacing here is UB, but I don't think we necessarily want to replace it with a runtime test in the constructor that's allegedly inefficient in some cases. If we're going to replace it with a runtime test, then IMO that test belongs in the test suite somewhere; we don't want to end up potentially paying a runtime cost every time we construct one of these objects. |
|
I'm fine with this going to a test. However, I think that I would rather that we do this once with a global constructor. I'll be following up with that soon. |
We don't really like global constructors — they cost us start-up time, so we generally try to avoid them. I know there are some, and I also know I added some of them myself, but I'd be strongly against using one to check a type at runtime that we could test in a unit test instead. The right place for this kind of check if we can't do it statically at compile time is in the test suite. |
|
I suppose that I am unsure how to do that in a unit test. I don't have a problem with this being sunk outside of the runtime. |
compnerd
force-pushed
the
dynamic-offsets
branch
from
874f811 to
9bfae70
Compare
|
@swift-ci please test |
|
@swift-ci please test Windows platform |
compnerd
force-pushed
the
dynamic-offsets
branch
from
9bfae70 to
03b2c6d
Compare
|
@swift-ci please test |
| // error: 'Value' is a private member of | ||
| // 'swift::VariadicMetadataCacheEntryBase<swift::GenericCacheEntry>' |
There was a problem hiding this comment.
Could probably use a friend class to work around that (e.g. add friend class MetadataCacheEntryTesting to VariadicMetadataCacheEntryBase then define the class in unittests/runtime/TypeLayoutChecks.cpp and have that contain a function that does the ASSERT_EQ or maybe just the offset_of calculations.
There was a problem hiding this comment.
The inline function definition sounds interesting - I think that doing that as a follow up makes sense.
The `-Winvalid-offsetof` warning is valid in this case. `offsetof` is being applied to types with a non-standard layout. The layout of this type is undefined by the specification. There is no guarantee that the type layout is uniform across all ABIs. It is not possible to portably compute the offset statically, especially efficiently. Sink this check into a unit test to avoid performing this test at runtime. In order to do this in the standard library, we would need to do this check through a global constructor.
compnerd
force-pushed
the
dynamic-offsets
branch
from
03b2c6d to
0174528
Compare
|
@swift-ci please smoke test |
|
@swift-ci please smoke test macOS platform |
The
-Winvalid-offsetofwarning is valid in this case.offsetofis being applied to types with a non-standard layout. The layout of this type is undefined by the specification. There is no guarantee that the type layout is uniform across all ABIs. It is not possible to portably compute the offset statically, especially efficiently.