From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x332.google.com (mail-wm1-x332.google.com [IPv6:2a00:1450:4864:20::332]) by sourceware.org (Postfix) with ESMTPS id AFA53385781D for ; Thu, 20 May 2021 10:33:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org AFA53385781D Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=andrew.burgess@embecosm.com Received: by mail-wm1-x332.google.com with SMTP id u133so8864482wmg.1 for ; Thu, 20 May 2021 03:33:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=hEMA38MPdSHM0IALItydfLGptGP/BkVHK3efjihy6r4=; b=R7M0NTwrW9+rTYjvfBJ4Rps13A2V/WbL+78UXT8uknmJYqg81uuNBf83QT97YWBUW9 2SKxFWdx7QA8OJRQ610VKrm1Xe4KuvlotZYmcaJBCV0R7kfXub4XtfjX/mEYED18kg2b vH+Az9fN39IP+QAtdNZgoexJ3+HHE/03fmtxlmrwzcDOygla8HE/shY7cLcHFesUoEPH DXK19ck47Owrc7qARQGQtyp+vdrHlH+E4146sfS+X7hjdzmUjejzrSzVjTuqP+iMjGPn XrcRxvcw8sYWJHhRSGFDBcm4BZWDGBKM9gcIsQQjjtiezJ8Q4y5uFgHK6hSUzkizHauY 7F4g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=hEMA38MPdSHM0IALItydfLGptGP/BkVHK3efjihy6r4=; b=lsegQ/kkrHAuWPcmH1/EpvZ0B54W7OXSamAAfrSnSFkaw9bGEUSL0e0Jk280Dl9jik EJZhcaxSlBCmkiDkAv/h+PW+gxThlEaydCqwSqqYLvbvIbZNtrTXVgUhloQlCVgw0QxD /BrA0MRo/tB7zEVNEewI7eGuoXubq7dJWfrcMf1pSsY1TOMUJrWW11OeyiTyCzXTVFk/ aM2ARKACre1ypDmIsgTQnpJmn+QTmS3JyeQ0kPHYJsqKqiNVYQIXl/oTivJC+Lwbt8HE /ERq3Bqsq7u/YvdYcUi75xcjxdOqzSYqoCUbA+6zubBpCiMLEzfPwTpVWefeR6H8Vdjy MT+w== X-Gm-Message-State: AOAM533MmMHt9UKYkF4rKJcL/DmFruyBcOMd6qw92tfUCWgahHJ8ZH3p ggNdd74vf23i/1lm1q8CUM0F0x+mDMHh3g== X-Google-Smtp-Source: ABdhPJwXKIZHlvhmhPtmw041RdQnjJ5/m3iiLXP9a5/NMwpUD0kU0Pg3TQrj80UFL/uSP/XTC7q+gA== X-Received: by 2002:a05:600c:4f04:: with SMTP id l4mr2920749wmq.18.1621506815741; Thu, 20 May 2021 03:33:35 -0700 (PDT) Received: from localhost (host109-151-46-70.range109-151.btcentralplus.com. [109.151.46.70]) by smtp.gmail.com with ESMTPSA id l8sm2572026wrw.71.2021.05.20.03.33.08 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 20 May 2021 03:33:19 -0700 (PDT) Date: Thu, 20 May 2021 11:32:57 +0100 From: Andrew Burgess To: George Barrett Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] guile: stop procedures on invalid breakpoints Message-ID: <20210520103257.GB2672@embecosm.com> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Operating-System: Linux/5.8.18-100.fc31.x86_64 (x86_64) X-Uptime: 11:28:00 up 1 day, 12 min, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Spam-Status: No, score=-12.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 20 May 2021 10:33:38 -0000 * George Barrett via Gdb-patches [2021-05-20 09:28:28 +1000]: > Stop procedures on objects are independent of the > breakpoints in GDB core: the only reference made to the GDB breakpoint > pointer in either of `breakpoint-stop' or `set-breakpoint-stop!' is in > the latter checking to ensure that there hasn't already been a stop > condition attached from elsewhere. This check is not applicable to > not-yet-registered objects allocated from Scheme. > > This commit changes the above-mentioned procedures to accept invalid > objects originating from Scheme; this allows the > decoupling of the creation of a specific breakpoint object from its > registration (as well as making the interface less restrictive than it > needs to be). Thanks for doing this. > > gdb/ChangeLog: > > 2021-05-20 George Barrett > > * guile/scm-breakpoint.c > (bpscm_get_valid_or_scm_breakpoint_smob_arg_unsafe): Add > helper function. > (gdbscm_breakpoint_stop): Use > bpscm_get_valid_or_scm_breakpoint_smob_arg_unsafe. > (gdbscm_set_breakpoint_stop_x): Likewise. Check that bp is > non-NULL before doing condition string tests. > > gdb/doc/ChangeLog: > > 2021-05-20 George Barrett > > * guile.texi (Breakpoints In Guile): Add note that > breakpoint-stop and set-breakpoint-stop! may be used on > invalid objects if they originated from > Scheme. Eli will review the doc parts. > > gdb/testsuite/ChangeLog: > > 2021-05-20 George Barrett > > * gdb.guile/scm-breakpoint.exp (test_bkpt_eval_funcs): Add > tests for stop procedure manipulation on invalid > objects originating from Scheme. > Add tests for stop procedure manipulation on valid and invalid > objects originating from GDB core. > --- > gdb/doc/guile.texi | 10 ++++++ > gdb/guile/scm-breakpoint.c | 31 +++++++++++++++--- > gdb/testsuite/gdb.guile/scm-breakpoint.exp | 37 +++++++++++++++++++++- > 3 files changed, 73 insertions(+), 5 deletions(-) > > diff --git a/gdb/doc/guile.texi b/gdb/doc/guile.texi > index c7e43c8d63a..84590eb19bf 100644 > --- a/gdb/doc/guile.texi > +++ b/gdb/doc/guile.texi > @@ -3182,6 +3182,11 @@ becomes unconditional. > @deffn {Scheme Procedure} breakpoint-stop breakpoint > Return the stop predicate of @var{breakpoint}. > See @code{set-breakpoint-stop!} below in this section. > + > +If @var{breakpoint} was created using @code{make-breakpoint}, this > +procedure may be used even if @code{breakpoint-valid?} would return > +@code{#f}. In that case it returns the stop procedure that will be used > +by @var{breakpoint} once the breakpoint has been registered. > @end deffn > > @deffn {Scheme Procedure} set-breakpoint-stop! breakpoint procedure|#f > @@ -3215,6 +3220,11 @@ Example @code{stop} implementation: > (register-breakpoint! bkpt) > (set-breakpoint-stop! bkpt my-stop?) > @end smallexample > + > +If @var{breakpoint} was created using @code{make-breakpoint}, this > +procedure may be used even if @code{breakpoint-valid?} would return > +@code{#f}. In that case @var{procedure} will be the stop procedure for > +@var{breakpoint} when the breakpoint is registered. > @end deffn > > @deffn {Scheme Procedure} breakpoint-commands breakpoint > diff --git a/gdb/guile/scm-breakpoint.c b/gdb/guile/scm-breakpoint.c > index 826dfa9b0a3..4215736ebfe 100644 > --- a/gdb/guile/scm-breakpoint.c > +++ b/gdb/guile/scm-breakpoint.c > @@ -326,6 +326,27 @@ bpscm_get_valid_breakpoint_smob_arg_unsafe (SCM self, int arg_pos, > > return bp_smob; > } > + > +/* Returns the breakpoint smob in SELF, verifying it's either valid or > + originates from Scheme. > + Throws an exception if SELF is not a object, > + or is invalid and not allocated from Scheme. */ > + > +static breakpoint_smob * > +bpscm_get_valid_or_scm_breakpoint_smob_arg_unsafe (SCM self, int arg_pos, > + const char *func_name) > +{ > + breakpoint_smob *bp_smob > + = bpscm_get_breakpoint_smob_arg_unsafe (self, arg_pos, func_name); > + > + if (!bpscm_is_valid (bp_smob) && !bp_smob->is_scheme_bkpt) > + { > + gdbscm_invalid_object_error (func_name, arg_pos, self, > + _("")); > + } The { ... } should be removed for single statement blocks. > + > + return bp_smob; > +} > > /* Breakpoint methods. */ > > @@ -918,7 +939,8 @@ static SCM > gdbscm_breakpoint_stop (SCM self) > { > breakpoint_smob *bp_smob > - = bpscm_get_valid_breakpoint_smob_arg_unsafe (self, SCM_ARG1, FUNC_NAME); > + = bpscm_get_valid_or_scm_breakpoint_smob_arg_unsafe (self, SCM_ARG1, > + FUNC_NAME); > > return bp_smob->stop; > } > @@ -930,7 +952,8 @@ static SCM > gdbscm_set_breakpoint_stop_x (SCM self, SCM newvalue) > { > breakpoint_smob *bp_smob > - = bpscm_get_valid_breakpoint_smob_arg_unsafe (self, SCM_ARG1, FUNC_NAME); > + = bpscm_get_valid_or_scm_breakpoint_smob_arg_unsafe (self, SCM_ARG1, > + FUNC_NAME); > const struct extension_language_defn *extlang = NULL; > > SCM_ASSERT_TYPE (gdbscm_is_procedure (newvalue) > @@ -938,9 +961,9 @@ gdbscm_set_breakpoint_stop_x (SCM self, SCM newvalue) > newvalue, SCM_ARG2, FUNC_NAME, > _("procedure or #f")); > > - if (bp_smob->bp->cond_string != NULL) > + if (bp_smob->bp != NULL && bp_smob->bp->cond_string != NULL) > extlang = get_ext_lang_defn (EXT_LANG_GDB); > - if (extlang == NULL) > + if (bp_smob->bp != NULL && extlang == NULL) > extlang = get_breakpoint_cond_ext_lang (bp_smob->bp, EXT_LANG_GUILE); I guess it's just me, but I'd prefer to see the 'bp_smob->bp != NULL' factored out of these two checks like: if (bp_smob->bp != nullptr) { if (bp_smob->bp->cond_string != nullptr) ... if (extlang == nullptr) ... } though this is not a requirement, if you feel strongly that your way is better then that's fine. But I think you should s/NULL/nullptr/ on these lines (and given there are only 2 other uses of NULL in this function, maybe replace them too?) Otherwise, this looks great. Thanks, Andrew > if (extlang != NULL) > { > diff --git a/gdb/testsuite/gdb.guile/scm-breakpoint.exp b/gdb/testsuite/gdb.guile/scm-breakpoint.exp > index 56058942e64..1739793465c 100644 > --- a/gdb/testsuite/gdb.guile/scm-breakpoint.exp > +++ b/gdb/testsuite/gdb.guile/scm-breakpoint.exp > @@ -376,10 +376,45 @@ proc_with_prefix test_bkpt_eval_funcs { } { > "= 4" \ > "check non firing same-location breakpoint eval function was also called at each stop 2" > > + # Check that stop funcs can be manipulated on invalid Scheme-created > + # breakpoints. > + > + delete_breakpoints > + gdb_test "guile (print (breakpoint-valid? eval-bp1))" "= #f" \ > + "check Scheme-created breakpoint is invalid" > + gdb_scm_test_silent_cmd "guile (set-breakpoint-stop! eval-bp1 (const 'test!))" \ > + "check setting stop procedure on invalid Scheme-created breakpoint" > + gdb_test "guile (print ((breakpoint-stop eval-bp1)))" "= test!" \ > + "check stop procedure on invalid Scheme-created breakpoint was successfully set" > + > + # Check that stop funcs can be manipulated on breakpoint wrappers. > + > + gdb_breakpoint "main" > + gdb_scm_test_silent_cmd "guile (define bp-wrapper (car (breakpoints)))" \ > + "get breakpoint wrapper" > + gdb_test "guile (print (breakpoint-valid? bp-wrapper))" "= #t" \ > + "check breakpoint wrapper is valid" > + gdb_scm_test_silent_cmd "guile (set-breakpoint-stop! bp-wrapper (const 'test!))" \ > + "check setting stop procedure on breakpoit wrapper" > + gdb_test "guile (print ((breakpoint-stop bp-wrapper)))" "= test!" \ > + "check stop procedure on breakpoint wrapper was successfully set" > + > + # Check that stop funcs cannot be manipulated on invalid breakpoint > + # wrappers. > + > + delete_breakpoints > + gdb_test "guile (print (breakpoint-valid? bp-wrapper))" "= #f" \ > + "check breakpoint wrapper is invalid" > + gdb_test "guile (set-breakpoint-stop! bp-wrapper (const 'test!))" \ > + "ERROR:.*Invalid object: .*" \ > + "check stop procedure cannot be set on invalid breakpoint wrapper" > + gdb_test "guile (breakpoint-stop bp-wrapper)" \ > + "ERROR:.*Invalid object: .*" \ > + "check stop procedure cannot be retrieved from invalid breakpoint wrapper" > + > # Check we cannot assign a condition to a breakpoint with a stop-func, > # and cannot assign a stop-func to a breakpoint with a condition. > > - delete_breakpoints > set cond_bp [gdb_get_line_number "Break at multiply."] > gdb_scm_test_silent_cmd "guile (define eval-bp1 (make-bp-eval \"$cond_bp\"))" \ > "create eval-bp1 breakpoint 2" > -- > 2.31.1