[Frontend] Turn symbols-missing-from-TBD validation on by default in debug builds on Apple platforms. #16838
Conversation
|
@swift-ci please test |
|
@swift-ci please test source compatibility |
|
I believe #16959 fixes the last of the source compat failures. |
|
@swift-ci please test source compatibility |
2 similar comments
|
@swift-ci please test source compatibility |
|
@swift-ci please test source compatibility |
|
@swift-ci please smoke test |
| @@ -246,7 +246,13 @@ class FrontendOptions { | |||
| }; | |||
|
|
|||
| /// Compare the symbols in the IR against the TBD file we would generate. | |||
| TBDValidationMode ValidateTBDAgainstIR = TBDValidationMode::None; | |||
| TBDValidationMode ValidateTBDAgainstIR = | |||
| #if !defined(NDEBUG) && defined(__APPLE__) | |||
There was a problem hiding this comment.
Should this be a target check instead, in case someone builds a cross-compiler on macOS?
There was a problem hiding this comment.
I couldn't tell if this was a target or host check, and if it wasn't, couldn't work out the target check. Tips?
There was a problem hiding this comment.
Target checks are always performed at run time, and since the target isn't stored in FrontendOptions itself you'd have to do it at a bit higher level. There's not a super clean way to do this in the argument parsing in the frontend, though, because frontend options are processed first, before even the -target option. (Check out CompilerInvocation::parseArgs.)
One way to deal with this could be to make the field Optional, and then fall back at the use site based on the target.
…debug builds on Apple platforms. TBD validation is effectively an expensive assertion, and is currently only tuned for Apple platforms. However, we don't want it to regress more, and it would be nice to start getting validation from people using master snapshots. Together, this means that turning it on by default for the cases mentioned above is an appropriate course of action. At the very least, this has the benefit of running validation across the stdlib, the overlays and the whole testsuite on each build, so people making changes to the compiler that change symbols are hopefully alerted. One limitation here is that this is only validating that the TBD is a superset of the true set of symbols: it could include spurious symbols that aren't actually in the binary. This case is less problematic for Swift than symbols missing from the TBD file, and so we've focused energy on this. Once we've fixed the extra-symbols problems and are confident in it, this validation can be upgraded to validate that too. Half of rdar://problem/40431434.
huonw
force-pushed
the
validate-tbd-by-default
branch
from
2c2b88a to
caa3dd4
Compare
|
@swift-ci please test |
|
Build failed |
|
Build failed |
|
@swift-ci please test |
|
@swift-ci please test source compatibility |
TBD validation is effectively an expensive assertion, and is currently only
tuned for Apple platforms. However, we don't want it to regress more, and it
would be nice to start getting validation from people using master
snapshots. Together, this means that turning it on by default for the cases
mentioned above is an appropriate course of action.
At the very least, this has the benefit of running validation across the stdlib,
the overlays and the whole testsuite on each build, so people making changes to
the compiler that change symbols are hopefully alerted.
One limitation here is that this is only validating that the TBD is a superset
of the true set of symbols: it could include spurious symbols that aren't
actually in the binary. This case is less problematic for Swift than symbols
missing from the TBD file, and so we've focused energy on this. Once we've fixed
the extra-symbols problems and are confident in it, this validation can be
upgraded to validate that too.
Half of rdar://problem/40431434.
(This PR currently includes #16837, but that should land earlier, since I want to test this a little more extensively.)