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

Nullary tagged union constructors no longer need parens #575

Merged
merged 1 commit into from
Jul 26, 2024

Conversation

hugomg
Copy link
Member

@hugomg hugomg commented May 19, 2023

For example, now we can write types.T.Any instead of types.T.Any(). It should have been like this from the beginning.

@hugomg
Copy link
Member Author

hugomg commented May 19, 2023

I'm trying to bisect this segfault and I believe it is unrelated to this PR. It seems to also happen on older commits. That said, to be safe we should probably track down that other bug first...

@hugomg
Copy link
Member Author

hugomg commented May 19, 2023

Turns out the segfault really was because of this PR. In our gc.lua we use Ir.Cmd objects as table keys, assuming that they are all separate objects with an unique address. Having many command share the same command object caused collision in the table keys, which messed up the liveliness analysis and resulted in an use-after-free segfault.

live_gc_vars[cmd] = {}

The underlying issue here is that our gc.lua is trying to come up with a flat "address" for a nested IR. Either we should embrace nestedness and have a nested live_gc_vars, our we should switch to flat IR. Today I'm slightly tending towards the later: it's "by the book" and would be compatible with goto.

If we don't want to wait too long for that, a possible workaround would be to require parenthesis for ir.Cmd (e.g. ir.Cmd.Break()) but not require parenthesis for other things (e.g. types.T.Integer) .

For example, now we can write types.T.Any instead of types.T.Any().
It should have been like this from the beginning, but we couldn't
because some of our code was using ir.Cmd objects as table keys.
(This was just fixed in a recent PR)
@gabrielsferre gabrielsferre merged commit 46881a9 into master Jul 26, 2024
2 checks passed
@gabrielsferre gabrielsferre deleted the empty-ctr branch July 26, 2024 01:07
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