From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11487 invoked by alias); 14 Aug 2017 11:53:36 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 11464 invoked by uid 89); 14 Aug 2017 11:53:34 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=releasing, classification, visibly X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 14 Aug 2017 11:53:32 +0000 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9F94161BAC; Mon, 14 Aug 2017 11:53:31 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 9F94161BAC Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=palves@redhat.com Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 9C90468710; Mon, 14 Aug 2017 11:53:30 +0000 (UTC) Subject: Re: [PATCH 0/3] Create arch_lwp_info class hierarchy To: Simon Marchi References: <1500892797-7523-1-git-send-email-simon.marchi@ericsson.com> <22f9058d-52de-293a-eef8-6af1572955d0@redhat.com> Cc: Simon Marchi , gdb-patches@sourceware.org From: Pedro Alves Message-ID: Date: Mon, 14 Aug 2017 11:53:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-08/txt/msg00286.txt.bz2 On 08/12/2017 12:29 PM, Simon Marchi wrote: > But then I realized that I forgot to include the header for s390, and > the compiler (when building for a s390 host) didn't warn me. This is > dangerous and fragile since we end up with two definitions of > arch_lwp_info (the s390 one and that fallback one), and nothing to warn > about it. Not sure whether that's really such a bad problem. It seems like the sort of thing that'd crash quite visibly/quickly if you actually try to run the resulting binary. It's not like we're adding ports every day. :-) > So I changed it to listing explicitly the architectures that > don't defined their own arch_lwp_info: > > ... > #elif defined __alpha__ || defined __powerpc__ || ... > /* Define a dummy arch_lwp_info for arches that don't define one. */ > struct arch_lwp_info {}; > #else > # error "Missing arch-specific include." > #endif > > That would work, but requires listing all the arches that need the > fallback definition of arch_lwp_info, so it gets pretty ugly. > > Any idea to make this simple but safe? Otherwise, I'll just go with the > current version of the patch. There's a 3rd, much simpler option. The problem we ran into is one of blurred division of responsibility: the arch-specific code is responsible for allocating the arch-specific arch_lwp_info, which it must be, because it's not possible to allocate an incomplete type (must know its size), but we free the object in common code, where the type is opaque. We can solve the original xfree poisoning problem by simply making the arch-specific code responsible for releasing arch_lwp_info too, where the type is known, just like it is responsible for allocating it. See below a proof of concept doing that for the x86 port. Making other archs do the same should be trivial. Using new/delete/cdtors/in-class initialization for arch_lwp_info would be a separate, orthogonal change (and so would class-ification of linux_nat_new_thread, linux_target_ops, etc.). >From 40188bdb4ff7eae2876960808672c72529c934b5 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Mon, 14 Aug 2017 12:16:11 +0100 Subject: [PATCH] arch-specific code deletes arch-specific bits --- gdb/gdbserver/linux-low.h | 4 ++++ gdb/gdbserver/linux-x86-low.c | 1 + gdb/linux-nat.c | 18 +++++++++++++++++- gdb/linux-nat.h | 3 +++ gdb/nat/x86-linux.c | 6 ++++++ gdb/nat/x86-linux.h | 4 ++++ gdb/x86-linux-nat.c | 1 + 7 files changed, 36 insertions(+), 1 deletion(-) diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h index 4e7fbec..e7f76d1 100644 --- a/gdb/gdbserver/linux-low.h +++ b/gdb/gdbserver/linux-low.h @@ -194,6 +194,10 @@ struct linux_target_ops allocate it here. */ void (*new_thread) (struct lwp_info *); + /* Hook to call when a thread is being deleted. If extra per-thread + architecture-specific data is needed, delete it here. */ + void (*delete_thread) (struct lwp_info *); + /* Hook to call, if any, when a new fork is attached. */ void (*new_fork) (struct process_info *parent, struct process_info *child); diff --git a/gdb/gdbserver/linux-x86-low.c b/gdb/gdbserver/linux-x86-low.c index 12c7a1b..338d203 100644 --- a/gdb/gdbserver/linux-x86-low.c +++ b/gdb/gdbserver/linux-x86-low.c @@ -2961,6 +2961,7 @@ struct linux_target_ops the_low_target = x86_siginfo_fixup, x86_linux_new_process, x86_linux_new_thread, + x86_linux_delete_thread, x86_linux_new_fork, x86_linux_prepare_to_resume, x86_linux_process_qsupported, diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c index 3dc3637..6be4f5b 100644 --- a/gdb/linux-nat.c +++ b/gdb/linux-nat.c @@ -197,6 +197,9 @@ static struct target_ops linux_ops_saved; /* The method to call, if any, when a new thread is attached. */ static void (*linux_nat_new_thread) (struct lwp_info *); +/* The method to call, if any, when a thread is destroyed. */ +static void (*linux_nat_delete_thread) (struct lwp_info *); + /* The method to call, if any, when a new fork is attached. */ static linux_nat_new_fork_ftype *linux_nat_new_fork; @@ -837,7 +840,9 @@ static int check_ptrace_stopped_lwp_gone (struct lwp_info *lp); static void lwp_free (struct lwp_info *lp) { - xfree (lp->arch_private); + /* Let the arch specific bits release arch_lwp_info. */ + if (linux_nat_delete_thread != NULL) + linux_nat_delete_thread (lp); delete lp; } @@ -4863,6 +4868,17 @@ linux_nat_set_new_thread (struct target_ops *t, linux_nat_new_thread = new_thread; } +/* Register a method to call whenever a new thread is attached. */ +void +linux_nat_set_delete_thread (struct target_ops *t, + void (*delete_thread) (struct lwp_info *)) +{ + /* Save the pointer. We only support a single registered instance + of the GNU/Linux native target, so we do not need to map this to + T. */ + linux_nat_delete_thread = delete_thread; +} + /* See declaration in linux-nat.h. */ void diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h index a783674..8d8e033 100644 --- a/gdb/linux-nat.h +++ b/gdb/linux-nat.h @@ -169,6 +169,9 @@ void linux_nat_add_target (struct target_ops *); /* Register a method to call whenever a new thread is attached. */ void linux_nat_set_new_thread (struct target_ops *, void (*) (struct lwp_info *)); +/* Register a method to call whenever a new thread is deleted. */ +void linux_nat_set_delete_thread (struct target_ops *, + void (*) (struct lwp_info *)); /* Register a method to call whenever a new fork is attached. */ typedef void (linux_nat_new_fork_ftype) (struct lwp_info *parent, diff --git a/gdb/nat/x86-linux.c b/gdb/nat/x86-linux.c index b499e74..e2e9cee 100644 --- a/gdb/nat/x86-linux.c +++ b/gdb/nat/x86-linux.c @@ -65,6 +65,12 @@ x86_linux_new_thread (struct lwp_info *lwp) lwp_set_debug_registers_changed (lwp, 1); } +void +x86_linux_delete_thread (struct lwp_info *lwp) +{ + xfree (lwp_arch_private_info (lwp)); +} + /* See nat/x86-linux.h. */ void diff --git a/gdb/nat/x86-linux.h b/gdb/nat/x86-linux.h index 1b7fee4..32c4922 100644 --- a/gdb/nat/x86-linux.h +++ b/gdb/nat/x86-linux.h @@ -39,6 +39,10 @@ extern int lwp_debug_registers_changed (struct lwp_info *lwp); extern void x86_linux_new_thread (struct lwp_info *lwp); +/* Function to call when a thread is being deleted. */ + +extern void x86_linux_delete_thread (struct lwp_info *lwp); + /* Function to call prior to resuming a thread. */ extern void x86_linux_prepare_to_resume (struct lwp_info *lwp); diff --git a/gdb/x86-linux-nat.c b/gdb/x86-linux-nat.c index 2c4afb1..d0480bd 100644 --- a/gdb/x86-linux-nat.c +++ b/gdb/x86-linux-nat.c @@ -387,6 +387,7 @@ x86_linux_add_target (struct target_ops *t) { linux_nat_add_target (t); linux_nat_set_new_thread (t, x86_linux_new_thread); + linux_nat_set_delete_thread (t, x86_linux_delete_thread); linux_nat_set_new_fork (t, x86_linux_new_fork); linux_nat_set_forget_process (t, x86_forget_process); linux_nat_set_prepare_to_resume (t, x86_linux_prepare_to_resume); -- 2.5.5