ponyc segfaults when trying to match using a single element tuple form

[chriskdon]

ponyc segfaults when trying to use match with a single element tuple destructure form. As expected, it works when you don't use the tuple syntax in the match and with multi-element tuples.

Use Case:

I understand that using a single-element tuple is strange since it's effectively the same as a standalone value. Still, in my use case, I had some code like: type Term is ((Tuple, (Value)) | (Tuple, (Value, Value)) | (Tuple, (Value, Value, Value)) | ...), where I wanted to support a few small tuple sizes for convenience in serialization code. (Value) being a tuple is ambiguous, I can see the compiler doesn't treat it as such since it doesn't generate an _1 member.

I can work around this using the non-tuple syntax in the match, but it's nice to have the match look the same as the type.

Minimal Example

// I32 is only used as an example; other types also fail
type CrashIt is (I32 | (I32, (I32)))

actor Main
  new create(env: Env) =>
    let x: CrashIt = 123
    match x
    | (456, (let t1: I32)) => None   // Segfaults
    // | (789, let t1: I32) => None  // Works
    end

Expected Result:

The compiler doesn't crash.

I would expect this syntax to be allowed in a match because (let y: I32) = (I32(100)) (assignment destructure) is valid and works.

Actual Result:

➜ ponyc
Building builtin -> /opt/homebrew/Cellar/ponyc/0.55.1/packages/builtin
Building . -> /Users/chriskdon/dev/ponyc_crash
Generating
 Reachability
 Selector painting
 Data prototypes
 Data types
 Function prototypes
 Functions
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
[1]    70114 segmentation fault  ponyc

Environment

• Apple M2 (AArch64)
• MacOS Ventura 13.4

Compiler

➜ ponyc -v
0.55.1-800527f7 [release]
Compiled with: LLVM 15.0.7 -- AppleClang-14.0.3.14030022-arm64
Defaults: pic=true

Also failed in 0.55.0

---

I'm guessing this has something to do with 1-element tuples not "really" being tuples, and the match isn't taking this into account like the regular assignment destructure. This is a guess; I still need to dig into the compiler code.

If there is some direction on the actual expected behaviour of the match, I'm happy to try and fix this myself. If you have any suggestions on where to begin in the code, it's always appreciated.

As an aside:

Because (MyType) doesn't generate a 1-tuple as might be expected, it may be worth having the compiler emit an error or warning if the (MyType) syntax is used when defining a type and the extra parens are not required (like when defining a union). This could help to avoid confusion. I didn't think about the (MyType) syntax not generating a tuple until I ran into other syntax errors like couldn't find _1. I assume this case can be detected unambiguously, but I could be totally wrong here, so please forgive my ignorance.

Alternatively, I would like it even more if this created a real 1-tuple for consistency. But I'm guessing there are reasons it doesn't.

[SeanTAllen]

At minimum, we want to display an error message, not have the compiler segfault. Running this with a debug version of the compiler to get a backtrace would be a good first step.

We discussed during sync and a change to 1 tuple type is a big change and not one that we are interested in taking on but an RFC could be opened for it to discuss in more depth and minds could be changed. Even if we were inclined towards the change, this would still require an RFC.

[chriskdon]

Is the match syntax that's causing the segfault supposed to be valid? Similar to how we can do | (let x: Int) => x in a match or (let y: I32) = I32(100) during an assignment. I'm asking because I'd like to make a fix that supports the syntax instead of outputting an error if possible. But I'm happy to go in any direction you advise.

In any case, I'll start looking into this.

I imagined the 1-tuple change would be a much larger ask. There isn't a deep need for it currently. I may revisit the idea of an RFC once I have more Pony experience. I appreciate that it was discussed. Thanks!

[SeanTAllen]

Here's a stacktrace:

Process 876 stopped
* thread #1, name = 'ponyc', stop reason = signal SIGSEGV: invalid address (fault address: 0x12)
    frame #0: 0x0000555555d63ea2 ponyc`LLVMIsConstant + 2
ponyc`LLVMIsConstant:
->  0x555555d63ea2 <+2>: cmpb   $0x14, 0x10(%rdi)
    0x555555d63ea6 <+6>: setb   %al
    0x555555d63ea9 <+9>: retq
    0x555555d63eaa:      nopw   (%rax,%rax)
(lldb) bt
* thread #1, name = 'ponyc', stop reason = signal SIGSEGV: invalid address (fault address: 0x12)
  * frame #0: 0x0000555555d63ea2 ponyc`LLVMIsConstant + 2
    frame #1: 0x0000555555c465ac ponyc`make_cmp_value(c=0x00007fffffffe088, sign=true, l_value=0x0000000000000002, r_value=0x00005555594c8330, cmp_f=LLVMRealOEQ, cmp_si=LLVMIntEQ, cmp_ui=LLVMIntEQ, safe=true) at genoperator.c:219:6
    frame #2: 0x0000555555c46542 ponyc`gen_eq_rvalue(c=0x00007fffffffe088, left=0x00007ffff72d2b80, r_value=0x00005555594c8330, safe=true) at genoperator.c:893:10
    frame #3: 0x0000555555c37eaa ponyc`gen_pattern_eq(c=0x00007fffffffe088, pattern=0x00007ffff72d2b80, r_value=0x00005555594c8330) at gencall.c:972:16
    frame #4: 0x0000555555c4380a ponyc`check_value(c=0x00007fffffffe088, pattern=0x00007ffff72d2b80, param_type=0x00007ffff13acf00, value=0x00005555594e79f0, next_block=0x00005555594e5470) at genmatch.c:234:25
    frame #5: 0x0000555555c43644 ponyc`dynamic_value_object(c=0x00007fffffffe088, object=0x00005555594e79f0, desc=0x00005555594e7b20, pattern=0x00007ffff72d2b80, next_block=0x00005555594e5470) at genmatch.c:460:10
    frame #6: 0x0000555555c43414 ponyc`dynamic_match_object(c=0x00007fffffffe088, object=0x00005555594e79f0, desc=0x00005555594e7b20, pattern=0x00007ffff72d2b80, next_block=0x00005555594e5470) at genmatch.c:529:14
    frame #7: 0x0000555555c431e5 ponyc`dynamic_tuple_element(c=0x00007fffffffe088, ptr=0x00005555594e5840, desc=0x00005555594e5610, pattern=0x00007ffff72d2b80, next_block=0x00005555594e5470, elem=1) at genmatch.c:288:7
    frame #8: 0x0000555555c42e6f ponyc`dynamic_tuple_ptr(c=0x00007fffffffe088, ptr=0x00005555594e5840, desc=0x00005555594e5610, pattern=0x00007ffff72d2e00, next_block=0x00005555594e5470) at genmatch.c:325:9
    frame #9: 0x0000555555c42364 ponyc`static_tuple(c=0x00007fffffffe088, value=0x00005555594e5330, type=0x00007ffff13ae280, pattern=0x00007ffff72d2e00, next_block=0x00005555594e5470) at genmatch.c:581:14
    frame #10: 0x0000555555c41ed8 ponyc`static_match(c=0x00007fffffffe088, value=0x00005555594e5330, type=0x00007ffff13ae280, pattern=0x00007ffff72d2e00, next_block=0x00005555594e5470) at genmatch.c:681:14
    frame #11: 0x0000555555c41aaa ponyc`gen_match(c=0x00007fffffffe088, ast=0x00007ffff72d3300) at genmatch.c:826:12
    frame #12: 0x0000555555c3e032 ponyc`gen_expr(c=0x00007fffffffe088, ast=0x00007ffff72d3300) at genexpr.c:92:13
    frame #13: 0x0000555555cfc144 ponyc`gen_seq(c=0x00007fffffffe088, ast=0x00007ffff72d2780) at gencontrol.c:21:13
    frame #14: 0x0000555555c3df06 ponyc`gen_expr(c=0x00007fffffffe088, ast=0x00007ffff72d2780) at genexpr.c:26:13
    frame #15: 0x0000555555d009d3 ponyc`genfun_newbe(c=0x00007fffffffe088, t=0x00007ffff7630a00, m=0x00007ffff7506f80) at genfun.c:579:24
    frame #16: 0x0000555555cffbb6 ponyc`genfun_method(c=0x00007fffffffe088, t=0x00007ffff7630a00, n=0x00007ffff7506f00, m=0x00007ffff7506f80) at genfun.c:775:15
    frame #17: 0x0000555555cff9e9 ponyc`genfun_method_bodies(c=0x00007fffffffe088, t=0x00007ffff7630a00) at genfun.c:960:11
    frame #18: 0x0000555555c55c50 ponyc`gentypes(c=0x00007fffffffe088) at gentype.c:799:9
    frame #19: 0x0000555555c3d516 ponyc`genexe(c=0x00007fffffffe088, program=0x00007ffff7c3ad00) at genexe.c:566:7
    frame #20: 0x0000555555c323b4 ponyc`codegen(program=0x00007ffff7c3ad00, opt=0x00007fffffffe548) at codegen.c:890:13
    frame #21: 0x0000555555c87aa3 ponyc`generate_passes(program=0x00007ffff7c3ad00, options=0x00007fffffffe548) at pass.c:370:10
    frame #22: 0x0000555555c263ab ponyc`compile_package(path=".", opt=0x00007fffffffe548, print_program_ast=false, print_package_ast=false) at main.c:67:13
    frame #23: 0x0000555555c261d1 ponyc`main(argc=1, argv=0x00007fffffffe758) at main.c:133:13
    frame #24: 0x00007ffff7ca6d90 libc.so.6`__libc_start_call_main(main=(ponyc`main at main.c:73), argc=2, argv=0x00007fffffffe758) at libc_start_call_main.h:58:16
    frame #25: 0x00007ffff7ca6e40 libc.so.6`__libc_start_main_impl(main=(ponyc`main at main.c:73), argc=2, argv=0x00007fffffffe758, init=0x00007ffff7ffd040, fini=<unavailable>, rtld_fini=<unavailable>, stack_end=0x00007fffffffe748) at libc-start.c:392:3
    frame #26: 0x0000555555c25ea5 ponyc`_start + 37

