From patchwork Mon Nov 25 12:54:51 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Simon Marchi (Code Review)" X-Patchwork-Id: 36184 Received: (qmail 110893 invoked by alias); 25 Nov 2019 12:54:58 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 110819 invoked by uid 89); 25 Nov 2019 12:54:57 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-21.5 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT autolearn=ham version=3.3.1 spammy=INTERNAL, H*F:D*io X-HELO: mx1.osci.io Received: from polly.osci.io (HELO mx1.osci.io) (8.43.85.229) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 25 Nov 2019 12:54:55 +0000 Received: by mx1.osci.io (Postfix, from userid 994) id 490D920362; Mon, 25 Nov 2019 07:54:53 -0500 (EST) Received: from gnutoolchain-gerrit.osci.io (gnutoolchain-gerrit.osci.io [8.43.85.239]) by mx1.osci.io (Postfix) with ESMTP id E4F19203AC for ; Mon, 25 Nov 2019 07:54:51 -0500 (EST) Received: from localhost (localhost [127.0.0.1]) by gnutoolchain-gerrit.osci.io (Postfix) with ESMTP id BB93128173 for ; Mon, 25 Nov 2019 07:54:51 -0500 (EST) X-Gerrit-PatchSet: 1 Date: Mon, 25 Nov 2019 07:54:51 -0500 From: "Mihails Strasuns (Code Review)" To: gdb-patches@sourceware.org Message-ID: Auto-Submitted: auto-generated X-Gerrit-MessageType: newchange Subject: [review] jit: remove bp locations when unregistering jit code X-Gerrit-Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1 X-Gerrit-Change-Number: 704 X-Gerrit-ChangeURL: X-Gerrit-Commit: 10a8b9ce25779daa2ce018a694b1390017e7eeb4 References: Reply-To: mihails.strasuns@intel.com, gdb-patches@sourceware.org MIME-Version: 1.0 Content-Disposition: inline User-Agent: Gerrit/3.0.3-79-g83ff7f88f1 Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/704 ...................................................................... jit: remove bp locations when unregistering jit code Fixes the rare problem when identical JIT code is registered and unregistered several times and its symbols happen to be relocated to the exactly the same addresses as previous time. In such situation gdb would still think that old breakpoint locations are still inserted (because `inserted` flag is set and objfile/address is the same) despite the fact that code memory was overwritten and does not contain a breakpoint trap anymore. This solution removes such breakpoint location from the respective breakpoint location list. That way next time `update_breakpoint_locations` is called, it will re-create a new bp_location instance if that JIT code was loaded again, and won't try reusing the previous one. It is a conservative solution which only applies to object files coming from JIT. Added test case tries to reproduce the problem using the opencl test suite (because current jit test suite doesn't actually execute the code). Without this patch the breakpoint would only trigger during the first kernel run and get completely ignored second run. gdb/ChangeLog: 2019-11-14 Mihails Strasuns * breakpoint.h, breakpoint.c (forget_breakpoint_locations_obj): new function to forget all breakpoint locations for a given objfile. * jit.c (jit_unregister_code): call `forget_breakpoint_locations_obj`. Signed-off-by: Mihails Strasuns Change-Id: Id9133540d67fa0c4619ac88324b0349b89e4b2b1 --- M gdb/breakpoint.c M gdb/breakpoint.h M gdb/jit.c A gdb/testsuite/gdb.opencl/multirun.exp M gdb/testsuite/lib/opencl_hostapp.c 5 files changed, 149 insertions(+), 2 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 583f46d..a184e46 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -3063,6 +3063,48 @@ } } +/* Tries to find `bp` in the linked list of `b` locations + and if found, removes it. In practice that means that breakpoint + will forget about this location and won't try to re-insert it. */ + +static bool +remove_location_from_bp (breakpoint *b, bp_location *bp) +{ + if (b->loc == bp) + { + b->loc = nullptr; + return true; + } + + bp_location *i; + for (i = b->loc; i != nullptr && i->next != bp; i = i->next) + ; + if (i == nullptr) + return false; + i->next = i->next->next; + return true; +} + +/* See breakpoint.h. */ + +int +forget_breakpoint_locations_obj (objfile *obj) +{ + struct bp_location **blp, *bl; + + int count = 0; + + ALL_BP_LOCATIONS (bl, blp) + if (bl->symtab != NULL && SYMTAB_OBJFILE (bl->symtab) == obj) + { + bool ret = remove_location_from_bp (bl->owner, bl); + gdb_assert (ret); + ++count; + } + + return count; +} + static int internal_breakpoint_number = -1; /* Set the breakpoint number of B, depending on the value of INTERNAL. diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h index 5c8f17c..5fa99e9 100644 --- a/gdb/breakpoint.h +++ b/gdb/breakpoint.h @@ -1405,6 +1405,13 @@ extern void remove_breakpoints_inf (inferior *inf); +/* Forgets all breakpoint locations that are inserted + for a given objfile by removing them from the owning breakpoint + linked list. Will not affect actual breakpoints and + if the object file remains these may be re-created during next + `update_breakpoint_locations` call. */ +extern int forget_breakpoint_locations_obj (objfile* obj); + /* This function can be used to update the breakpoint package's state after an exec() system call has been executed. diff --git a/gdb/jit.c b/gdb/jit.c index 5014dfd..9c5a92c 100644 --- a/gdb/jit.c +++ b/gdb/jit.c @@ -960,6 +960,11 @@ { if (jit_debug) fprintf_unfiltered (gdb_stdlog, "jit_unregister_code (%p)\n", objfile); + int count = forget_breakpoint_locations_obj (objfile); + if (jit_debug > 1 && count > 0) + fprintf_unfiltered ( + gdb_stdlog, + "ignoring %d breakpoint locations in the unregistered code\n", count); delete objfile; } diff --git a/gdb/testsuite/gdb.opencl/multirun.exp b/gdb/testsuite/gdb.opencl/multirun.exp new file mode 100644 index 0000000..48e270c --- /dev/null +++ b/gdb/testsuite/gdb.opencl/multirun.exp @@ -0,0 +1,83 @@ +# Copyright 2019 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . */ +# +# Tests global symbol lookup in case a symbol's name in the kernel +# coincides with another in the main executable. + +load_lib opencl.exp + +if { [skip_opencl_tests] } { + return 0 +} + +set name "opencl_kernel" +set app [remote_download target ${srcdir}/lib/${name}.cl] +if { [gdb_compile_opencl_hostapp "${app}" "${name}" "additional_flags=-DRUN_COUNT=2" ] != "" } { + untested "failed to compile" + return -1 +} + +clean_restart ${name} + +# Get to the kernel first +gdb_breakpoint "testkernel" {allow-pending} {temporary} +gdb_test "set debug jit 2" +gdb_test "start" +gdb_continue_to_breakpoint "testkernel" + +# First run +gdb_breakpoint "testkernel" +set count_first 1 +while 1 { + gdb_test_multiple "continue" "first run" { + -re "Breakpoint" { + incr count_first +1 + pass "expected breakpount" + } + -re "jit_unregister_code" { + pass "end of first run" + break + } + -re "exited" { + fail "unexpected temrination" + break + } + } +} + +# Second run +set count_second 0 +while 1 { + gdb_test_multiple "continue" "second run" { + -re "Breakpoint" { + incr count_second +1 + pass "expected breakpount" + } + -re "jit_unregister_code" { + pass "end of second run" + break + } + -re "exited" { + fail "unexpected temrination" + break + } + } +} + +if [expr $count_first==$count_second] then { + pass "same count of breakpoint hits between two runs" +} else { + fail "${count_first} != ${count_second}" +} \ No newline at end of file diff --git a/gdb/testsuite/lib/opencl_hostapp.c b/gdb/testsuite/lib/opencl_hostapp.c index 9d6ce0e..7d63d09 100644 --- a/gdb/testsuite/lib/opencl_hostapp.c +++ b/gdb/testsuite/lib/opencl_hostapp.c @@ -32,13 +32,17 @@ #error "Please specify the OpenCL source file using the CL_SOURCE define" #endif +#ifndef RUN_COUNT +#define RUN_COUNT 1 +#endif + #define STRINGIFY(S) _STRINGIFY(S) #define _STRINGIFY(S) #S #define SIZE 16 -int -main () +static void +run_once () { int err, i; cl_platform_id platform; @@ -163,6 +167,12 @@ CHK (clReleaseCommandQueue (queue)); CHK (clReleaseContext (context)); free (data); +} +int +main () +{ + for (int i = 0; i < RUN_COUNT; ++i) + run_once(); return 0; }