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

tests: Simplify VaList run-make test #56396

Merged
merged 1 commit into from
Dec 2, 2018

Conversation

dlrobertson
Copy link
Contributor

@dlrobertson dlrobertson commented Dec 1, 2018

The va_list tests were too complex and were causing some spurious
test failures on Windows.

Example: #55011 (comment)

The va_list tests were too complex and were causing some spurious
test failures on Windows
@rust-highfive
Copy link
Collaborator

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 1, 2018
@nikic
Copy link
Contributor

nikic commented Dec 1, 2018

It's not obvious to me why the previous implementation is triggering assertions. struct Answer is #[repr(C)], so I would expect this to guarantee that the alignment matches on the Rust and the C side.

@nikic
Copy link
Contributor

nikic commented Dec 1, 2018

Okay, I get it now. Turns out that MSVC on x86-32 uses 4-byte stack alignment. Even though the alignment of the structure (in the sense of _Alignof()) is higher, it may still only be 4-byte alignment on the stack, unless you explicitly specify __declspec(align(N)).

@nikic

This comment has been minimized.

@bors

This comment has been minimized.

@bors

This comment has been minimized.

pub skip_ty: AnswerType,
unsafe fn compare_c_str(ptr: *const c_char, val: &str) -> bool {
let cstr0 = CStr::from_ptr(ptr);
let cstr1 = CString::new(val).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this fails, this would panic across an FFI boundary, which is UB. Not expected to happen, but doing something more conservative (return false / abort) would be better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW this'll soon become a deterministic abort (not UB), so this is probably fine to leave in

@alexcrichton
Copy link
Member

@bors: r=nikic p=10

(hopefully fixing spurious failures!)

@bors
Copy link
Contributor

bors commented Dec 1, 2018

📌 Commit 28ca35f has been approved by nikic

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 1, 2018
@nikic nikic mentioned this pull request Dec 1, 2018
@bors
Copy link
Contributor

bors commented Dec 2, 2018

⌛ Testing commit 28ca35f with merge af7554d...

bors added a commit that referenced this pull request Dec 2, 2018
tests: Simplify VaList run-make test

The va_list tests were too complex and were causing some spurious
test failures on Windows.

Example: #55011 (comment)
@dlrobertson
Copy link
Contributor Author

Okay, I get it now. Turns out that MSVC on x86-32 uses 4-byte stack alignment. Even though the alignment of the structure (in the sense of _Alignof()) is higher, it may still only be 4-byte alignment on the stack, unless you explicitly specify __declspec(align(N)).

Exactly. Didn't compile locally with debug_assertions or think of this when I first wrote the test.

@bors
Copy link
Contributor

bors commented Dec 2, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikic
Pushing af7554d to master...

@bors bors merged commit 28ca35f into rust-lang:master Dec 2, 2018
@dlrobertson dlrobertson deleted the fix_va_list_tests branch December 2, 2018 02:56
@workingjubilee workingjubilee added the F-c_variadic `#![feature(c_variadic)]` label Jun 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-c_variadic `#![feature(c_variadic)]` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants