Make backtrace buffer handling more systematic (#33277)

Increase expressibility of what can be stored in backtrace buffers, while ensuring that the GC can find roots without knowing about the detail. To do this, introduce a new "extended backtrace entry" format which carries along the number of roots and other data in a bitpacked format. This allows the backtrace buffer to be traversed and the roots collected in a general way, without the GC knowing about interpreter frames. Use this to add the module to InterperterIP so that the module of interpreted top level thunks can be known. In the future the extended entry format should allow us to be a lot more flexible with what can be stored in a backtrace. For example, we could * Compress the backtrace cycles of runaway recursive functions so that stack overflows are much more likely to fit in the fixed-size bt_data array. * Integrate external or other types of interpreter frames into the backtrace machinery.
JuliaLang · Oct 15, 2019 · 643ec18 · 643ec18
1 parent 876cd55
commit 643ec18
Show file tree

Hide file tree

Showing 18 changed files with 364 additions and 148 deletions.
diff --git a/base/error.jl b/base/error.jl
@@ -62,25 +62,39 @@ rethrow(e) = ccall(:jl_rethrow_other, Bottom, (Any,), e)
 struct InterpreterIP
     code::Union{CodeInfo,Core.MethodInstance,Nothing}
     stmt::Csize_t
+    mod::Union{Module,Nothing}
 end
 
-# convert dual arrays (ips, interpreter_frames) to a single array of locations
+# convert dual arrays (raw bt buffer, array of GC managed values) to a single
+# array of locations
 function _reformat_bt(bt, bt2)
     ret = Vector{Union{InterpreterIP,Ptr{Cvoid}}}()
     i, j = 1, 1
     while i <= length(bt)
         ip = bt[i]::Ptr{Cvoid}
-        if UInt(ip) == (-1 % UInt)
-            # The next one is really a CodeInfo
-            push!(ret, InterpreterIP(
-                bt2[j],
-                bt[i+2]))
-            j += 1
-            i += 3
-        else
-            push!(ret, Ptr{Cvoid}(ip))
+        if UInt(ip) != (-1 % UInt) # See also jl_bt_is_native
+            # native frame
+            push!(ret, ip)
             i += 1
+            continue
+        end
+        # Extended backtrace entry
+        entry_metadata = reinterpret(UInt, bt[i+1])
+        njlvalues =  entry_metadata & 0x7
+        nuintvals = (entry_metadata >> 3) & 0x7
+        tag       = (entry_metadata >> 6) & 0xf
+        header    =  entry_metadata >> 10
+        if tag == 1 # JL_BT_INTERP_FRAME_TAG
+            code = bt2[j]
+            mod = njlvalues == 2 ? bt2[j+1] : nothing
+            push!(ret, InterpreterIP(code, header, mod))
+        else
+            # Tags we don't know about are an error
+            throw(ArgumentError("Unexpected extended backtrace entry tag $tag at bt[$i]"))
         end
+        # See jl_bt_entry_size
+        j += njlvalues
+        i += Int(2 + njlvalues + nuintvals)
     end
     ret
 end

diff --git a/base/errorshow.jl b/base/errorshow.jl
@@ -705,7 +705,7 @@ end
 function show(io::IO, ip::InterpreterIP)
     print(io, typeof(ip))
     if ip.code isa Core.CodeInfo
-        print(io, " in top-level CodeInfo at statement $(Int(ip.stmt))")
+        print(io, " in top-level CodeInfo for $(ip.mod) at statement $(Int(ip.stmt))")
     else
         print(io, " in $(ip.code) at statement $(Int(ip.stmt))")
     end

diff --git a/base/stacktraces.jl b/base/stacktraces.jl
@@ -7,6 +7,7 @@ module StackTraces
 
 
 import Base: hash, ==, show
+import Core: CodeInfo, MethodInstance
 
 export StackTrace, StackFrame, stacktrace
 
@@ -52,7 +53,7 @@ struct StackFrame # this type should be kept platform-agnostic so that profiles
     "the line number in the file containing the execution context"
     line::Int
     "the MethodInstance or CodeInfo containing the execution context (if it could be found)"
-    linfo::Union{Core.MethodInstance, Core.CodeInfo, Nothing}
+    linfo::Union{MethodInstance, CodeInfo, Nothing}
     "true if the code is from C"
     from_c::Bool
     "true if the code is from an inlined frame"
@@ -118,7 +119,7 @@ end
 const top_level_scope_sym = Symbol("top-level scope")
 
 function lookup(ip::Base.InterpreterIP)
-    if ip.code isa Core.MethodInstance && ip.code.def isa Method
+    if ip.code isa MethodInstance && ip.code.def isa Method
         codeinfo = ip.code.uninferred
         func = ip.code.def.name
         file = ip.code.def.file
@@ -127,7 +128,7 @@ function lookup(ip::Base.InterpreterIP)
         # interpreted top-level expression with no CodeInfo
         return [StackFrame(top_level_scope_sym, empty_sym, 0, nothing, false, false, 0)]
     else
-        @assert ip.code isa Core.CodeInfo
+        @assert ip.code isa CodeInfo
         codeinfo = ip.code
         func = top_level_scope_sym
         file = empty_sym
@@ -206,7 +207,7 @@ function remove_frames!(stack::StackTrace, m::Module)
     return stack
 end
 
-is_top_level_frame(f::StackFrame) = f.linfo isa Core.CodeInfo || (f.linfo === nothing && f.func === top_level_scope_sym)
+is_top_level_frame(f::StackFrame) = f.linfo isa CodeInfo || (f.linfo === nothing && f.func === top_level_scope_sym)
 
 function show_spec_linfo(io::IO, frame::StackFrame)
     if frame.linfo === nothing
@@ -220,7 +221,7 @@ function show_spec_linfo(io::IO, frame::StackFrame)
                         :nothing
             printstyled(io, Base.demangle_function_name(string(frame.func)), color=color)
         end
-    elseif frame.linfo isa Core.MethodInstance
+    elseif frame.linfo isa MethodInstance
         def = frame.linfo.def
         if isa(def, Method)
             sig = frame.linfo.specTypes
@@ -243,7 +244,7 @@ function show_spec_linfo(io::IO, frame::StackFrame)
         else
             Base.show(io, frame.linfo)
         end
-    elseif frame.linfo isa Core.CodeInfo
+    elseif frame.linfo isa CodeInfo
         print(io, "top-level scope")
     end
 end
@@ -276,7 +277,7 @@ function from(frame::StackFrame, m::Module)
     finfo = frame.linfo
     result = false
 
-    if finfo isa Core.MethodInstance
+    if finfo isa MethodInstance
         frame_m = finfo.def
         isa(frame_m, Method) && (frame_m = frame_m.module)
         result = nameof(frame_m) === nameof(m)

diff --git a/src/gc.c b/src/gc.c
@@ -2059,34 +2059,41 @@ excstack: {
         gc_mark_excstack_t *stackitr = gc_pop_markdata(&sp, gc_mark_excstack_t);
         jl_excstack_t *excstack = stackitr->s;
         size_t itr = stackitr->itr;
-        size_t i = stackitr->i;
+        size_t bt_index = stackitr->bt_index;
+        size_t jlval_index = stackitr->jlval_index;
         while (itr > 0) {
             size_t bt_size = jl_excstack_bt_size(excstack, itr);
-            uintptr_t *bt_data = jl_excstack_bt_data(excstack, itr);
-            while (i+2 < bt_size) {
-                if (bt_data[i] != JL_BT_INTERP_FRAME) {
-                    i++;
+            jl_bt_element_t *bt_data = jl_excstack_bt_data(excstack, itr);
+            for (; bt_index < bt_size; bt_index += jl_bt_entry_size(bt_data + bt_index)) {
+                jl_bt_element_t *bt_entry = bt_data + bt_index;
+                if (jl_bt_is_native(bt_entry))
                     continue;
-                }
-                // found an interpreter frame to mark
-                new_obj = (jl_value_t*)bt_data[i+1];
-                uintptr_t nptr = 0;
-                i += 3;
-                if (gc_try_setmark(new_obj, &nptr, &tag, &bits)) {
-                    stackitr->i = i;
-                    stackitr->itr = itr;
-                    gc_repush_markdata(&sp, gc_mark_excstack_t);
-                    goto mark;
+                // Found an extended backtrace entry: iterate over any
+                // GC-managed values inside.
+                size_t njlvals = jl_bt_num_jlvals(bt_entry);
+                while (jlval_index < njlvals) {
+                    new_obj = jl_bt_entry_jlvalue(bt_entry, jlval_index);
+                    uintptr_t nptr = 0;
+                    jlval_index += 1;
+                    if (gc_try_setmark(new_obj, &nptr, &tag, &bits)) {
+                        stackitr->itr = itr;
+                        stackitr->bt_index = bt_index;
+                        stackitr->jlval_index = jlval_index;
+                        gc_repush_markdata(&sp, gc_mark_excstack_t);
+                        goto mark;
+                    }
                 }
             }
-            // mark the exception
+            // The exception comes last - mark it
             new_obj = jl_excstack_exception(excstack, itr);
             itr = jl_excstack_next(excstack, itr);
-            i = 0;
+            bt_index = 0;
+            jlval_index = 0;
             uintptr_t nptr = 0;
             if (gc_try_setmark(new_obj, &nptr, &tag, &bits)) {
-                stackitr->i = i;
                 stackitr->itr = itr;
+                stackitr->bt_index = bt_index;
+                stackitr->jlval_index = jlval_index;
                 gc_repush_markdata(&sp, gc_mark_excstack_t);
                 goto mark;
             }
@@ -2359,7 +2366,7 @@ mark: {
             if (ta->excstack) {
                 gc_setmark_buf_(ptls, ta->excstack, bits, sizeof(jl_excstack_t) +
                                 sizeof(uintptr_t)*ta->excstack->reserved_size);
-                gc_mark_excstack_t stackdata = {ta->excstack, ta->excstack->top, 0};
+                gc_mark_excstack_t stackdata = {ta->excstack, ta->excstack->top, 0, 0};
                 gc_mark_stack_push(&ptls->gc_cache, &sp, gc_mark_laddr(excstack),
                                    &stackdata, sizeof(stackdata), 1);
             }
@@ -2654,13 +2661,15 @@ static void jl_gc_queue_remset(jl_gc_mark_cache_t *gc_cache, jl_gc_mark_sp_t *sp
 
 static void jl_gc_queue_bt_buf(jl_gc_mark_cache_t *gc_cache, jl_gc_mark_sp_t *sp, jl_ptls_t ptls2)
 {
-    size_t n = 0;
-    while (n+2 < ptls2->bt_size) {
-        if (ptls2->bt_data[n] == JL_BT_INTERP_FRAME) {
-            gc_mark_queue_obj(gc_cache, sp, (jl_value_t*)ptls2->bt_data[n+1]);
-            n += 2;
-        }
-        n++;
+    jl_bt_element_t *bt_data = ptls2->bt_data;
+    size_t bt_size = ptls2->bt_size;
+    for (size_t i = 0; i < bt_size; i += jl_bt_entry_size(bt_data + i)) {
+        jl_bt_element_t *bt_entry = bt_data + i;
+        if (jl_bt_is_native(bt_entry))
+            continue;
+        size_t njlvals = jl_bt_num_jlvals(bt_entry);
+        for (size_t j = 0; j < njlvals; j++)
+            gc_mark_queue_obj(gc_cache, sp, jl_bt_entry_jlvalue(bt_entry, j));
     }
 }
 

diff --git a/src/gc.h b/src/gc.h
@@ -153,9 +153,10 @@ typedef struct {
 
 // Exception stack data
 typedef struct {
-    jl_excstack_t *s;  // Stack of exceptions
-    size_t itr;        // Iterator into exception stack
-    size_t i;          // Iterator into backtrace data for exception
+    jl_excstack_t *s;   // Stack of exceptions
+    size_t itr;         // Iterator into exception stack
+    size_t bt_index;    // Current backtrace buffer entry index
+    size_t jlval_index; // Index into GC managed values for current bt entry
 } gc_mark_excstack_t;
 
 // Module bindings. This is also the beginning of module scanning.

diff --git a/src/interpreter-stacktrace.c b/src/interpreter-stacktrace.c
@@ -390,20 +390,29 @@ JL_DLLEXPORT int jl_is_enter_interpreter_frame(uintptr_t ip)
     return enter_interpreter_frame_start <= ip && ip <= enter_interpreter_frame_end;
 }
 
-JL_DLLEXPORT size_t jl_capture_interp_frame(uintptr_t *data, uintptr_t sp, uintptr_t fp, size_t space_remaining)
+JL_DLLEXPORT size_t jl_capture_interp_frame(jl_bt_element_t *bt_entry, uintptr_t sp,
+                                            uintptr_t fp, size_t space_remaining)
 {
 #ifdef FP_CAPTURE_OFFSET
     interpreter_state *s = (interpreter_state *)(fp-FP_CAPTURE_OFFSET);
 #else
     interpreter_state *s = (interpreter_state *)(sp+TOTAL_STACK_PADDING);
 #endif
-    int required_space = 3;
+    int need_module = !s->mi;
+    int required_space = need_module ? 4 : 3;
     if (space_remaining < required_space)
-        return 0;
-    // Sentinel value to indicate an interpreter frame
-    data[0] = JL_BT_INTERP_FRAME;
-    data[1] = s->mi ? (uintptr_t)s->mi : s->src ? (uintptr_t)s->src : (uintptr_t)jl_nothing;
-    data[2] = (uintptr_t)s->ip;
+        return 0; // Should not happen
+    size_t njlvalues = need_module ? 2 : 1;
+    uintptr_t entry_tags = jl_bt_entry_descriptor(njlvalues, 0, JL_BT_INTERP_FRAME_TAG, s->ip);
+    bt_entry[0].uintptr = JL_BT_NON_PTR_ENTRY;
+    bt_entry[1].uintptr = entry_tags;
+    bt_entry[2].jlvalue = s->mi  ? (jl_value_t*)s->mi  :
+                          s->src ? (jl_value_t*)s->src : (jl_value_t*)jl_nothing;
+    if (need_module) {
+        // If we only have a CodeInfo (s->src), we are in a top level thunk and
+        // need to record the module separately.
+        bt_entry[3].jlvalue = (jl_value_t*)s->module;
+    }
     return required_space;
 }
 
@@ -419,7 +428,8 @@ JL_DLLEXPORT int jl_is_enter_interpreter_frame(uintptr_t ip)
     return 0;
 }
 
-JL_DLLEXPORT size_t jl_capture_interp_frame(uintptr_t *data, uintptr_t sp, uintptr_t fp, size_t space_remaining)
+JL_DLLEXPORT size_t jl_capture_interp_frame(jl_bt_element_t *bt_entry, uintptr_t sp,
+                                            uintptr_t fp, size_t space_remaining)
 {
     // Leave bt_entry[0] as the native instruction ptr
     return 0;

diff --git a/src/julia.h b/src/julia.h
@@ -236,6 +236,13 @@ typedef struct _jl_llvm_functions_t {
 
 typedef struct _jl_method_instance_t jl_method_instance_t;
 
+typedef struct _jl_line_info_node_t {
+    jl_value_t *method;
+    jl_sym_t *file;
+    intptr_t line;
+    intptr_t inlined_at;
+} jl_line_info_node_t;
+
 // This type describes a single function body
 typedef struct _jl_code_info_t {
     // ssavalue-indexed arrays of properties: