Fix serialisation of Array and String

[Praetonus]

This change fixes a bug in Array/String_Serialise_Trace where the data pointer would be traced for immutable and opaque objects, instead of mutable and immutable ones.

Closes #2245.

[SeanTAllen]

This is what I get locally for the source of the test failures:

(lldb) run
Process 3197 launched: './stdlib' (x86_64)
Process 3197 stopped
* thread #2: tid = 0x51c4cc, 0x000000010033d78c stdlib`pony_serialise_offset(ctx=0x00000001097ff448, p=0x00000001003689c1) + 188 at serialise.c:213, stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT)
    frame #0: 0x000000010033d78c stdlib`pony_serialise_offset(ctx=0x00000001097ff448, p=0x00000001003689c1) + 188 at serialise.c:213
   210 	  // If we are not in the map, we are an untraced primitive. Return the type id
   211 	  // with the high bit set.
   212 	  pony_type_t* t = *(pony_type_t**)p;
-> 213 	  return (size_t)t->id | HIGH_BIT;
   214 	}
   215
   216 	PONY_API void pony_serialise(pony_ctx_t* ctx, void* p, pony_type_t* t,
(lldb) bt
warning: could not load any Objective-C class information. This will significantly reduce the quality of type information available.
* thread #2: tid = 0x51c4cc, 0x000000010033d78c stdlib`pony_serialise_offset(ctx=0x00000001097ff448, p=0x00000001003689c1) + 188 at serialise.c:213, stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT)
  * frame #0: 0x000000010033d78c stdlib`pony_serialise_offset(ctx=0x00000001097ff448, p=0x00000001003689c1) + 188 at serialise.c:213
    frame #1: 0x00000001003101b8 stdlib`String_Serialise + 88
    frame #2: 0x000000010033d962 stdlib`pony_serialise(ctx=0x00000001097ff448, p=0x000000012162b920, t=0x0000000000000000, out=0x000000012162b960, alloc_fn=(stdlib`serialise_$35$0_apply_oZo), throw_fn=(stdlib`serialise_$35$1_apply_o)) + 434 at serialise.c:245
    frame #3: 0x000000010008e1c4 stdlib`serialise_Serialised_ref_create_ooo + 196
    frame #4: 0x00000001002a05d9 stdlib`serialise__TestArrays_ref_apply_oo + 3065
    frame #5: 0x000000010006fff2 stdlib`ponytest__TestRunner_tag_run_o + 146
    frame #6: 0x0000000100006472 stdlib`ponytest__TestRunner_Dispatch + 1250
    frame #7: 0x00000001003326d9 stdlib`handle_message(ctx=0x00000001097ff448, actor=0x00000001217dd600, msg=0x0000000121732ee0) + 457 at actor.c:152
    frame #8: 0x000000010033237b stdlib`ponyint_actor_run(ctx=0x00000001097ff448, actor=0x00000001217dd600, batch=100) + 107 at actor.c:209
    frame #9: 0x0000000100345f19 stdlib`run(sched=0x00000001097ff400) + 185 at scheduler.c:301
    frame #10: 0x0000000100345c29 stdlib`run_thread(arg=0x00000001097ff400) + 57 at scheduler.c:352
    frame #11: 0x00007fff97df399d libsystem_pthread.dylib`_pthread_body + 131
    frame #12: 0x00007fff97df391a libsystem_pthread.dylib`_pthread_start + 168
    frame #13: 0x00007fff97df1351 libsystem_pthread.dylib`thread_start + 13

[SeanTAllen]

Looking at this, it appears to fail for Arrays of Strings.

It passes if I turn off this test:

    let x5: Array[String] = ["hi"; "there"; "folks"]
    sx = Serialised(serialise, x5)?
    let y5 = sx(deserialise)? as Array[String]
    h.assert_true(x5 isnt y5)
    h.assert_array_eq[String](x5, y5)

[dipinhora]

@Praetonus The following diff fixes the original issue and the segfault that CI is running into with your current fix:

diff --git a/src/libponyc/codegen/genprim.c b/src/libponyc/codegen/genprim.c
index 0e6c9b7..d1fa836 100644
--- a/src/libponyc/codegen/genprim.c
+++ b/src/libponyc/codegen/genprim.c
@@ -782,7 +782,7 @@ void genprim_array_serialise_trace(compile_t* c, reach_type_t* t)
   LLVMBasicBlockRef post_block = codegen_block(c, "post");
 
   LLVMValueRef cond = LLVMBuildICmp(c->builder, LLVMIntNE, size,
-    LLVMConstInt(c->intptr, PONY_TRACE_OPAQUE, false), "");
+    LLVMConstInt(c->intptr, 0, false), "");
   LLVMBuildCondBr(c->builder, cond, trace_block, post_block);
 
   LLVMPositionBuilderAtEnd(c->builder, trace_block);
