Skip to content

Conversation

0xllx0
Copy link
Contributor

@0xllx0 0xllx0 commented Sep 12, 2025

Thank you for making Rust GCC better!

If your PR fixes an issue, you can add "Fixes #issue_number" into this
PR description and the git commit message. This way the issue will be
automatically closed when your PR is merged. If your change addresses
an issue but does not fully fix it please mark it as "Addresses #issue_number"
in the git commit message.

Here is a checklist to help you with your PR.

Note that you can skip the above if you are just opening a WIP PR in
order to get feedback.

Converts an assert into an early return during AST parsing.

Resolves: #4145

@@ -0,0 +1,8 @@
struct S {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure on the exact decorators needed here, or if this test should go into the xfail directory. The test is expected to fail compilation, just with an error message instead of an ICE.

Copy link
Collaborator

Choose a reason for hiding this comment

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

grep for dg-error in the testsuite directory -- that should give you some nice examples for expecting error messages

Copy link
Member

@CohenArthur CohenArthur left a comment

Choose a reason for hiding this comment

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

one tiny nitpick but the changes are really good, thank you for the work!

@0xllx0 0xllx0 force-pushed the fix/issue-4145 branch 3 times, most recently from 98c8cc2 to b84e8ac Compare September 18, 2025 16:38
@philberty
Copy link
Member

You need to fixup the regressions here those tests probably emit a new error now and they need updated accordingly

@0xllx0
Copy link
Contributor Author

0xllx0 commented Sep 26, 2025

You need to fixup the regressions here those tests probably emit a new error now and they need updated accordingly

I rebased on the latest master (32ce955), and didn't get a failing test for the gcc/testsuite/rust/compile/issue-4145.rs test. Is there something specific where you are noticing regressions? Can you please provide reproduction steps so I can work on resolving the issue?

struct S {
field: [u8; {
#[path = "outer/inner.rs"]
// { dg-error "error handling module file for .inner." "#4145" { xfail *-*-* } .-1 }
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think .-1 should be .1, if you want to catch an error on the next line instead of the previous line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I changed it to .+1, and changed xfail to target since the compiler successfully returns an error (opposed to an ICE). Please let me know if that logic makes sense to you.

Converts an assert into an early return during AST parsing.

Resolves: Rust-GCC#4145

gcc/rust/ChangeLog:

	* ast/rust-ast.cc (Module::process_file_path): empty module early return

Signed-off-by: Elle Rhumsaa <[email protected]>
@0xllx0 0xllx0 force-pushed the fix/issue-4145 branch 3 times, most recently from 4559576 to b388091 Compare September 30, 2025 05:11
@P-E-P
Copy link
Member

P-E-P commented Sep 30, 2025

I rebased on the latest master (32ce955), and didn't get a failing test for the gcc/testsuite/rust/compile/issue-4145.rs test. Is there something specific where you are noticing regressions? Can you please provide reproduction steps so I can work on resolving the issue?

I can't remember the state of that PR when you first submitted it. This message seems to be out of date (there are multiple tests that are not passing anymore). Do you struggle with it ? Do you need some help ?

You probably need more constraints over your condition. Try executing the test using GDB to understand why it emits an error. Note that the tests may also have a slight chance of being wrong, even though I doubt it.

gcc/testsuite/ChangeLog:

	* rust/compile/issue-4145.rs: New test.

Signed-off-by: Elle Rhumsaa <[email protected]>
@0xllx0
Copy link
Contributor Author

0xllx0 commented Sep 30, 2025

Do you struggle with it ? Do you need some help ?

I don't know enough at this point about the internals of gccrs to determine exactly why the early return is breaking the other tests. The test for issue-4145.rs passes, but obviously the other tests break.

Not sure what the best way forward is.

You probably need more constraints over your condition.

Possibly, but I currently lack the appropriate context for what the test condition should be that captures the motivating edge-case, and doesn't break the rest of the test suite. It will take me some time to get more familiar with the surrounding code.

Try executing the test using GDB to understand why it emits an error. Note that the tests may also have a slight chance of being wrong, even though I doubt it.

Thanks, that will probably help trace the relevant code paths.

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.

ICE in process_file_path, at rust/ast/rust-ast.cc:3386 , path name attr == mod name
5 participants