Segmentation fault when capturing Env via lambda

[alurm]

Minimal example: directory with file main.pony:

actor Main
	let e: Env
	let read: {()} = {() => e}
	new create(e': Env) => e = e'

Environment:

• Linux running under Windows (Windows subsystem for Linux 2) on AMD64
• ponyc installed via ponyup

$ ponyc -v
0.54.1-297efcef [release]
Compiled with: LLVM 15.0.7 -- Clang-14.0.0-x86_64
Defaults: pic=true

Actual error:

$ ponyc -V 0 && ./segmentation_violation
fish: Job 1, './segmentation_violation' terminated by signal SIGSEGV (Address boundary error)

Zulip topic: https://ponylang.zulipchat.com/#narrow/stream/189934-general/topic/Unexpected.20segfault

[SeanTAllen]

This segfaults tracing the read variable because there's no pony type attached to what is being traced.

[SeanTAllen]

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 read is capturing the uninitialized e which leads to our "this object has no type" kaboom in the gc code.

I believe that the compiler should be rejecting the original code and this shouldn't end up as a runnable program.

[SeanTAllen]

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'

[SeanTAllen]

So I think in lambda.c, around line 66, we want to verify if we are capturing any of:

• TK_FVAR
• TK_FLET
• TK_EMBED

that the object being captured as a status of SYM_DEFINED.

[SeanTAllen]

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'

[SeanTAllen]

This is the same issue as #4301

[SeanTAllen]

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 true rather than false

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.

[SeanTAllen]

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 lambda.c is too late to do any sort of checking.

[SeanTAllen]

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.

[SeanTAllen]

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(%Bar* 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 %Bar, %Bar* %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
  %10 = bitcast i8* %9 to i8**
  store i8* %1, i8** %10, align 8
  %11 = getelementptr inbounds %Bar, %Bar* %this, i64 0, i32 2
  %12 = bitcast %__object** %11 to i8**
  store i8* %7, i8** %12, align 8
  ret void
}

and dur ya, doesn't...

define private fastcc void @Bar_ref_create_o(%Bar* nocapture dereferenceable(24) %this) unnamed_addr #2 !pony.abi !2 {
entry:
  %0 = tail call i8* @pony_ctx()
  %1 = getelementptr inbounds %Bar, %Bar* %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 %Bar, %Bar* %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)
  %10 = bitcast i8* %9 to %Foo_Desc**
  store %Foo_Desc* @Foo_Desc, %Foo_Desc** %10, align 8
  %11 = getelementptr inbounds i8, i8* %9, i64 8
  %12 = bitcast i8* %11 to i64*
  store i64 123, i64* %12, align 8
  %13 = bitcast %Foo** %1 to i8**
  store i8* %9, i8** %13, 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.

[SeanTAllen]

@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
  %10 = load %String*, %String** %x, align 8, !dbg !177
  store %String* @3, %String** %x, align 8, !dbg !177
  %11 = bitcast %String** %x to i8*
  call void @llvm.lifetime.end.p0i8(i64 8, i8* %11)
  %12 = bitcast %"$1$0"** %y to i8*
  call void @llvm.lifetime.end.p0i8(i64 8, i8* %12)
  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"

[SeanTAllen]

I think I understand what is going on and if I'm correct it is insidious. I need to discuss with @jemc.

[SeanTAllen]

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))

[SeanTAllen]

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.

[SeanTAllen]

@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.

[SeanTAllen]

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.

[SeanTAllen]

@jemc and I discussed multiple options today and we have an approach that I am going to try.