Re: JIT debugging (Attach and speed)

Yichao Yu
2016-03-23T02:18:47Z

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