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

Support closured vars inside Const initializer #10478

Merged
merged 4 commits into from
Mar 16, 2021

Conversation

RX14
Copy link
Contributor

@RX14 RX14 commented Mar 6, 2021

Probably isn't the cleanest fix, but this solves it with the least changes.

I found another bug with const initializer codegen while I was solving this, that's why there's 2 commits.

Fixes #10474 .

@RX14
Copy link
Contributor Author

RX14 commented Mar 6, 2021

Seems the alpine build failing is pre-existing, the windows build failing is ???

I think I should not test CONST being 0, since it's nil and the value is undefined?

@asterite
Copy link
Member

asterite commented Mar 6, 2021

This is great! I think if we name it def instead of fake def then it's much better. That is, this is the correct solution, not a temporary one.

@RX14
Copy link
Contributor Author

RX14 commented Mar 6, 2021

I'd prefer to name it this way, since it is a fake def, the function it refers to never gets defined, and does not exist in the program. The Def exists only to provide a context to type the initializer in.

@asterite
Copy link
Member

asterite commented Mar 6, 2021

I see. Yeah, that makes sense.

@RX14
Copy link
Contributor Author

RX14 commented Mar 6, 2021

A more complete refactor I think would make the proc initializers actual Defs in the AST, which are treated mostly normally (i.e. actually visited by the visitors), just with special name mangling and are called as the const initializer. But this works, for now.

@straight-shoota
Copy link
Member

I'd say the windows failure is an actual bug. The CONST value should really be nil, it's not undefined.
And converting nil to an integer shouldn't give 699531520.

@RX14
Copy link
Contributor Author

RX14 commented Mar 6, 2021

@straight-shoota I think the values here are not actual nil.to_i, this would perform normally in a real system, it's just weirdness coming from uninitialized memory and the JIT.

At least, without doing some unsafe memory wrangling.

@RX14
Copy link
Contributor Author

RX14 commented Mar 6, 2021

In llvm, %Nil = type {}, and it has zero length. I think this is fine, nil has no binary representation. Especially since unions of nil are optimized.

@RX14
Copy link
Contributor Author

RX14 commented Mar 6, 2021

Yeah, sizeof(Nil) == 0, so pointerof(nil_var).unsafe_as(Pointer(UInt8)).value is reading the byte in memory after the nil value, theoretically.

@RX14
Copy link
Contributor Author

RX14 commented Mar 6, 2021

Just to further make sure that that spec is correct: https://carc.in/#/r/aibt

@straight-shoota
Copy link
Member

Okay 👍 Then maybe the spec could check CONST.nil??

@RX14
Copy link
Contributor Author

RX14 commented Mar 6, 2021

done!

@RX14
Copy link
Contributor Author

RX14 commented Mar 15, 2021

bump?

@bcardiff bcardiff requested a review from asterite March 16, 2021 14:26
Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

I still think fake_def should be just renamed to def. It doesn't matter if it's "fake" or that we fake it: in the end we end up kind of creating a def for it. But this rename get to into a later PR if needed.

@bcardiff bcardiff added this to the 1.0.0 milestone Mar 16, 2021
@bcardiff bcardiff merged commit e572b56 into crystal-lang:master Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nil assertion failed in codegen_fun when using closured object inside captured block inside CONST initializer
4 participants