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

revert #20719; relieve std/assertions of the sysFatal dep #20743

Merged
merged 2 commits into from
Nov 4, 2022
Merged

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented Nov 3, 2022

revert #20719

ref https://github.com/nim-lang/Nim/pull/20718/files#r1010607801

I'm not sure whether import system/sysFatal works and the leaking is just a flaky when investigating #20718 . Better safe than sorry. I revert that PR and relieve std/assertions of the sysFatal dep following Nim/lib/std/syncio.nim

template sysFatal(exc, msg) =
  raise newException(exc, msg)

For the record, the leak was and I can reproduce it on wsl2.

==99591== HEAP SUMMARY:
==99591==     in use at exit: 10 bytes in 1 blocks
==99591==   total heap usage: 32 allocs, 31 frees, 5,113 bytes allocated
==99591== 
==99591== 10 bytes in 1 blocks are definitely lost in loss record 1 of 1
==99591==    at 0x483DD99: calloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==99591==    by 0x10CE14: alloc0Impl__system_1765 (in /home/vsts/work/1/s/tests/arc/tcaseobj)
==99591==    by 0x10CE42: allocShared0Impl__system_1778 (in /home/vsts/work/1/s/tests/arc/tcaseobj)
==99591==    by 0x10F091: prepareAdd (in /home/vsts/work/1/s/tests/arc/tcaseobj)
==99591==    by 0x10F3F3: setLengthStrV2 (in /home/vsts/work/1/s/tests/arc/tcaseobj)
==99591==    by 0x10ACD9: addChars__stdZprivateZdigitsutils_100 (in /home/vsts/work/1/s/tests/arc/tcaseobj)
==99591==    by 0x10B31B: addIntImpl__stdZprivateZdigitsutils_59 (in /home/vsts/work/1/s/tests/arc/tcaseobj)
==99591==    by 0x10B3C7: addInt__stdZprivateZdigitsutils_150 (in /home/vsts/work/1/s/tests/arc/tcaseobj)
==99591==    by 0x10B637: addInt__stdZprivateZdigitsutils_153 (in /home/vsts/work/1/s/tests/arc/tcaseobj)
==99591==    by 0x10C608: addInt__stdZprivateZdigitsutils_175 (in /home/vsts/work/1/s/tests/arc/tcaseobj)
==99591==    by 0x10C6A9: dollar___systemZdollars_8 (in /home/vsts/work/1/s/tests/arc/tcaseobj)
==99591==    by 0x111608: raiseFieldError2 (in /home/vsts/work/1/s/tests/arc/tcaseobj)
==99591== 
==99591== LEAK SUMMARY:
==99591==    definitely lost: 10 bytes in 1 blocks
==99591==    indirectly lost: 0 bytes in 0 blocks
==99591==      possibly lost: 0 bytes in 0 blocks
==99591==    still reachable: 0 bytes in 0 blocks
==99591==         suppressed: 0 bytes in 0 blocks
==99591== 
==99591== For lists of detected and suppressed errors, rerun with: -s
==99591== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

@ringabout ringabout changed the title relieve std/assertions of the sysFatal dep relieve std/assertions of the sysFatal dep Nov 3, 2022
@@ -9,8 +9,6 @@

## This module implements assertion handling.

import system/fatal
Copy link
Member Author

@ringabout ringabout Nov 3, 2022

Choose a reason for hiding this comment

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

It was actually

when not declared(sysFatal):
  include "system/fatal"

before. I think it is better to do what std/syncio did, which was done years ago.

@ringabout ringabout changed the title relieve std/assertions of the sysFatal dep revert #20719; relieve std/assertions of the sysFatal dep Nov 3, 2022
@ringabout ringabout marked this pull request as draft November 4, 2022 03:05
@ringabout ringabout marked this pull request as ready for review November 4, 2022 03:13
@Araq Araq merged commit 12a20b9 into devel Nov 4, 2022
@Araq Araq deleted the pr_assertions branch November 4, 2022 08:53
@github-actions
Copy link
Contributor

github-actions bot commented Nov 4, 2022

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

Hint: mm: orc; opt: speed; options: -d:release
164620 lines; 6.811s; 613.938MiB peakmem

capocasa pushed a commit to capocasa/Nim that referenced this pull request Mar 31, 2023
…nim-lang#20743)

* Revert "make `system/fatal` importable (nim-lang#20718)"

This reverts commit d735c44.

* relieve `std/assertions` of the sysFatal dep
bung87 pushed a commit to bung87/Nim that referenced this pull request Jul 29, 2023
…nim-lang#20743)

* Revert "make `system/fatal` importable (nim-lang#20718)"

This reverts commit d735c44.

* relieve `std/assertions` of the sysFatal dep
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.

2 participants