From patchwork Thu Nov 30 22:59:16 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Joel Brobecker X-Patchwork-Id: 24648 Received: (qmail 42725 invoked by alias); 30 Nov 2017 22:59:20 -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 42715 invoked by uid 89); 30 Nov 2017 22:59:20 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.0 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KB_WAM_FROM_NAME_SINGLEWORD, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=limiting X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 30 Nov 2017 22:59:18 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 08B1C116B08; Thu, 30 Nov 2017 17:59:17 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id oli6gRyOWE1w; Thu, 30 Nov 2017 17:59:16 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id D1F9C116B07; Thu, 30 Nov 2017 17:59:16 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 57882803E6; Thu, 30 Nov 2017 17:59:16 -0500 (EST) Date: Thu, 30 Nov 2017 17:59:16 -0500 From: Joel Brobecker To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [RFA 5/6] Change Ada exceptions to use std::string Message-ID: <20171130225916.csgxhw2gcauyby6x@adacore.com> References: <20171130030140.14830-1-tom@tromey.com> <20171130030140.14830-6-tom@tromey.com> <20171130220348.cqv3ik5c2lie7xm5@adacore.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20171130220348.cqv3ik5c2lie7xm5@adacore.com> User-Agent: NeoMutt/20170113 (1.7.2) [really attached, this time!] > > This changes the Ada exception code to use std::string, allowing the > > removal of some more cleanups. > > > > ChangeLog > > 2017-11-29 Tom Tromey > > > > * mi/mi-cmd-catch.c (mi_cmd_catch_assert) > > (mi_cmd_catch_exception): Update. > > * ada-lang.h (create_ada_exception_catchpoint): Update. > > * ada-lang.c (struct ada_catchpoint) : Now a > > std::string. > > (create_excep_cond_exprs, ~ada_catchpoint) > > (should_stop_exception, print_one_exception) > > (print_mention_exception, print_recreate_exception): Update. > > (ada_get_next_arg): Remove. > > (catch_ada_exception_command_split): Change two arguments to > > "std::string *". Remove cleanups. > > (ada_exception_catchpoint_cond_string): Change "string" to > > std::string. > > (ada_exception_sal): Remove excep_string parameter. > > (create_ada_exception_catchpoint): Change two arguments to > > "std::string &&". > > (catch_ada_exception_command): Update. > > As hinted in my answer to the cover email, I found a few issues. > Attached is a commit that explains and fixes them. If the fixes > look good to you, a new commit combining the two is preapproved. > > (I tested my patch on x86_64-linux, limiting myself to gdb.ada) From c2ad16ea37be009bf58afb7467f2bdd67b31710a Mon Sep 17 00:00:00 2001 From: Joel Brobecker Date: Thu, 30 Nov 2017 16:15:09 -0500 Subject: [PATCH] Fix a couple of regressions introduced by the previous patch Pb #1: Crash while trying to insert an assert catchpoint: With any program built with "gnatmake -g -gnata ...", trying to insert a catchpoint on failed assertions would cause an internal error: (gdb) catch assert /[...]/common/cleanups.c:264: internal-warning: restore_my_cleanups has found a stale cleanup I tracked this down to an issue in the call to create_ada_exception_catchpoint, where we pass NULL for at least one of the parameters declared as a "std::string &&". I don't know C++ well enough, but instruction-by-instruction leads me to believe that the constructor doesn't like NULL as a char *, so we get an exception, and that exception eventually somehow leads to the cleanup error above (not sure how, unfortunately). The fix was to pass an std::string() instead. And by the time I understood this for the cond string parameter (on which I had target fixation), I had C++-ifed catch_ada_assert_command_split. Pb #2: Same kind of crash insertin an exception in GDB/MI mode. In this case, it was just a case of getting confused between a couple of temporary "char *" variables, and the corresponding std::string parameters that Tom introduced. I first fixed the confusion, but then I wondered why using intermediate "char *" variables at all. So I changed this function to only use std::string variables. Pb #3: condition handling in exception catchpoints is broken (gdb) catch exception constraint_error if True Junk at end of expression Tom removed a call to "skip_spaces" before checking for the next argument, and I am not sure I understand why. But debugging the problem showed by arg was equal to... "constraint_error if True" ... and that once extract_arg is called, args is now equal to... " if True" ... so the condition identifying an "if" block no longer works. I restored the call to skip_spaces. --- gdb/ada-lang.c | 9 +++++---- gdb/mi/mi-cmd-catch.c | 12 +++--------- 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c index 5e9aebf..1caa3dc 100644 --- a/gdb/ada-lang.c +++ b/gdb/ada-lang.c @@ -12744,6 +12744,7 @@ catch_ada_exception_command_split (const char *args, /* Check to see if we have a condition. */ + args = skip_spaces (args); if (startswith (args, "if") && (isspace (args[2]) || args[2] == '\0')) { @@ -12991,7 +12992,7 @@ catch_ada_exception_command (const char *arg_entry, int from_tty, (the memory needs to be deallocated after use). */ static void -catch_ada_assert_command_split (const char *args, char **cond_string) +catch_ada_assert_command_split (const char *args, std::string *cond_string) { args = skip_spaces (args); @@ -13003,7 +13004,7 @@ catch_ada_assert_command_split (const char *args, char **cond_string) args = skip_spaces (args); if (args[0] == '\0') error (_("condition missing after `if' keyword")); - *cond_string = xstrdup (args); + *cond_string = args; } /* Otherwise, there should be no other argument at the end of @@ -13021,7 +13022,7 @@ catch_assert_command (const char *arg_entry, int from_tty, const char *arg = arg_entry; struct gdbarch *gdbarch = get_current_arch (); int tempflag; - char *cond_string = NULL; + std::string cond_string; tempflag = get_cmd_context (command) == CATCH_TEMPORARY; @@ -13029,7 +13030,7 @@ catch_assert_command (const char *arg_entry, int from_tty, arg = ""; catch_ada_assert_command_split (arg, &cond_string); create_ada_exception_catchpoint (gdbarch, ada_catch_assert, - NULL, cond_string, + std::string (), std::move (cond_string), tempflag, 1 /* enabled */, from_tty); } diff --git a/gdb/mi/mi-cmd-catch.c b/gdb/mi/mi-cmd-catch.c index f69cdaa..70939ee 100644 --- a/gdb/mi/mi-cmd-catch.c +++ b/gdb/mi/mi-cmd-catch.c @@ -93,9 +93,9 @@ void mi_cmd_catch_exception (const char *cmd, char *argv[], int argc) { struct gdbarch *gdbarch = get_current_arch(); - char *condition = NULL; + std::string condition; int enabled = 1; - char *exception_name = NULL; + std::string exception_name; int temp = 0; enum ada_exception_catchpoint_kind ex_kind = ada_catch_exception; @@ -152,16 +152,10 @@ mi_cmd_catch_exception (const char *cmd, char *argv[], int argc) /* Specifying an exception name does not make sense when requesting an unhandled exception breakpoint. */ - if (ex_kind == ada_catch_exception_unhandled && exception_name != NULL) + if (ex_kind == ada_catch_exception_unhandled && ! exception_name.empty()) error (_("\"-e\" and \"-u\" are mutually exclusive")); scoped_restore restore_breakpoint_reporting = setup_breakpoint_reporting (); - std::string except_str; - if (exception_name != NULL) - except_str = exception_name; - std::string cond_str; - if (condition != NULL) - cond_str = condition; create_ada_exception_catchpoint (gdbarch, ex_kind, std::move (exception_name), std::move (condition), -- 2.1.4