search

Shopify / liquid-c

Liquid performance extension in C.
MIT License
120 stars 25 forks source link

Rename vm.c to liquid_vm.c #195

Closed peterzhu2118 closed 1 year ago

peterzhu2118 commented 1 year ago

It's confusing to debug core dumps because Ruby also has vm.c so it becomes hard to tell which frames come from Ruby and which ones come from liquid-c.

dylanahsmith commented 1 year ago

Doesn't it end up showing the path to the shared library as well as the file in the C level backtrace information? /tmp/bundle/ruby/3.2.0/bundler/gems/liquid-c-e1cad818edff/lib/liquid_c.so(vm_render_until_error+0x2d) [0x7fdb9e20ddd2] vm.c:441. Or are you seeing it elsewhere without that context?

peterzhu2118 commented 1 year ago

We don't have access to the crash report printed by Ruby in production core dumps on bugsnag, so it ends up looking like this in GDB:

#18 0x00007f0ee7a78af8 in vm_invoke_filter (args=0x7f0df298d4b0, num_args=1, filter_name=<optimized out>, vm=0x7f0d5abbd280) at vm.c:179
#19 vm_render_until_error (uncast_args=uncast_args@entry=140729589167664) at vm.c:340
#20 0x000055cf5fdcce9e in rb_vrescue2 (b_proc=0x7f0ee7a78a00 <vm_render_until_error>, data1=140729589167664, r_proc=
    0x7f0ee7a78710 <vm_render_rescue>, data2=140729589167648, args=args@entry=0x7ffe292bf540) at eval.c:917
#21 0x000055cf5fdcd11e in rb_rescue2 (b_proc=b_proc@entry=0x7f0ee7a78a00 <vm_render_until_error>, data1=data1@entry=140729589167664, r_proc=r_proc@entry=
    0x7f0ee7a78710 <vm_render_rescue>, data2=data2@entry=140729589167648) at eval.c:898
#22 0x000055cf5fdcd155 in rb_rescue (b_proc=b_proc@entry=0x7f0ee7a78a00 <vm_render_until_error>, data1=data1@entry=140729589167664, r_proc=r_proc@entry=
    0x7f0ee7a78710 <vm_render_rescue>, data2=data2@entry=140729589167648) at eval.c:967
#23 0x00007f0ee7a79395 in liquid_vm_render (body=<optimized out>, const_ptr=0x7f0d5aba8940, context=context@entry=139696144303520, output=output@entry=139697051592120) at vm.c:575
#24 0x00007f0ee7a721fe in block_body_render_to_output_buffer (self=<optimized out>, context=139696144303520, output=139697051592120) at document_body.h:43
#25 0x000055cf5ffb7fd5 in vm_call_cfunc_with_frame (ec=0x7f0efd07f050, reg_cfp=0x7f0efd180080, calling=<optimized out>) at vm_insnhelper.c:3268
dylanahsmith commented 1 year ago

My concern being that the same argument could be made about many of the other files (e.g. variable.c), which could lead to wanting to prefix all the C source files.

peterzhu2118 commented 1 year ago

I've certainly lost time more than once when debugging because I just brushed over vm.c and thought the frame came from Ruby.

peterzhu2118 commented 1 year ago

My concern being that the same argument could be made about many of the other files (e.g. variable.c), which could lead to wanting to prefix all the C source files.

Fair point, another alternative would be to prefix all liquid functions with liquid_. LMK which one you prefer.

dylanahsmith commented 1 year ago

Renaming files seems preferable than renaming functions. Luckily, it looks like there aren't that many conflicts in practice with ruby itself and maybe the vm.c file is especially common to end up in backtraces.

I wonder how hard it would be to get our crash reporter to provide that extra information when sending it to bugsnag.