-
-
Notifications
You must be signed in to change notification settings - Fork 410
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
Segmentation fault when capturing Env via lambda #4343
Comments
This works: actor Main
var e: (Env | None) = None
let read: {()} = {() => e }
new create(e': Env) => e = e' So, in the original code, it appears from what I am seeing without digging "too deeply" that I believe that the compiler should be rejecting the original code and this shouldn't end up as a runnable program. |
This also doesn't give a compiler error and should so any regression tests should cover this as well: actor Main
let e: Env
let read: {()}
new create(e': Env) =>
read = {() => e }
e = e' |
So I think in lambda.c, around line 66, we want to verify if we are capturing any of:
that the object being captured as a status of |
Here's another version to address: actor Main
let e: Env
let read: {()}
new create(e': Env) =>
read = object
fun apply() =>
e
end
e = e' |
This is the same issue as #4301 |
There's an interesting twist to this bug as it is handled well but surprisingly with locals. For example: actor Main
new create(env: Env) =>
var x: String
let y = object
fun apply() =>
env.out.print(x)
end
y()
if false then
x = "Foo"
end results in " can't use an undefined variable in an expression" the same for if but... actor Main
new create(env: Env) =>
var x: String
let y = object
fun apply() =>
env.out.print(x)
end
y()
x = "Foo" will print "Foo". I think it would be good to make member and local implicit capture be "the same" and not wildly different. |
Related to this and #4301, the lambda and object literal handling is in the expression pass. We set fields as being defined in the symbol table in the refer pass so by the time we get to the expression pass, as far as the compiler is concerned, this is a defined field and there's no way to see it as not initialized yet. So the code in |
I haven't checked but I am assuming that this is an issue for fields because we don't reorder their initialization (or prevent it from happening) but it is ok for locals because we don't disallow reordering. See: actor Main
new create(env: Env) =>
var x: String
let y = object
fun apply() =>
env.out.print(x)
end
y()
x = "Foo" as an example with a local variable of one that works. |
Here's the important LLVM IR we can see from an example from #4301: works (yup): ; Function Attrs: nounwind
define private fastcc void @Bar_ref_create_o(�r* nocapture writeonly dereferenceable(24) %this) unnamed_addr #2 !pony.abi !2 {
entry:
%0 = tail call i8* @pony_ctx()
%1 = tail call i8* @pony_alloc_small(i8* %0, i32 0)
%2 = bitcast i8* %1 to %Foo_Desc**
store %Foo_Desc* @Foo_Desc, %Foo_Desc** %2, align 8
%3 = getelementptr inbounds i8, i8* %1, i64 8
%4 = bitcast i8* %3 to i64*
store i64 123, i64* %4, align 8
%5 = getelementptr inbounds �r, �r* %this, i64 0, i32 1
%6 = bitcast %Foo** %5 to i8**
store i8* %1, i8** %6, align 8
%7 = tail call i8* @pony_alloc_small(i8* %0, i32 0)
%8 = bitcast i8* %7 to %"$1$1_Desc"**
store %"$1$1_Desc"* @"$1$1_Desc", %"$1$1_Desc"** %8, align 8
%9 = getelementptr inbounds i8, i8* %7, i64 8
= bitcast i8* %9 to i8**
store i8* %1, i8** , align 8
= getelementptr inbounds �r, �r* %this, i64 0, i32 2
= bitcast %__object** to i8**
store i8* %7, i8** , align 8
ret void
} and dur ya, doesn't... define private fastcc void @Bar_ref_create_o(�r* nocapture dereferenceable(24) %this) unnamed_addr #2 !pony.abi !2 {
entry:
%0 = tail call i8* @pony_ctx()
%1 = getelementptr inbounds �r, �r* %this, i64 0, i32 1
%2 = load %Foo*, %Foo** %1, align 8
%3 = tail call i8* @pony_alloc_small(i8* %0, i32 0)
%4 = bitcast i8* %3 to %"$1$1_Desc"**
store %"$1$1_Desc"* @"$1$1_Desc", %"$1$1_Desc"** %4, align 8
%5 = getelementptr inbounds i8, i8* %3, i64 8
%6 = bitcast i8* %5 to %Foo**
store %Foo* %2, %Foo** %6, align 8
%7 = getelementptr inbounds �r, �r* %this, i64 0, i32 2
%8 = bitcast %__object** %7 to i8**
store i8* %3, i8** %8, align 8
%9 = tail call i8* @pony_alloc_small(i8* %0, i32 0)
= bitcast i8* %9 to %Foo_Desc**
store %Foo_Desc* @Foo_Desc, %Foo_Desc** , align 8
= getelementptr inbounds i8, i8* %9, i64 8
= bitcast i8* to i64*
store i64 123, i64* , align 8
= bitcast %Foo** %1 to i8**
store i8* %9, i8** , align 8
ret void
} By the time the refer pass is over, we don't have any good way from symbol table to know that we are going to get "bad code". We could try to get clever and reorder, but, not so good to be clever. |
@jemc this is interesting... llvm (debug as otherwise a ton gets optimized away) for the "it works with a local variable"... define fastcc void @Main_tag_create_oo(%Main* dereferenceable(272) %this, %Env* readonly dereferenceable(64) %env) unnamed_addr !dbg !120 !pony.abi !4 {
entry:
%y = alloca %"$1$0"*, align 8
%0 = call i8* @pony_ctx()
%x = alloca %String*, align 8
%this1 = alloca %Main*, align 8
store %Main* %this, %Main** %this1, align 8
call void @llvm.dbg.declare(metadata %Main** %this1, metadata !154, metadata !DIExpression()), !dbg !156
%env2 = alloca %Env*, align 8
store %Env* %env, %Env** %env2, align 8
call void @llvm.dbg.declare(metadata %Env** %env2, metadata !157, metadata !DIExpression()), !dbg !158
call void @llvm.dbg.declare(metadata %String** %x, metadata !159, metadata !DIExpression()), !dbg !167
%1 = load %String*, %String** %x, align 8
%2 = call i8* @pony_alloc_small(i8* %0, i32 0)
%3 = bitcast i8* %2 to %"$1$0"*
%4 = getelementptr inbounds %"$1$0", %"$1$0"* %3, i32 0, i32 0
store %"$1$0_Desc"* @"$1$0_Desc", %"$1$0_Desc"** %4, align 8
call fastcc void @"$1$0_x_create_ooo"(%"$1$0"* %3, %Env* %env, %String* %1), !dbg !168, !pony.newcall !4
call void @llvm.dbg.declare(metadata %"$1$0"** %y, metadata !169, metadata !DIExpression()), !dbg !175
%5 = bitcast %"$1$0"** %y to i8*
call void @llvm.lifetime.start.p0i8(i64 8, i8* %5)
%6 = load %"$1$0"*, %"$1$0"** %y, align 8
store %"$1$0"* %3, %"$1$0"** %y, align 8
%7 = load %"$1$0"*, %"$1$0"** %y, align 8
%8 = call fastcc %None* @"$1$0_ref_apply_o"(%"$1$0"* %7), !dbg !176
%9 = bitcast %String** %x to i8*, !dbg !177
call void @llvm.lifetime.start.p0i8(i64 8, i8* %9), !dbg !177
= load %String*, %String** %x, align 8, !dbg !177
store %String* @3, %String** %x, align 8, !dbg !177
= bitcast %String** %x to i8*
call void @llvm.lifetime.end.p0i8(i64 8, i8* )
= bitcast %"$1$0"** %y to i8*
call void @llvm.lifetime.end.p0i8(i64 8, i8* )
ret void
} and corresponding pony: actor Main
new create(env: Env) =>
var x: String
let y = object
fun apply() =>
env.out.print(x)
end
y()
x = "Foo" |
I think I understand what is going on and if I'm correct it is insidious. I need to discuss with @jemc. |
I think the issue is that we do the refer pass and set all the fields as defined because they are by the end of the refer pass and then in the expression pass, we update the AST and capture (in expr_object in lambda.c) field(s) that need rechecked from scratch to see if they are actually defined at the time they are used when we rerun back to the expression pass going through the refer pass again. (see the end of lambda.c) if(!ast_passes_subtree(astp, opt, PASS_EXPR)) |
A better, less clever option might be to move expr_object and expr_lambda to a new pass that comes before refer. much like the traits pass comes before refer and can make sure everything is all set before refer. that's probably a much better approach. |
@jemc i have what i think is a straightforward fix to this, but you would be the expert here. we merge the refer pass back into the expression pass. that solves our entire problem for this. i think we should discuss the tradeoffs here now that we know a serious drawback of the creation of the refer pass. or, move the "valid reference" and related code back into the expression pass that would probably be less code. |
or @jemc, they move into the completeness pass from refer. i think either is a reasonable approach. i think you suggested moving from refer to completeness at sync yesterday. |
@jemc and I discussed multiple options today and we have an approach that I am going to try. |
Minimal example: directory with file
main.pony
:Environment:
ponyc
installed viaponyup
Actual error:
Zulip topic: https://ponylang.zulipchat.com/#narrow/stream/189934-general/topic/Unexpected.20segfault
The text was updated successfully, but these errors were encountered: