Skip to content

Fix failing unit tests under ARM#960

Merged
volsa merged 7 commits intomasterfrom
arm
Sep 8, 2023
Merged

Fix failing unit tests under ARM#960
volsa merged 7 commits intomasterfrom
arm

Conversation

@volsa
Copy link
Copy Markdown
Member

@volsa volsa commented Sep 5, 2023

This PR does does two things to fix the failing ARM test:

  1. It replaces compile_and_run_no_params with compile_and_run for String-Function tests, which would otherwise fail due to invalid pointer access (we previously did something similar)
  2. (My understanding of this and it might not be 100% correct) It ignores some #[should_panic] stdlib tests under ARM because of undefined behaviour. More specifically some of our FFI functions call panic!, the current rustc implementation however assumes that any extern "C" function cannot unwind and doing so would trigger undefined behaviour. Consequentially the #[should_panic] attribute isn't able to catch panics / unwind the stack. I did some experiments if we could bypass this somehow (see code below) but while these tests worked under x86_64, they did not under ARM. One potential fix to this would be to not panic inside FFI functions and instead return error codes which would be a somewhat big-ish refactor. Luckily there's a RFC being stabilized, so maybe in a few Rust versions we can just change the function signature of FFIs from pub extern "C" to pub extern "C-unwind", potentially fixing this issue.

TL;DR: Wait for some future Rust version where C-unwind is introduced, in the meantime #[ignore] some tests under ARM.

Some more references I found

...and the bypass attempts

#[test]
fn right_string_substring_too_long() {
    let src = r#"
	    FUNCTION main : STRING
	    VAR_TEMP
	        in : STRING;
	    END_VAR
	        in := 'sample text';
	        main := RIGHT(in, 12);
	    END_FUNCTION
        "#;

    // These attempts worked under x86_64, but not under aarch64.
    // 1. Attempt: Run code inside catch_unwind closure and check if the result is an error (= paniced).
    let catch = std::panic::catch_unwind(|| {
        let sources = add_std!(src, "string_functions.st");
        let _: [u8; 81] = compile_and_run_no_params(sources);
    });
    assert!(catch.is_err());

    // 2. Attempt: Spawn a thread, join it and check if the thread result is an error (= paniced).
    let thread = std::thread::spawn(move || {
        let sources = add_std!(src, "string_functions.st");
        let _: [u8; 81] = compile_and_run_no_params(sources);
    });
    assert!(thread.join().is_err());
}

@volsa volsa linked an issue Sep 5, 2023 that may be closed by this pull request
@volsa volsa changed the title Fix ARM unit tests Fix failing unit tests under ARM Sep 5, 2023
@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 5, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02% 🎉

Comparison is base (fe3a9e2) 95.42% compared to head (76c9ef2) 95.45%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #960      +/-   ##
==========================================
+ Coverage   95.42%   95.45%   +0.02%     
==========================================
  Files         152      152              
  Lines       40774    40818      +44     
==========================================
+ Hits        38909    38962      +53     
+ Misses       1865     1856       -9     
Files Changed Coverage Δ
.../stdlib/tests/date_time_numeric_functions_tests.rs 100.00% <ø> (ø)
libs/stdlib/src/string_functions.rs 98.18% <100.00%> (ø)
libs/stdlib/tests/string_function_tests.rs 97.12% <100.00%> (+0.08%) ⬆️

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@volsa volsa requested a review from ghaith September 7, 2023 12:29
@volsa volsa marked this pull request as ready for review September 7, 2023 12:29
@volsa volsa merged commit 87663fb into master Sep 8, 2023
@volsa volsa deleted the arm branch September 8, 2023 07:15
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.

Failing tests under ARM

2 participants