Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[validation-test] More granular FixedPoint tests #1324

Merged

Conversation

modocache
Copy link
Contributor

FixedPoint.swift.gyb generates a massive amount of assertions: one for each Int type (such as UInt8 and Int64) as a source, to one of each Int type as a destination, for each of 27 bit patterns. By my math that's 8 * 8 * 27 == 1728 assertions. As a result, when one of those assertions fails, it's difficult to
tell what went wrong.

Split each assertion into its own test case. This makes the test take a little longer to run, but it produces much more valuable output when it fails.

`FixedPoint.swift.gyb` generates a massive amount of assertions:
one for each `Int` type (such as `UInt8` and `Int64`) as a source,
to one of each `Int` type as a destination, for each of 27 bit
patterns. By my math that's 8 * 8 * 27 == 1728 assertions.
As a result, when one of those assertions fails, it's difficult to
tell what went wrong.

Split each assertion into its own test case. This makes the test
take a little longer to run, but it produces much more valuable
output when it fails.
@modocache
Copy link
Contributor Author

I would mark this as "NFC", but it actually fixes a SIGSEGV when the test is run on android-armv7. I think the change is beneficial for all of the supported Swift architectures regardless, but for the curious, I've written up what I learned debugging android-armv7 here: SwiftAndroid#17. Feedback welcome!

@gribozavr
Copy link
Contributor

@modocache I don't see a problem accepting this PR, but the fact that long functions cause silent miscompiles is alarming. I recommend you to investigate further, taking the .swift input file and trying to reduce it to a minimal input that crashes. You can use delta (http://delta.tigris.org/) for that.

@gribozavr
Copy link
Contributor

@swift-ci Please test

@modocache
Copy link
Contributor Author

@gribozavr Absolutely will do. Thanks for the delta tip!

@modocache
Copy link
Contributor Author

Weird, it looks like CI encountered an error in test/IRGen/protocol_resilience.sil, which should be unrelated. Perhaps a stochastic failure?

@gribozavr
Copy link
Contributor

I think master is broken... I'm going to revert something and try again.

@gribozavr
Copy link
Contributor

I have fixed test/IRGen/protocol_resilience.sil in c8a3579

@modocache
Copy link
Contributor Author

@gribozavr I believe I may be running into the same issue in another test: validation-test/stdlib/Sort.swift.gyb. That test mentions that gyb has trouble with nested loops and references rdar://problem/1890035. This test references rdar://problem/17548877. I'm going to try using delta to narrow things down, but if those radars have some relevant information could you share it with me? 🙇

@gribozavr
Copy link
Contributor

rdar://problem/17548877 is fixed. It was about this code:

$ cat ../a.gyb
% foo = 1
% for bar in [ 10, 20 ]:
twice
%   foo += 1
% end

once

rdar://problem/18900352 is also fixed. It was about this code:

% for i1 in [1, 2]:
%   for i2 in [10, 20]:
%     i3 = i1 + i2
${i1} + ${i2} = ${i3}
% end

Now both snippets seem to execute correctly.

@gribozavr
Copy link
Contributor

@modocache It is likely that you are hitting a different bug in the same area.

@modocache
Copy link
Contributor Author

@gribozavr Excellent, thanks for the tips. I'll continue to look into it!

gribozavr added a commit that referenced this pull request Feb 16, 2016
…-fixedpoint

[validation-test] More granular FixedPoint tests
@gribozavr gribozavr merged commit 9e9d0e7 into swiftlang:master Feb 16, 2016
@modocache modocache deleted the more-granular-validation-test-fixedpoint branch February 16, 2016 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants