Re: JIT debugging (Attach and speed)

[Yichao Yu <yyc1992 at gmail dot com>]

Sorry for the delay, I was busy with other stuff...

>>>>> I re-read the 2011 discussion, and it seems like we had an idea for a fix:
>>>>
>>>> IIUC the proposed fix might cause regression in some cases?
>>>
>>> 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.
>>>
>>> It'd even be better to somehow restrict breakpoint re-setting
>>> to the jit modules that were added/removed/changed, but
>>> that's harder.
>>>
>>>>
>>>>>>
>>>>>> 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