Line 891 in genoperator.c appears to be returning "junk", but not NULL which would be "ok". Next steps is to debug what is going on in the genexpr call.

  LLVMValueRef l_value = gen_expr(c, left);

Once we know what is going on, I think we can discuss the correct fix.

[chriskdon]

The issue seems to be an extra seq AST node that genmatch.c:dynamic_tuple_ptr(...) doesn't expect due to the extra set of parens.

dynamic_tuple_ptr expects to unwrap exactly one seq for each tuple child:

ponyc/src/libponyc/codegen/genmatch.c

Lines 318 to 329 in 46eb348

	for(int i = 0; pattern_child != NULL; i++)
	{
	pony_assert(ast_id(pattern_child) == TK_SEQ);
	// Skip over the SEQ node.
	ast_t* pattern_expr = ast_child(pattern_child);
	if(!dynamic_tuple_element(c, ptr, desc, pattern_expr, next_block, i))
	return false;
	pattern_child = ast_sibling(pattern_child);
	}

The generated AST of the problematic match looks like this:

(match:scope
  ...
  (cases:scope
    (case:scope
       # \/--- dynamic_tuple_ptr is operating on this node
      (tuple
        (seq (456 ...) [nominal (id $0) (id I32) x val x x])
        
        # \/--- Extra seq here causes it to generate a bad value comparison
        (seq (seq ($let ...) [nominal (id $0) (id I32) x val x x])
          [nominal (id $0) (id I32) x val x x])

        [tupletype 
          (nominal (id $0) (id I32) x val x x) 
          (nominal (id $0) (id I32) x val x x)]
      )
      ...
    )
    ...
  )
)

A pseudo trace of what is happening that leads to the crash is as follows:

gen_match()
-> ...
-> dynamic_tuple_ptr()
-> dynamic_match_object(pattern)
  + The `pattern` AST is `(seq ($let ...))`, causing the switch to fall into the default
    case. The `let` is treated as a value check instead of generating the correct match
    capture. Without the parens, the correct `pattern = ($let ...)` AST without the `seq` 
    would be here, and the match capture would be generated.
-> dynamic_value_object()
-> check_value()
-> gen_pattern_eq()
-> gen_eq_rvalue()
  + Calls `LLVMValueRef l_value = gen_expr(c, left)` with `left = (seq ($let ...)`, 
    returning `GEN_NOTNEEDED`
  + `l_value = GEN_NOTNEEDED`
-> make_cmp_value(..., l_value, rvalue, ...)
  + `l_value` is not a real `LLVMValueRef` as it equals `GEN_NOTNEEDED`
  +  `LLVMIsConstant(l_value)` then segfaults trying to check `GEN_NOTNEEDED`

Changing this to a loop in dynamic_tuple_ptr that skips over all the seq nodes seems to work, and all unit tests pass:

for(int i = 0; pattern_child != NULL; i++)
{
  pony_assert(ast_id(pattern_child) == TK_SEQ);

  // Skip over the SEQ nodes
  ast_t* pattern_expr = ast_child(pattern_child);
  while(ast_id(pattern_expr) == TK_SEQ)
  {
    pony_assert(ast_childcount(pattern_expr) == 1);
    pattern_expr = ast_child(pattern_expr);
  }

  if(!dynamic_tuple_element(c, ptr, desc, pattern_expr, next_block, i))
    return false;

  pattern_child = ast_sibling(pattern_child);
}

I noticed this same seq skipping occurring in assign_rvalue:

ponyc/src/libponyc/codegen/genoperator.c

Lines 435 to 442 in 46eb348

	case TK_SEQ:
	// The actual expression is inside a sequence node.
	while(ast_id(left) == TK_SEQ)
	{
	pony_assert(ast_childcount(left) == 1);
	left = ast_child(left);
	}
	return assign_rvalue(c, left, r_type, r_value);

---

I imagine this could occur elsewhere if unexpected nested seq nodes are created due to extra parens. Maybe there is a more generic long term fix where either nested single child seq's (i.e. (seq (seq ...)) are not created when generating the AST, or they are normalized down to a single (seq ...) as a second pass on the entire AST. Is there any reason nested single child seq nodes should exist, or is it safe to say that (seq (seq ...)) = (seq ...)?

[SeanTAllen]

@jemc I think you know this code better than I. Do you have anything to contribute here? If we don't have any feedback for you by next week @chriskdon I'll dig into this, but I wouldn't have time to early October.

[chriskdon]

No rush at all, @SeanTAllen. Thanks for taking a look. I'm happy to take this in whatever the preferred direction is.