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

[vm]fixes #15974 #12551 #19464 #16020 #16780 #16613 #14553 #19909 #18641 #19902

Merged
merged 6 commits into from
Jun 22, 2022

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented Jun 16, 2022

Reverts #12217 since the root problem seems to have been fixed;
The tests (see tests/vm/tmisc_vm.nim) fixed by #12217 continues to work after reverting the pr.

fixes #15974 regression(0.20.0 => devel): var params assignment gives silently wrong results in VM
fixes #12551 [regression] var param corruption and variable mutation when run the code with VM
fixes #19464 [VM] Immutable proc argument is incorrectly modified
fixes #16020 addr(object) doesn't work in VM
fixes #16780 object copy broken in vm
fixes #16613 Nim crashes randomly
fixes #14553 GC crash in VM with tuple unpacking loop and table
fixes #19909 addressing ref objects and comparing the pointer address in vm give FieldDefect
fixes #18641 addr doesn't work inside static at proc scope

@ringabout ringabout changed the title revert #12217 since the root problem seems to have been fixed; fix #15974;fix #12551; fix #19464 revert #12217 since the root problem seems to have been fixed; fix #15974;fix #12551; fix #19464; fix #16020; fix #16780 Jun 16, 2022
@ringabout ringabout marked this pull request as draft June 16, 2022 13:14
@ringabout ringabout changed the title revert #12217 since the root problem seems to have been fixed; fix #15974;fix #12551; fix #19464; fix #16020; fix #16780 revert #12217 since the root problem seems to have been fixed; fix #15974;fix #12551; fix #19464; fix #16020; fix #16780; #16613 Jun 16, 2022
@ringabout ringabout changed the title revert #12217 since the root problem seems to have been fixed; fix #15974;fix #12551; fix #19464; fix #16020; fix #16780; #16613 revert #12217 since the root problem seems to have been fixed; fix #15974 #12551 #19464 #16020 #16780 #16613 #14553 Jun 16, 2022
@ringabout ringabout changed the title revert #12217 since the root problem seems to have been fixed; fix #15974 #12551 #19464 #16020 #16780 #16613 #14553 revert #12217 since the root problem seems to have been fixed; fixes #15974 #12551 #19464 #16020 #16780 #16613 #14553 Jun 16, 2022
@ringabout ringabout changed the title revert #12217 since the root problem seems to have been fixed; fixes #15974 #12551 #19464 #16020 #16780 #16613 #14553 fixes #15974 #12551 #19464 #16020 #16780 #16613 #14553 Jun 16, 2022
@ringabout ringabout changed the title fixes #15974 #12551 #19464 #16020 #16780 #16613 #14553 [vm]fixes #15974 #12551 #19464 #16020 #16780 #16613 #14553 Jun 16, 2022
@ringabout ringabout marked this pull request as ready for review June 16, 2022 15:53
compiler/vm.nim Outdated
@@ -1028,9 +1027,9 @@ proc rawExecute(c: PCtx, start: int, tos: PStackFrame): TFullReg =
if regs[rc].kind == rkNodeAddr:
ret = regs[rb].nodeAddr == regs[rc].nodeAddr
else:
ret = ptrEquality(regs[rb].nodeAddr, regs[rc].node)
ret = ptrEquality(regs[rb].nodeAddr, regs[rc].regToNode) # todo fixme
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm checking it...

Copy link
Member Author

@ringabout ringabout Jun 19, 2022

Choose a reason for hiding this comment

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

It seems to be a pre-existing issue:

type
  SinglyLinkedList[T] = ref object

proc addMoved[T](a, b: var SinglyLinkedList[T]) =
  if a.addr != b.addr: discard

proc main =
  var a: SinglyLinkedList[int]; new a
  var b: SinglyLinkedList[int]; new b
  a.addMoved b

static: main()

gives the same error messages as objects(my PR - regToNode) => field 'node' is not accessible for type 'TFullReg' using 'kind = rkRegisterAddr' [FieldDefect] with 1.6.6 for ref objects.

My PR re-exposes the scar hidden for objects. I'm gonna to fix this pre-existing issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

=> #19909

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed on the latest commit

@Araq
Copy link
Member

Araq commented Jun 17, 2022

Ping me again when it's ready.

@ringabout ringabout changed the title [vm]fixes #15974 #12551 #19464 #16020 #16780 #16613 #14553 [vm]fixes #15974 #12551 #19464 #16020 #16780 #16613 #14553 #19909 Jun 20, 2022
@ringabout ringabout changed the title [vm]fixes #15974 #12551 #19464 #16020 #16780 #16613 #14553 #19909 [vm]fixes #15974 #12551 #19464 #16020 #16780 #16613 #14553 #19909 #18641 Jun 20, 2022
@ringabout ringabout marked this pull request as draft June 20, 2022 10:25
@ringabout ringabout marked this pull request as ready for review June 22, 2022 04:28
@Araq Araq merged commit 3cb2d7a into devel Jun 22, 2022
@Araq Araq deleted the pr_revert_vm branch June 22, 2022 06:44
narimiran pushed a commit that referenced this pull request Jun 23, 2022
 (#19902) [backport]

* revert #12217 since the root problem seems to have been fixed; fix #15974;fix #12551; fix #19464

* fix #16020; fix #16780

* fix tests and #16613

* fix #14553

* fix #19909; skip skipRegisterAddr

* fix #18641

(cherry picked from commit 3cb2d7a)
@konsumlamm
Copy link
Contributor

@narimiran could this be backported (preferrably 1.6 and 1.4)? This would allow to fix a bug in https://github.com/nim-lang/bigints for more than just devel (see nim-lang/bigints#94).

@narimiran
Copy link
Member

@narimiran could this be backported (preferrably 1.6 and 1.4)?

It is already backported to the 1.6, which is our LTS, and it will be part of our next 1.6 release.

The 1.4 line is not really maintained anymore, i.e. don't expect any new releases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment