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

roc dev: report the status code of the child process #5557

Merged
merged 5 commits into from
Oct 21, 2023

Conversation

folkertdev
Copy link
Contributor

fix #5428

we now report the status code of the child process, which is important if the status code is nonzero (indicating failure)

@folkertdev folkertdev requested a review from Anton-4 June 16, 2023 18:34
@folkertdev
Copy link
Contributor Author

@Anton-4 so we actually get some failures reported now. did you look into those errors already (I'm not sure what prompted the issue #5428)

@Anton-4
Copy link
Collaborator

Anton-4 commented Jun 17, 2023

Thanks @folkertdev :)
I can't find what failure I was analyzing when I posted #5428, btw GPT4 says unix_wait_status(35584) is a segfault.

@github-actions
Copy link

Thank you for your contribution! Sometimes PRs end up staying open for a long time without activity, which can make the list of open PRs get long and time-consuming to review. To keep things manageable for reviewers, this bot automatically closes PRs that haven’t had activity in 60 days. This PR hasn’t had activity in 30 days, so it will be automatically closed if there is no more activity in the next 30 days. Keep in mind that PRs marked Closed are not deleted, so no matter what, the PR will still be right here in the repo. You can always access it and reopen it anytime you like!

@github-actions
Copy link

Thank you for your contribution! Sometimes PRs end up staying open for a long time without activity, which can make the list of open PRs get long and time-consuming to review. To keep things manageable for reviewers, this bot automatically closes PRs that haven’t had activity in 60 days. This PR hasn’t had activity in 30 days, so it will be automatically closed if there is no more activity in the next 30 days. Keep in mind that PRs marked Closed are not deleted, so no matter what, the PR will still be right here in the repo. You can always access it and reopen it anytime you like!

@Anton-4
Copy link
Collaborator

Anton-4 commented Oct 10, 2023

All the cli_run tests pass now :)
But we do have some segfaults remaining:

glue_cli_run::nullable_unwrapped
glue_cli_run::nullable_wrapped

I'll do a quick valgrind check tomorrow.

@Anton-4
Copy link
Collaborator

Anton-4 commented Oct 11, 2023

valgrind ./crates/glue/tests/fixtures/nullable-wrapped/app
==708913== Memcheck, a memory error detector
==708913== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==708913== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
==708913== Command: ./crates/glue/tests/fixtures/nullable-wrapped/app
==708913== 
tag_union was: StrFingerTree::More("foo", StrFingerTree::More("bar", StrFingerTree::Empty))
`More "small str" (Single "other str")` is: StrFingerTree::More("small str", StrFingerTree::Single("other str"))
`More "small str" Empty` is: StrFingerTree::More("small str", StrFingerTree::Empty)
`Single "small str"` is: StrFingerTree::Single("small str")
`Empty` is: StrFingerTree::Empty
==708913== Can't extend stack to 0x485d138 during signal delivery for thread 1:
==708913==   no stack segment
==708913== 
==708913== Process terminating with default action of signal 11 (SIGSEGV)
==708913==  Access not within mapped region at address 0x485D138
==708913==    at 0x249024: ??? (in /home/small-ci-user/gitrepos/roc2/roc/crates/glue/tests/fixtures/nullable-wrapped/app)
==708913==    by 0x48D1494: __run_exit_handlers (exit.c:113)
==708913==    by 0x48D160F: exit (exit.c:143)
==708913==    by 0x1D7266: ??? (in /home/small-ci-user/gitrepos/roc2/roc/crates/glue/tests/fixtures/nullable-wrapped/app)
==708913==    by 0x1: ???
==708913==    by 0x1CC95E: ??? (in /home/small-ci-user/gitrepos/roc2/roc/crates/glue/tests/fixtures/nullable-wrapped/app)
==708913==  If you believe this happened as a result of a stack
==708913==  overflow in your program's main thread (unlikely but
==708913==  possible), you can try to increase the size of the
==708913==  main thread stack using the --main-stacksize= flag.
==708913==  The main thread stack size used in this run was 8388608.
==708913== 
==708913== HEAP SUMMARY:
==708913==     in use at exit: 600 bytes in 15 blocks
==708913==   total heap usage: 27 allocs, 12 frees, 3,833 bytes allocated
==708913== 
==708913== LEAK SUMMARY:
==708913==    definitely lost: 400 bytes in 10 blocks
==708913==    indirectly lost: 200 bytes in 5 blocks
==708913==      possibly lost: 0 bytes in 0 blocks
==708913==    still reachable: 0 bytes in 0 blocks
==708913==         suppressed: 0 bytes in 0 blocks
==708913== Rerun with --leak-check=full to see details of leaked memory
==708913== 
==708913== For lists of detected and suppressed errors, rerun with: -s
==708913== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
Segmentation fault (core dumped)

@Anton-4
Copy link
Collaborator

Anton-4 commented Oct 11, 2023

@Anton-4
Copy link
Collaborator

Anton-4 commented Oct 11, 2023

Running with --debug did not clear up the ??? in the valgrind output.

@Anton-4
Copy link
Collaborator

Anton-4 commented Oct 21, 2023

I've ignored the nullable_unwrapped and nullable_wrapped tests (on linux only). This PR would help us detect newly introduced errors so it seems smart to ignore those tests and get this merged in.

@ayazhafiz
Copy link
Member

Wait, are we sure that this PR does not cause the glue failures? It is odd to me that it would SIGSEGV on this branch but not elsewhere.

Comment on lines -1105 to +1111
std::process::exit(0)
std::process::exit(exit_code)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ayazhafiz I double-checked for you:

roc on  main [!] is 📦 v0.0.1 via 🦀 v1.71.1 via ❄️  impure (nix-shell-env) took 4s 
❯ ./target/release/roc dev /home/username/gitrepos/roc5/roc/crates/glue/tests/fixtures/nullable-wrapped/app.roc
🔨 Rebuilding platform...
tag_union was: StrFingerTree::More("foo", StrFingerTree::More("bar", StrFingerTree::Empty))
`More "small str" (Single "other str")` is: StrFingerTree::More("small str", StrFingerTree::Single("other str"))
`More "small str" Empty` is: StrFingerTree::More("small str", StrFingerTree::Empty)
`Single "small str"` is: StrFingerTree::Single("small str")
`Empty` is: StrFingerTree::Empty

roc on  main [!] is 📦 v0.0.1 via 🦀 v1.71.1 via ❄️  impure (nix-shell-env) 
❯ echo $?
0

roc on  main [!] is 📦 v0.0.1 via 🦀 v1.71.1 via ❄️  impure (nix-shell-env) 
❯ ./crates/glue/tests/fixtures/nullable-wrapped/app
tag_union was: StrFingerTree::More("foo", StrFingerTree::More("bar", StrFingerTree::Empty))
`More "small str" (Single "other str")` is: StrFingerTree::More("small str", StrFingerTree::Single("other str"))
`More "small str" Empty` is: StrFingerTree::More("small str", StrFingerTree::Empty)
`Single "small str"` is: StrFingerTree::Single("small str")
`Empty` is: StrFingerTree::Empty
Segmentation fault (core dumped)

It is segfaulting on main but it's hidden because the output was correct. We used to forget to check the exit code (which indicated the segfault) this PR fixes that.

@Anton-4 Anton-4 merged commit 001ab25 into main Oct 21, 2023
14 checks passed
@Anton-4 Anton-4 deleted the roc-dev-report-status branch October 21, 2023 18:58
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.

roc dev ends with exit code 0 on panic
3 participants