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

[backport] fixes #23748; do not skip materializing temporaries for proc arguments #23769

Merged
merged 17 commits into from
Jun 30, 2024

Conversation

alex65536
Copy link
Contributor

@alex65536 alex65536 commented Jun 28, 2024

fixes #23748

@Araq
Copy link
Member

Araq commented Jun 28, 2024

Superb work, but this fails: tests/destructor/t23748.nim cpp --gc:arc

@alex65536
Copy link
Contributor Author

Superb work, but this fails: tests/destructor/t23748.nim cpp --gc:arc

Thanks, fixed :)

Though, fixing the issue for C++ was a bit more tough than for C.

@alex65536
Copy link
Contributor Author

Oops, seems that I've broken something else trying to fix it :(

@alex65536
Copy link
Contributor Author

alex65536 commented Jun 29, 2024

It seems to pass the tests better, but I think that the current solution with setting tfVarIsPtr back and forth is quite hacky, so I want to look up for another solution.

@Araq
Copy link
Member

Araq commented Jun 29, 2024

Unfortunately tests are still red.

@alex65536
Copy link
Contributor Author

alex65536 commented Jun 29, 2024

It seems that tests fail just because http://example.com/404 returns error 500 instead of 404:

$ curl -v http://example.com/404 |& grep 'HTTP/1.1'
> GET /404 HTTP/1.1
< HTTP/1.1 500 Internal Server Error

The same test also fails in devel. I am going to comment it out.

@alex65536
Copy link
Contributor Author

Made a separate PR to fix the flaky test: #23772.

@alex65536
Copy link
Contributor Author

It seems that the rest of the failing tests are flaky (with Permission denied from linker under Windows) and not related to the actual change.

@Araq could you please take a look?

@alex65536
Copy link
Contributor Author

(probably we should also merge #23772 before this)

@alex65536
Copy link
Contributor Author

All the tests pass now, could you please merge this?

@Araq Araq merged commit 4202b60 into nim-lang:devel Jun 30, 2024
18 checks passed
@Araq
Copy link
Member

Araq commented Jun 30, 2024

Sure, sorry that it took me one hour on a Sunday to respond. ;-)

Copy link
Contributor

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

Hint: mm: orc; opt: speed; options: -d:release
179041 lines; 8.393s; 664.703MiB peakmem

narimiran pushed a commit that referenced this pull request Jun 30, 2024
@alex65536 alex65536 deleted the bug23748 branch June 30, 2024 17:29
@narimiran
Copy link
Member

narimiran commented Jul 1, 2024

@alex65536 I've seen that you marked this PR with [backport]. When I tried to backport it to the version-2-0, it produced errors with cpp backend: example of a failing CI

If you're willing, you could investigate why it is happening, i.e. if some previous devel commit also needs to be backported or maybe you can fix it in another way.

If you want to test it locally (with version-2-0 + your fix from this PR), you can run: koch temp cpp --mm:refc tests/stdlib/trepr.nim to reproduce the error reported by CIs.

Graveflo pushed a commit to Graveflo/Nim that referenced this pull request Jul 1, 2024
@alex65536
Copy link
Contributor Author

@narimiran it seems that the test referenced by you passes successfully if you also backport the following commit: f7c6e04.

Let me know if there are other issues with backporting this fix.

@narimiran
Copy link
Member

@narimiran it seems that the test referenced by you passes successfully if you also backport the following commit: f7c6e04.

Let me know if there are other issues with backporting this fix.

Great find, thanks!

Now I've backported both this PR and that commit.

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.

Local variables can be prematurely moved to closure, causing use-after-move
3 participants