rust-lang / rustc_codegen_gcc

libgccjit AOT codegen for rustc
Apache License 2.0
893 stars 61 forks source link

ui pattern failure tests #524

Closed lordshashank closed 3 weeks ago

lordshashank commented 1 month ago
lordshashank commented 1 month ago

@antoyo after ideating for hours on how to proceed and looking for various options like implementing completely new function for this, including simple bool param in test_rustc_inner for this, making global bool variable to include/exclude these tests, etc. I thought best option would be to include dynamic boxed closure callback function as parameter to test_rustc_inner function, this would enable us to implement separate tests/ci jobs for other checks pattern in different test suites if we implement in future. Let me know if you have any better way and if this didn't worked correctly, thanks.

antoyo commented 1 month ago

To make this useful, we would need the test to fail when there's an ICE (internal compiler error).

We can see in the logs that some tests have an ICE:

2024-05-26T23:54:45.8957411Z thread 'rustc' panicked at compiler/rustc_trait_selection/src/traits/select/confirmation.rs:795:50:
2024-05-26T23:54:45.8957573Z future has no bound vars
2024-05-26T23:54:45.8957837Z note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
2024-05-26T23:54:45.8957843Z 
2024-05-26T23:54:45.8958035Z error: the compiler unexpectedly panicked. this is a bug.

Maybe we could grep for the compiler unexpectedly panicked and fail the test in this case. We would then need a file for to ignore the tests that are known to produce an ICE.

It would also be interesting to check if this is a good strategy. Perhaps some tests also produce an ICE with the LLVM codegen. Perhaps there is an annotation for the tests that produce an ICE even with LLVM? Perhaps it is known-bug?

lordshashank commented 1 month ago

@antoyo where are these logs from? From workflow run I can see that around 474 ui-pattern-failure-test passed. Do you mean that some of them have an ICE and we should fail them manually?

antoyo commented 1 month ago

To see those logs, I went to the failure workflow and clicked on "View raw logs" (because the logs are too long to be shown on the previous page).

antoyo commented 1 month ago

Yeah, we should fail the CI when there's an ICE, but please consider the notes I mentioned above.

lordshashank commented 1 month ago

@antoyo what do you mean by

we would need the test to fail when there's an ICE

all the cases for ICE (with "the compiler unexpectedly panicked") that I see in logs are for the ui tests that are already failing.

antoyo commented 1 month ago

Indeed, but the overall CI task does not fail because we eat the exit code. I would want the CI task to fail when an ICE is detected (except if it is expected: see notes above).

lordshashank commented 1 month ago

@antoyo I checked few of them manually, could get "known-bug" in the one you shared above only. Could you tell me how can I grep for the compiler unexpectedly panicked to get the tests that are giving this as all this is being run through exec_command function, not sure how to get logs to grep this.

antoyo commented 1 month ago

You can see the logs by going to this page, then click on the gear at the top right and either select "View raw logs" or "Download log archive". You need to do this since the logs are too long to be shown in the page.

lordshashank commented 1 month ago

Ah! I already did that, is there only manual way to get the tests with ICE from finding the logs, I was thinking of some programmatic way. (there are many, approx 100 such cases in logs)

antoyo commented 1 month ago

You mean, finding the name of those tests?

lordshashank commented 1 month ago

yes (name or file) , cause then only i would be able to create txt file to omit them or check for "known-bug", right?

antoyo commented 1 month ago

Yes.

I do not know any programmatic way to do this.

@GuillaumeGomez mentioned to me the results are in JSON at some point, but we don't know if we could get the JSON.

GuillaumeGomez commented 1 month ago

To give more details: there is the tester which runs ui test with the --output-format json, then parses this output and checks if it's expected or not. I don't think this tester itself has an option to output json though.

lordshashank commented 1 month ago

Ok, so had to do a lot here. First I looked for the ICE failures using script on logs downloaded, checked for known bug found just one case you shared. Was reluctant to check for any other error pattern as didn't seem universal here, and could have led to removal of some successfull test as once happened before. Next, while getting the compiler unexpectedly panicked found one case in run-make tests as well here. But I guess this was expected here. Then, had do callback for tests, improved some function naming there (let me know if thats an issue). For CI failure, had to grep from logs, found no other way, let me know if you have one. Also, @antoyo I mistakingly raised PR to this branch rather than yours, let me know if I have close this and raise there. Huh 😅 Will look why the runs failed next. Thanks.

lordshashank commented 1 month ago

@antoyo ICE tests should get removed through the callback here, I tried to log them here and the failing tests were not there. Still the CI run took those tests leading to failing of CI. Could you help me know what am I missing here?

antoyo commented 1 month ago

From what I can see, the command ./y.sh test --release --clean --build-sysroot --test-failing-ui-pattern-tests will not run the tests from the file failing-ice-tests.txt. If we still see failures of tests like tests/ui/sepcomp/sepcomp-statics.rs, they happen before we run this command.

In fact, we see them in the command ./y.sh test --release --clean --build-sysroot --test-failing-rustc because the file tests/failing-ui-tests.txt lists those same tests tests/ui/sepcomp/sepcomp-statics.rs.

(You can search for "build system" in the output to see the command that is ran about 14 lines above "build system".)

I tried to log them here and the failing tests were not there

For the record, this is normal that you don't see them here because they were removed by the call to prepare_files_callback_remove.

All of that to say, the CI fails because some other tests (for instance tests/ui/extern/issue-64655-allow-unwind-when-calling-panic-directly.rs gives an ICE), but these tests are not removed (currenty because the callback should_run_test_callback returns true), so I believe you need to add these tests to the file failing-ice-tests.txt.

lordshashank commented 1 month ago

ok, so it was that. Actually I looked at logs and saw same files that I omitted in failing-ice-tests.txt , thus thought they were not getting omitted, but actually they were from another ci task, sorry. I guess CI has passed but I got a mail of it failing, not sure why. @antoyo let me know if anything else is needed here.

lordshashank commented 3 weeks ago

@antoyo added the bool function, let me know if anything else is needed now. Also updated description.

lordshashank commented 3 weeks ago

@antoyo ig we are good to go now.

lordshashank commented 3 weeks ago

LFG 🚀