Skip to content

Commit

Permalink
seq point fix
Browse files Browse the repository at this point in the history
This was incredibly unstable before this fix. Below is a common class
of failure:

```
* thread mono#1, name = 'tid_307', queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT)
  * frame #0: 0x000000010b5f3877 mono-sgen`decode_var_int(buf="", out_buf=0x00007ffee4a27a10) at seq-points-data.c:49
    frame mono#1: 0x000000010b5f39cf mono-sgen`seq_point_read(seq_point=0x00007ffee4a27a68, ptr="", buffer_ptr="", has_debug_data=0) at seq-points-data.c:144
    frame mono#2: 0x000000010b5f341e mono-sgen`mono_seq_point_iterator_next(it=0x00007ffee4a27a68) at seq-points-data.c:303
    frame mono#3: 0x000000010b5f3476 mono-sgen`mono_seq_point_find_prev_by_native_offset(info=0x00007f89b8c26620, native_offset=50, seq_point=0x00007ffee4a27b90) at seq-points-data.c:243
    frame mono#4: 0x000000010b2e5629 mono-sgen`mono_walk_stack_full(func=(mono-sgen`summarize_frame at mini-exceptions.c:1555), start_ctx=0x00007ffee4a31b20, domain=0x00007f89b8f0dd60, jit_tls=0x00007f89b9820600, lmf=0x00007f89b8f161b0, unwind_options=MONO_UNWIND_LOOKUP_IL_OFFSET, user_data=0x00007ffee4a28128, crash_context=1) at mini-exceptions.c:1279
    frame mono#5: 0x000000010b2e2a82 mono-sgen`mono_summarize_managed_stack(out=0x00007ffee4a29270) at mini-exceptions.c:1614
    frame mono#6: 0x000000010b5c7e43 mono-sgen`summarizer_state_term(state=0x000000010b820ea0, out=0x00007ffee4a31cc0, mem=0x0000000000000000, provided_size=0, controlling=0x00007ffee4a29270) at threads.c:6363
    frame mono#7: 0x000000010b5c7709 mono-sgen`mono_threads_summarize_execute(ctx=0x00007ffee4a31b20, out=0x00007ffee4a31cc0, hashes=0x00007ffee4a31cb0, silent=0, mem=0x0000000000000000, provided_size=0) at threads.c:6434
    frame mono#8: 0x000000010b5c8052 mono-sgen`mono_threads_summarize(ctx=0x00007ffee4a31b20, out=0x00007ffee4a31cc0, hashes=0x00007ffee4a31cb0, silent=0, signal_handler_controller=1, mem=0x0000000000000000, provided_size=0) at threads.c:6487
    frame mono#9: 0x000000010b3f35a6 mono-sgen`dump_native_stacktrace(signal="SIGABRT", ctx=0x00007ffee4a32a80) at mini-posix.c:1017
    frame mono#10: 0x000000010b3f33cf mono-sgen`mono_dump_native_crash_info(signal="SIGABRT", ctx=0x00007ffee4a32a80, info=0x00007ffee4a32a18) at mini-posix.c:1129
    frame mono#11: 0x000000010b2e90d9 mono-sgen`mono_handle_native_crash(signal="SIGABRT", ctx=0x00007ffee4a32a80, info=0x00007ffee4a32a18) at mini-exceptions.c:3227
    frame mono#12: 0x000000010b3f2790 mono-sgen`sigabrt_signal_handler(_dummy=6, _info=0x00007ffee4a32a18, context=0x00007ffee4a32a80) at mini-posix.c:223
    frame mono#13: 0x00007fff73dbdf5a libsystem_platform.dylib`_sigtramp + 26
    frame mono#14: 0x00007fff73bffb67 libsystem_kernel.dylib`__pthread_kill + 11
    frame mono#15: 0x00007fff73dca080 libsystem_pthread.dylib`pthread_kill + 333
    frame mono#16: 0x00007fff73b5b1ae libsystem_c.dylib`abort + 127
    frame mono#17: 0x00007fff73c59822 libsystem_malloc.dylib`free + 521
    frame mono#18: 0x000000010d7699d4 libtest.0.dylib`monoeg_g_free(ptr=0x00007ffee4a32c24) at gmem.c:86
    frame mono#19: 0x000000010d76333b libtest.0.dylib`mono_test_MerpCrashMalloc at libtest.c:7710
```

Where right before the call to seq_point_read, we see:

(lldb) p seq_point_info_inflate(info)
(SeqPointInfoInflated) $11 = (data = <no value available>, len = 20, has_debug_data = 0, alloc_data = 0)

(lldb) p seq_point_info_inflate(info).data
(guint8 *) $12 = 0x5050505050505050 <no value available>

And see that it results from evaluating:

```
if (!if (info_inflated.alloc_data)) memcpy (&info_inflated.data, ptr, sizeof (guint8*));
```

which made me try.

```
            if (info_inflated.alloc_data)
                info_inflated.data = ptr;
            else
-               memcpy (&info_inflated.data, ptr, sizeof (guint8*));
+               memcpy (&info_inflated.data, &ptr, sizeof (ptr));
```.

This works flawlessly.

Now this confuses me because that's the same (I think) as the other if
branch (direct pointer assignment).

What is `alloc_data` supposed to be doing? There are three mentions in
the codebase. There's the changed one, and then these two:

```
/* When alloc_data is set to true data allocation/deallocation is managed by this structure */
```

```
metadata/seq-points-data.c:     if (alloc_data)
metadata/seq-points-data.c-             memcpy (info_ptr, data, len);
metadata/seq-points-data.c-     else
metadata/seq-points-data.c-             memcpy (info_ptr, &data, sizeof (guint8*));
```

So it seems that when it's set, we're supposed to directly point to the
data in the other struct and when it's not set, we're supposed to point
to the address of it? It seems kind of devoid of consistent semantics.
We don't seem to actually have any mentions of this field in
deallocation logic.

Either way, this commit takes a crash that happens in 4/100 merp crash
runs and makes it not reproduce once in 10,000 runs. Don't revert
without an argument why the change makes sense.
  • Loading branch information
alexanderkyte committed Jan 4, 2019
1 parent 65e7e32 commit f44bd91
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion mono/metadata/seq-points-data.c
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ seq_point_info_inflate (MonoSeqPointInfo *info)
if (info_inflated.alloc_data)
info_inflated.data = ptr;
else
memcpy (&info_inflated.data, ptr, sizeof (guint8*));
memcpy (&info_inflated.data, &ptr, sizeof (ptr));

return info_inflated;
}
Expand Down

0 comments on commit f44bd91

Please sign in to comment.