@@ -1044,15 +1044,6 @@ void genprim_string_serialise_trace(compile_t* c, reach_type_t* t)
 
   LLVMValueRef size = field_value(c, object, 1);
 
-  LLVMBasicBlockRef trace_block = codegen_block(c, "trace");
-  LLVMBasicBlockRef post_block = codegen_block(c, "post");
-
-  LLVMValueRef cond = LLVMBuildICmp(c->builder, LLVMIntNE, size,
-    LLVMConstInt(c->intptr, PONY_TRACE_OPAQUE, false), "");
-  LLVMBuildCondBr(c->builder, cond, trace_block, post_block);
-
-  LLVMPositionBuilderAtEnd(c->builder, trace_block);
-
   LLVMValueRef alloc = LLVMBuildAdd(c->builder, size,
     LLVMConstInt(c->intptr, 1, false), "");
 
@@ -1065,10 +1056,6 @@ void genprim_string_serialise_trace(compile_t* c, reach_type_t* t)
   args[2] = alloc;
   gencall_runtime(c, "pony_serialise_reserve", args, 3, "");
 
-  LLVMBuildBr(c->builder, post_block);
-
-  LLVMPositionBuilderAtEnd(c->builder, post_block);
-
   LLVMBuildRetVoid(c->builder);
   codegen_finishfun(c);
 }

The condition in genprim_array_serialise_trace at

ponyc/src/libponyc/codegen/genprim.c

Lines 784 to 785 in a5c4948

	LLVMValueRef cond = LLVMBuildICmp(c->builder, LLVMIntNE, size,
	LLVMConstInt(c->intptr, PONY_TRACE_OPAQUE, false), "");

should remain 0 and not be changed to PONY_TRACE_OPAQUE because it is comparing with the size of the array to determine if there are any elements to serialise or not. Changing it to not serialize the elements when there are PONY_TRACE_OPAQUE (2) elements is not correct.

The condition in genprim_string_serialise_trace at

ponyc/src/libponyc/codegen/genprim.c

Lines 1050 to 1051 in a5c4948

	LLVMValueRef cond = LLVMBuildICmp(c->builder, LLVMIntNE, size,
	LLVMConstInt(c->intptr, PONY_TRACE_OPAQUE, false), "");

also checks against the size of a string to determine if there is anything to serialize or not. This originally caused a segfault because the call to pony_serialise_reserve wasn't made if the size == 0 (as reported in #2245) and then when pony_serialise_offset is called by genprim_string_serialise later it results in a segfault because the pointer address wasn't added to the serialize hashmap by pony_serialise_reserve earlier and the check at

ponyc/src/libponyrt/gc/serialise.c

Lines 199 to 213 in a5c4948

	serialise_t* s = ponyint_serialise_get(&ctx->serialise, &k, &index);
	// If we are in the map, return the offset.
	if(s != NULL)
	{
	if(s->block || (s->t != NULL && s->t->serialise != NULL))
	return s->value;
	else
	return ALL_BITS;
	}
	// If we are not in the map, we are an untraced primitive. Return the type id
	// with the high bit set.
	pony_type_t* t = *(pony_type_t**)p;
	return (size_t)t->id | HIGH_BIT;

fails resulting in the runtime attempting to dereference the pony_type_t for a pointer. The change to PONY_TRACE_OPAQUE happened to cause the segfault to now occur for any strings of length 2 instead of length 0 and luckily one of the values in the serialise/Arrays test happened to be a 2 character string.

While the diff I've proposed removes the conditional check altogether from genprim_string_serialise_trace, another possible solution would be to add a similar conditional check to genprim_string_serialise to only call pony_serialise_offset if the string size > 0 to avoid the segfault.

[SeanTAllen]

I'm closing this as we merged #2247.