Re: JIT debugging (Attach and speed)

[Yichao Yu <yyc1992@gmail.com>]

>>>> Yeah, there's no full fix available, only some ideas thrown out.
>>>> The last discussed one wouldn't cause a regression -- the
>>>> "longjmp"-caching idea.  We may still need to defer breakpoint re-set
>>>> to at most once per jit load event, something like Paul's original
>>>> patch, but with a breakpoint_re_set call somewhere.

I've just run the profile myself and got some quite different result
from the 2011 thread[1].
With no breakpoint in gdb and simply jitting O(2000) functions: [2]
With one (un-triggered) breakpoint in gdb and jitting O(1500) functions: [3]

It seems that there's other slowness when there are breakpoint created
but in terms of scaling, the fastest growing one is the sorting in
`update_section_map`

[1] https://sourceware.org/ml/gdb/2011-01/msg00009.html
[2] http://i.imgur.com/6Ca6Yal.jpg
[3] http://i.imgur.com/aHKGACX.jpg

>>>> It'd even be better to somehow restrict breakpoint re-setting
>>>> to the jit modules that were added/removed/changed, but
>>>> that's harder.

I've re-read the 2011 thread and I think I have a slightly better
understanding now. IIUC we need to check if the newly registered file
contains any pending breakpoints. Is the problem that this is done by
checking it over all the registered object files? Is it possible to
avoid O(n^2) scaling without only re-setting on the jit object file
currently being modified?

>>>>
>>>>>
>>>>>>>
>>>>>>> Do you know whether this happens with 7.11 and master, and if so,
>>>>>>> would it be possible for you to git bisect the culprit?
>>>>>
>>>>> This is 7.11 package from ArchLinux. I could try bi-secting although
>>>>> apparently you are faster at pin-point the issue.
>>>>>
>>>>>>
>>>>>> Currently, jit_inferior_created_hook -> jit_inferior_init is only
>>>>>> called when the inferior execs...
>>>>>>
>>>>>> Grepping around, I think that might have been
>>>>>> the fix for PR gdb/13431 (03bef283c2d3):
>>>>>>    https://sourceware.org/ml/gdb-patches/2012-02/msg00023.html
>>>>>> which removed the inferior_created (jit_inferior_created_observer).
>>>>>>
>>>>>> Adding an inferior_created observer back likely fixes the issue.
>>>>>
>>>>> I'm happy to test patches.
>>>>
>>>> I'm happy to provide guidance, but a fix would likely happen faster
>>>> if someone else stepped up to write it.
>>>
>>> Are these lines (or at least the first one) the ones you think should
>>> be added back?
>>>
>>> -  observer_attach_inferior_created (jit_inferior_created_observer);
>>>     observer_attach_inferior_exit (jit_inferior_exit_hook);
>>> -  observer_attach_executable_changed (jit_executable_changed_observer);
>>>
>>
>> Something like that.  At least the first one.  Not sure the second is
>> needed, since with Tromey's change the data is associated with the objfile.
>>
>>> I can try that although I'm not particularly sure what was the reason
>>> they are removed
>>
>> Not sure either.  I assume studying Tromey's description of the original
>> change helps bring that to light.
>>
>>> and how to check for regressions.
>>
>> GDB has a regression test suite under src/gdb/testsuite/.  The
>> gdb/testsuite/README file has instructions.
>>
>> Basically, run "make check -j8" before the patch, "make check -j8"
>> after the patch, and diff the resulting testsuite/gdb.sum files.
>>
>> Note that there are some tests that may be racy on your machine, so you
>> may get unrelated some noise.  Running a particular test a
>> couple times, with:
>>
>>  make check TESTS="gdb.base/foo.exp"
>>
>> should help you determine whether that's the case.
>>
>> It'd be very nice if we had a _new_ test that covers your use case,
>> to avoid regressing again.  That likely makes the patch bigger than
>> what we could accept without a copyright assignment though.  If you'd
>> like to pursue that, let me know and I'll send you the forms.
>
> I've got a simple patch that fixes the issue for me and AFAICT all of
> the failing tests are racy and/or failing on this machine before the
> change too. I haven't add test yet since I'm not so sure how to add it
> (I found test for both jit interface and attach but haven't figured
> out how to write a new one yet...).
>
> I'm happy to complete copyright related forms necessary.
>
>>
>> Thanks,
>> Pedro Alves
>>
>
>
> From a94b2c68d83e13ee80e5c21ab27dfedaddfda590 Mon Sep 17 00:00:00 2001
> From: Yichao Yu <yyc1992@gmail.com>
> Date: Tue, 22 Mar 2016 15:24:11 -0400
> Subject: [PATCH] Fix JIT debug when attaching to a process.
>
> ---
>  gdb/jit.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/gdb/jit.c b/gdb/jit.c
> index afc1c51..0bd127b 100644
> --- a/gdb/jit.c
> +++ b/gdb/jit.c
> @@ -1026,7 +1026,7 @@ jit_breakpoint_deleted (struct breakpoint *b)
>  }
>
>  /* (Re-)Initialize the jit breakpoint if necessary.
> -   Return 0 on success.  */
> +   Return 0 if the jit breakpoint has been successfully initialized.  */
>
>  static int
>  jit_breakpoint_re_set_internal (struct gdbarch *gdbarch,
> @@ -1070,7 +1070,7 @@ jit_breakpoint_re_set_internal (struct gdbarch *gdbarch,
>                         paddress (gdbarch, addr));
>
>    if (ps_data->cached_code_address == addr)
> -    return 1;
> +    return 0;
>
>    /* Delete the old breakpoint.  */
>    if (ps_data->jit_breakpoint != NULL)
> @@ -1288,7 +1288,8 @@ static const struct frame_unwind jit_frame_unwind =
>    jit_frame_prev_register,
>    NULL,
>    jit_frame_sniffer,
> -  jit_dealloc_cache
> +  jit_dealloc_cache,
> +  NULL
>  };
>
>
> @@ -1375,6 +1376,12 @@ jit_inferior_created_hook (void)
>    jit_inferior_init (target_gdbarch ());
>  }
>
> +static void
> +jit_inferior_created (struct target_ops *ops, int from_tty)
> +{
> +  jit_inferior_created_hook ();
> +}
> +
>  /* Exported routine to call to re-set the jit breakpoints,
>     e.g. when a program is rerun.  */
>
> @@ -1496,6 +1503,7 @@ _initialize_jit (void)
>                              show_jit_debug,
>                              &setdebuglist, &showdebuglist);
>
> +  observer_attach_inferior_created (jit_inferior_created);
>    observer_attach_inferior_exit (jit_inferior_exit_hook);
>    observer_attach_breakpoint_deleted (jit_breakpoint_deleted);
>
> --
> 2.7.4