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

Create new test system for name resolution 2.0 #3010

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

powerboat9
Copy link
Contributor

This should allow us to more easily test and record progress on name resolution 2.0

@powerboat9
Copy link
Contributor Author

powerboat9 commented May 18, 2024

Note that, until name resolution 2.0 improves sufficiently, a decent fraction of new tests after this is merged (?) will have to be entered into the exclude file.

@powerboat9 powerboat9 force-pushed the test-nr2 branch 2 times, most recently from 95d701d to ad118fa Compare May 18, 2024 21:21
@powerboat9 powerboat9 marked this pull request as ready for review June 4, 2024 01:43
@CohenArthur
Copy link
Member

@powerboat9 I like the implementation and I think this feature makes sense, but I'm not sure if it's going to make our life easier in the long run. I'm in favor of merging it if the rest of the community thinks it is a good idea. it does add a burden which is maintaining and revisiting that exclude file. but the alternative is also annoying, as we'd need to add a copy of existing testcases with -frust-name-resolution-2.0 added as an argument...

maybe we can do something like keep the nr2 folder, run it with different dejagnu options but only keep symlinks to testcases that pass under NR2.0?

@P-E-P
Copy link
Member

P-E-P commented Jul 18, 2024

maybe we can do something like keep the nr2 folder, run it with different dejagnu options but only keep symlinks to testcases that pass under NR2.0?

AFAIK symlink wouldn't work when running tests on windows.

On one hand I really like this PR's idea but maintaining this list of file looks horrible. I'm in favor of merging this for now, we could maintain that list until the nr2 is stable and working as expected, we could then come back to this decision.

@powerboat9
Copy link
Contributor Author

Thoughts on merging this? Should I make some tweaks?

@P-E-P
Copy link
Member

P-E-P commented Aug 28, 2024

Thoughts on merging this? Should I make some tweaks?

To be honest, multiple peoples are not too keen on the implementation (and so am I). The idea is neat in theory but name resolution 2.0 should be merged shortly once we have fixed the final missing test and this PR brings a burden as we have to update the list. So there are two points to figure out:

  • Do we really want to maintain a list of name resolution 2.0 test like this ? Isn't there a way to automagically add the tests to their own batch
  • Is it too late to separate nr 2.0 tests from the others ? I have one final test to fix before creating a new PR for nr 2.0, this does not mean we will turn it by default as soon as it is merged but the transition should happen fairly early.

Recently I've introduced some tests subdirectories (macros), but in your PR you chose a different approach, with a text list, is there any reason you chose not to got with a subdirectory ? I surely miss a detail somewhere.

@powerboat9
Copy link
Contributor Author

Ah, I didn't realize nr2.0 was so close to completion. The idea was that every compile test for non-nr2.0 would automagically run with nr2.0 as well, except for those in the exclude list. As nr2.0 got closer to completion we could slowly remove tests from the exclude list, and once it was empty we'd be able to enable nr2.0 for all tests and revert this PR. If nr2.0 is about to be completed this would be moot.

@powerboat9
Copy link
Contributor Author

powerboat9 commented Aug 28, 2024

Where is the development on nr2.0 taking place? I think I might be a bit out of the loop

@P-E-P
Copy link
Member

P-E-P commented Aug 29, 2024

Where is the development on nr2.0 taking place? I think I might be a bit out of the loop

I need to open a PR but I've been working on top of #2940. If you want to get an exact idea on what is working/missing you could compile my branch https://github.com/P-E-P/gccrs/tree/allow-use-before-items-being-used-continued. Right now one test is still failing (use duplication, name_resolution21.rs).

@powerboat9
Copy link
Contributor Author

When I try to force-enable nr2.0 on allow-use-before-items-being-used-continued (by replacing flag_name_resolution_2_0 references with 1) I get a bunch of test errors. Are you sure there's only one non-working test?

@P-E-P
Copy link
Member

P-E-P commented Sep 13, 2024

When I try to force-enable nr2.0 on allow-use-before-items-being-used-continued (by replacing flag_name_resolution_2_0 references with 1) I get a bunch of test errors. Are you sure there's only one non-working test?

You're right, I didn't apply the correct patch. Looks like most of those fails are due to an ICE (but not all of them):

  • rust/resolve/rust-late-name-resolver-2.0.cc:206
  • rust/resolve/rust-late-name-resolver-2.0.cc:157
  • rust/hir/rust-ast-lower-base.cc:655
  • rust/resolve/rust-name-resolver.cc:661
  • We also have a segfault somewhere

The situation is worse than I initially thought.

Copy link
Member

@P-E-P P-E-P left a comment

Choose a reason for hiding this comment

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

I can't fix those in the upcoming PR as it is becoming too big. And there is a lot of work remaining, we should merge this PR.

@P-E-P P-E-P added this pull request to the merge queue Sep 13, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 13, 2024
@P-E-P P-E-P added this pull request to the merge queue Sep 13, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 14, 2024
@P-E-P P-E-P added this pull request to the merge queue Sep 14, 2024
@P-E-P P-E-P removed this pull request from the merge queue due to a manual request Sep 14, 2024
@P-E-P
Copy link
Member

P-E-P commented Sep 14, 2024

Wait I thought the error was due to some github queue shenanigans but it doesn't. @powerboat9 would you mind rebasing this PR on master ?

@powerboat9
Copy link
Contributor Author

Yep, looks like it needs some tweaks after some tests were moved around. Will need to be adjusted to handle sub directories properly.

@CohenArthur CohenArthur added this pull request to the merge queue Sep 16, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 16, 2024
@powerboat9 powerboat9 marked this pull request as draft September 16, 2024 18:42
This runs the standard compile/**.rs tests
with name resolution 2.0 enabled. The exclude file
can be used to exclude tests which are not yet working
with name resolution 2.0.

gcc/testsuite/ChangeLog:

	* rust/compile/nr2/compile.exp: New test.
	* rust/compile/nr2/exclude: New.

Signed-off-by: Owen Avery <powerboat9.gamer@gmail.com>
@powerboat9 powerboat9 marked this pull request as ready for review September 16, 2024 22:30
@powerboat9
Copy link
Contributor Author

Alright, this should do it. We should also now get XPASS reports when tests in the exclude list no longer need to be excluded.

@P-E-P P-E-P added this pull request to the merge queue Sep 17, 2024
Merged via the queue into Rust-GCC:master with commit 483ee3a Sep 17, 2024
11 of 12 checks passed
@powerboat9 powerboat9 deleted the test-nr2 branch September 17, 2024 19:08
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.

3 participants