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.

Sign up for GitHub

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

Jump to bottom

fix #15704 #15597 wrong VM register was freed #15705

Merged
merged 5 commits into from
Oct 26, 2020
Merged

fix #15704 #15597 wrong VM register was freed #15705

merged 5 commits into from
Oct 26, 2020

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Oct 24, 2020 •
edited
Loading

fix #15704
fix #15597

(regressions caused by #12485)

This was painful to track down, we should improve debug-ability of nim vm; I used -d:nimVMDebug for that but -d:nimVMDebug should be further improved

  • make it easy to track lifetime of a given register
  • improve the listing from -d:nimVMDebug by showing more contextual information

CI failure unrelated

=> flaky timotheecour#352

future work

  • allow configuring HighRegisterPressure = 40; will allow for eg lowering it to find more bugs easily, or increasing it for temporary workarounds when there's a similar bug; at least useful for debugging (it was for me in this PR)

@timotheecour timotheecour force-pushed the pr_fix_15704_register_free branch from 5ab8b8c to 28c198a Compare October 24, 2020 15:38
@timotheecour timotheecour marked this pull request as ready for review October 24, 2020 15:39
@timotheecour timotheecour changed the title fix #15704 wrong VM register was freed fix #15704 #15597 wrong VM register was freed Oct 24, 2020
@timotheecour timotheecour requested a review from Araq October 24, 2020 15:44
@timotheecour timotheecour mentioned this pull request Oct 24, 2020
Closed
@Araq
Copy link
Member

Araq commented Oct 24, 2020

This needs to be backported all the way down to 1.0.x, @narimiran

@timotheecour
Copy link
Member Author

PTAL:

can you explain the added c.freeTemp(dest) in genBinaryStmtVar, genBinaryStmt, genAsgn, nkDerefExpr etc ?

@timotheecour timotheecour mentioned this pull request Oct 25, 2020
Open
@Araq
Copy link
Member

Araq commented Oct 25, 2020

can you explain the added c.freeTemp(dest) in genBinaryStmtVar, genBinaryStmt, genAsgn, nkDerefExpr etc ?

Not anymore but I did review the entire VM codegen looking for register allocator leaks. Apparently my work wasn't good enough. ;-)

@timotheecour timotheecour force-pushed the pr_fix_15704_register_free branch from cba8c75 to e78a62c Compare October 25, 2020 11:06
@timotheecour timotheecour mentioned this pull request Oct 25, 2020
Open
@timotheecour timotheecour force-pushed the pr_fix_15704_register_free branch from e78a62c to 89d4143 Compare October 26, 2020 01:13
Araq
Araq reviewed Oct 26, 2020
View reviewed changes
lib/system.nim Outdated
@@ -2106,7 +2106,7 @@ const
## is the minor number of Nim's version.
## Odd for devel, even for releases.

NimPatch* {.intdefine.}: int = 1
NimPatch* {.intdefine.}: int = 3
Copy link
Member

Choose a reason for hiding this comment

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

What the heck? That should not be part of this PR!

Copy link
Member Author

Choose a reason for hiding this comment

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

that was so code could branch on this bugfix, as explained in #14124 (comment) and it was useful in some PR's to branch on that.

It also makes nim -v a little more helpful to show, instead of just a hash + fixed 1.5.1 it shows hash + 1.5.x so it's immediately clearer in bug reports how recent was the nim version used.

Finally, it makes pre-releases / nightly builds easier to distinguish via nim -v

I'm not aware of negative consequences in bumping NimPatch, it's a common practice to bump patch on bug fixes. I'm not suggesting to do it on each commit (nim isn't following standard semver anyways), but bumping patch somewhat regularly between 2 stable releases is IMO a good thing.

Araq
Araq reviewed Oct 26, 2020
View reviewed changes
lib/system.nim Outdated Show resolved Hide resolved
@Araq Araq merged commit d4c2e2c into nim-lang:devel Oct 26, 2020
@Araq
Copy link
Member

Araq commented Oct 26, 2020

Sorry, I took over a bit as we want to release 1.2.8 this morning. :-)

timotheecour added a commit to timotheecour/fusion that referenced this pull request Oct 26, 2020
@timotheecour
update now that nim-lang/Nim#15705 was merged
e5c10ab
narimiran pushed a commit that referenced this pull request Oct 26, 2020
@timotheecour @narimiran
fix #15704 #15597 wrong VM register was freed (#15705)
d777a8f 
(cherry picked from commit d4c2e2c)
narimiran pushed a commit that referenced this pull request Oct 26, 2020
@timotheecour @narimiran
fix #15704 #15597 wrong VM register was freed (#15705)
52ad63b 
(cherry picked from commit d4c2e2c)
@timotheecour
Copy link
Member Author

Sorry, I took over a bit as we want to release 1.2.8 this morning. :-)

no problem. too bad #15724 couldn't make the cut for 1.2.8

@timotheecour timotheecour deleted the pr_fix_15704_register_free branch October 26, 2020 10:05
@timotheecour timotheecour mentioned this pull request Oct 26, 2020
Open
narimiran pushed a commit that referenced this pull request Oct 27, 2020
@timotheecour @narimiran
fix #15704 #15597 wrong VM register was freed (#15705)
4e91c11 
* fix #15704 #15597 wrong VM register was freed

* same treatment for nkCheckedFieldExpr

* note concerning HighRegisterPressure

* bump NimPatch

* Update lib/system.nim

Co-authored-by: Andreas Rumpf <[email protected]>
(cherry picked from commit d4c2e2c)
@timotheecour timotheecour mentioned this pull request Jan 3, 2021
Merged
mildred pushed a commit to mildred/Nim that referenced this pull request Jan 11, 2021
@timotheecour @Araq @mildred
fix nim-lang#15704 nim-lang#15597 wrong VM register was freed (nim-la…
f1fbe76 
…ng#15705)

* fix nim-lang#15704 nim-lang#15597 wrong VM register was freed

* same treatment for nkCheckedFieldExpr

* note concerning HighRegisterPressure

* bump NimPatch

* Update lib/system.nim

Co-authored-by: Andreas Rumpf <[email protected]>
timotheecour added a commit to timotheecour/fusion that referenced this pull request Jan 15, 2021
@timotheecour
update now that nim-lang/Nim#15705 was merged
8520e1f
irdassis pushed a commit to irdassis/Nim that referenced this pull request Mar 16, 2021
@timotheecour @Araq @irdassis
fix nim-lang#15704 nim-lang#15597 wrong VM register was freed (nim-la…
19e1dc5 
…ng#15705)

* fix nim-lang#15704 nim-lang#15597 wrong VM register was freed

* same treatment for nkCheckedFieldExpr

* note concerning HighRegisterPressure

* bump NimPatch

* Update lib/system.nim

Co-authored-by: Andreas Rumpf <[email protected]>
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
@timotheecour @Araq @ardek66
fix nim-lang#15704 nim-lang#15597 wrong VM register was freed (nim-la…
2f6628d 
…ng#15705)

* fix nim-lang#15704 nim-lang#15597 wrong VM register was freed

* same treatment for nkCheckedFieldExpr

* note concerning HighRegisterPressure

* bump NimPatch

* Update lib/system.nim

Co-authored-by: Andreas Rumpf <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Araq Araq Araq left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@timotheecour @Araq