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

Don't crash for invalid toplevel parseStmt/Expr calls #23089

Merged
merged 10 commits into from
Dec 19, 2023

Conversation

ire4ever1190
Copy link
Contributor

This code will crash check/nimsuggest since the ra register is uninitialised

import macros

static:
  discard parseExpr("'")

Now it assigns an empty node so that it has something

Testament changes were so I could properly write a test. It would pass even with a segfault since it could find the error

@Araq Araq added the merge_when_passes_CI mergeable once green label Dec 17, 2023
@ire4ever1190
Copy link
Contributor Author

ire4ever1190 commented Dec 18, 2023

Failure is related, investigating

Exit codes seem to always be 0 for rejection on windows

Fixed: Issue was the error code was read after the process was closed which made it always return 0 on windows

@ire4ever1190 ire4ever1190 force-pushed the fix/recover-invalid-parse branch 2 times, most recently from 057b73a to f141376 Compare December 18, 2023 09:49
Needed this for writing the test so that I can still check for errors while also testing that the compiler didn't crash
The error flag is checked after anyways so this shouldn't cause problem

This stops the segfault so it is accessing something

Alternative solution is to check the error flag ourselves and assign an empty node (Or maybe an error node?) if something goes wrong
File never failed so exit code differing is better error than files differing
…fter messages

The other stuff isn't failing for me :/ so think this is the safest option since it should keep backwards compaitability
On windows `peekExitCode` will always return 0 if read after the process is closed
Now I just check the suffix so that editing the file won't have affects
@Araq Araq merged commit db9d800 into nim-lang:devel Dec 19, 2023
19 checks passed
Copy link
Contributor

Thanks for your hard work on this PR!
The lines below are statistics of the Nim compiler built from db9d800

Hint: mm: orc; opt: speed; options: -d:release
177494 lines; 7.542s; 765.922MiB peakmem

@ire4ever1190 ire4ever1190 deleted the fix/recover-invalid-parse branch December 21, 2023 06:53
narimiran pushed a commit that referenced this pull request Apr 19, 2024
This code will crash `check`/`nimsuggest` since the `ra` register is
uninitialised

```nim
import macros

static:
  discard parseExpr("'")
```
Now it assigns an empty node so that it has something

Testament changes were so I could properly write a test. It would pass
even with a segfault since it could find the error

(cherry picked from commit db9d800)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge_when_passes_CI mergeable once green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants