* [RFA 0/3] More Ada cleanup removal @ 2018-05-19 20:37 Tom Tromey 2018-05-19 16:06 ` [RFA 2/3] Remove cleanup from ada_collect_symbol_completion_matches Tom Tromey ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Tom Tromey @ 2018-05-19 20:37 UTC (permalink / raw) To: gdb-patches This resurrects some of the patches from my old Ada cleanup removal thread. https://sourceware.org/ml/gdb-patches/2017-11/msg00812.html Now, I believe patch #3 addresses the comments in https://sourceware.org/ml/gdb-patches/2017-11/msg00853.html Tested by the buildbot. Tom ^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFA 2/3] Remove cleanup from ada_collect_symbol_completion_matches 2018-05-19 20:37 [RFA 0/3] More Ada cleanup removal Tom Tromey @ 2018-05-19 16:06 ` Tom Tromey 2018-05-21 0:09 ` Simon Marchi 2018-05-19 16:06 ` [RFA 3/3] Change ada_catchpoint::excep_string to be a std::string Tom Tromey 2018-05-19 20:37 ` [RFA 1/3] Remove cleanup from ada-lang.c Tom Tromey 2 siblings, 1 reply; 11+ messages in thread From: Tom Tromey @ 2018-05-19 16:06 UTC (permalink / raw) To: gdb-patches; +Cc: Tom Tromey ada_collect_symbol_completion_matches installs a null_cleanup but not any other cleanups. This patch removes it. gdb/ChangeLog 2018-05-18 Tom Tromey <tom@tromey.com> * ada-lang.c (ada_collect_symbol_completion_matches): Remove cleanup. --- gdb/ChangeLog | 5 +++++ gdb/ada-lang.c | 3 --- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c index da5f75030c..cbb0fe9d82 100644 --- a/gdb/ada-lang.c +++ b/gdb/ada-lang.c @@ -6445,7 +6445,6 @@ ada_collect_symbol_completion_matches (completion_tracker &tracker, struct objfile *objfile; const struct block *b, *surrounding_static_block = 0; struct block_iterator iter; - struct cleanup *old_chain = make_cleanup (null_cleanup, NULL); gdb_assert (code == TYPE_CODE_UNDEF); @@ -6550,8 +6549,6 @@ ada_collect_symbol_completion_matches (completion_tracker &tracker, lookup_name, text, word); } } - - do_cleanups (old_chain); } /* Field Access */ -- 2.13.6 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFA 2/3] Remove cleanup from ada_collect_symbol_completion_matches 2018-05-19 16:06 ` [RFA 2/3] Remove cleanup from ada_collect_symbol_completion_matches Tom Tromey @ 2018-05-21 0:09 ` Simon Marchi 0 siblings, 0 replies; 11+ messages in thread From: Simon Marchi @ 2018-05-21 0:09 UTC (permalink / raw) To: Tom Tromey, gdb-patches On 2018-05-19 12:06 PM, Tom Tromey wrote: > ada_collect_symbol_completion_matches installs a null_cleanup but not > any other cleanups. This patch removes it. It doesn't seem like any of the called function leaves a dangling cleanup, so LGTM. Simon ^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFA 3/3] Change ada_catchpoint::excep_string to be a std::string 2018-05-19 20:37 [RFA 0/3] More Ada cleanup removal Tom Tromey 2018-05-19 16:06 ` [RFA 2/3] Remove cleanup from ada_collect_symbol_completion_matches Tom Tromey @ 2018-05-19 16:06 ` Tom Tromey 2018-05-21 1:12 ` Simon Marchi 2018-05-19 20:37 ` [RFA 1/3] Remove cleanup from ada-lang.c Tom Tromey 2 siblings, 1 reply; 11+ messages in thread From: Tom Tromey @ 2018-05-19 16:06 UTC (permalink / raw) To: gdb-patches; +Cc: Tom Tromey This changes ada_catchpoint::excep_string to be a std::string and then fixes up all t he users. This found a memory leak in catch_ada_exception_command_split, where "cond" was copied but never freed. I changed the type of the "cond_string" argument to catch_ada_exception_command_split to follow the rule that out parameters should be pointers and not references. This patch enables the removal of some cleanups and also the function ada_get_next_arg. gdb/ChangeLog 2018-05-18 Tom Tromey <tom@tromey.com> * mi/mi-cmd-catch.c (mi_cmd_catch_assert) (mi_cmd_catch_exception, mi_cmd_catch_handlers): Update. * ada-lang.h (create_ada_exception_catchpoint): Update. * ada-lang.c (struct ada_catchpoint) <excep_string>: 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): Use std::string. Change type of "excep_string", "cond_string". (catch_ada_exception_command): Update. (create_ada_exception_catchpoint): Change type of excep_string. (ada_exception_sal): Remove excep_string parameter. --- gdb/ChangeLog | 17 +++++++ gdb/ada-lang.c | 122 +++++++++++++++----------------------------------- gdb/ada-lang.h | 2 +- gdb/mi/mi-cmd-catch.c | 18 +++----- 4 files changed, 59 insertions(+), 100 deletions(-) diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c index cbb0fe9d82..159c1a0da2 100644 --- a/gdb/ada-lang.c +++ b/gdb/ada-lang.c @@ -12480,7 +12480,7 @@ struct ada_catchpoint : public breakpoint ~ada_catchpoint () override; /* The name of the specific exception the user specified. */ - char *excep_string; + std::string excep_string; }; /* Parse the exception condition string in the context of each of the @@ -12493,7 +12493,7 @@ create_excep_cond_exprs (struct ada_catchpoint *c, struct bp_location *bl; /* Nothing to do if there's no specific exception to catch. */ - if (c->excep_string == NULL) + if (c->excep_string.empty ()) return; /* Same if there are no locations... */ @@ -12503,7 +12503,7 @@ create_excep_cond_exprs (struct ada_catchpoint *c, /* Compute the condition expression in text form, from the specific expection we want to catch. */ std::string cond_string - = ada_exception_catchpoint_cond_string (c->excep_string, ex); + = ada_exception_catchpoint_cond_string (c->excep_string.c_str (), ex); /* Iterate over all the catchpoint's locations, and parse an expression for each. */ @@ -12541,7 +12541,6 @@ create_excep_cond_exprs (struct ada_catchpoint *c, ada_catchpoint::~ada_catchpoint () { - xfree (this->excep_string); } /* Implement the ALLOCATE_LOCATION method in the breakpoint_ops @@ -12584,7 +12583,7 @@ should_stop_exception (const struct bp_location *bl) int stop; /* With no specific exception, should always stop. */ - if (c->excep_string == NULL) + if (c->excep_string.empty ()) return 1; if (ada_loc->excep_cond_expr == NULL) @@ -12734,12 +12733,12 @@ print_one_exception (enum ada_exception_catchpoint_kind ex, switch (ex) { case ada_catch_exception: - if (c->excep_string != NULL) + if (!c->excep_string.empty ()) { - char *msg = xstrprintf (_("`%s' Ada exception"), c->excep_string); + std::string msg = string_printf (_("`%s' Ada exception"), + c->excep_string.c_str ()); uiout->field_string ("what", msg); - xfree (msg); } else uiout->field_string ("what", "all Ada exceptions"); @@ -12751,11 +12750,11 @@ print_one_exception (enum ada_exception_catchpoint_kind ex, break; case ada_catch_handlers: - if (c->excep_string != NULL) + if (!c->excep_string.empty ()) { uiout->field_fmt ("what", _("`%s' Ada exception handlers"), - c->excep_string); + c->excep_string.c_str ()); } else uiout->field_string ("what", "all Ada exceptions handlers"); @@ -12789,10 +12788,10 @@ print_mention_exception (enum ada_exception_catchpoint_kind ex, switch (ex) { case ada_catch_exception: - if (c->excep_string != NULL) + if (!c->excep_string.empty ()) { std::string info = string_printf (_("`%s' Ada exception"), - c->excep_string); + c->excep_string.c_str ()); uiout->text (info.c_str ()); } else @@ -12804,11 +12803,11 @@ print_mention_exception (enum ada_exception_catchpoint_kind ex, break; case ada_catch_handlers: - if (c->excep_string != NULL) + if (!c->excep_string.empty ()) { std::string info = string_printf (_("`%s' Ada exception handlers"), - c->excep_string); + c->excep_string.c_str ()); uiout->text (info.c_str ()); } else @@ -12838,8 +12837,8 @@ print_recreate_exception (enum ada_exception_catchpoint_kind ex, { case ada_catch_exception: fprintf_filtered (fp, "catch exception"); - if (c->excep_string != NULL) - fprintf_filtered (fp, " %s", c->excep_string); + if (!c->excep_string.empty ()) + fprintf_filtered (fp, " %s", c->excep_string.c_str ()); break; case ada_catch_exception_unhandled: @@ -13048,40 +13047,6 @@ print_recreate_catch_handlers (struct breakpoint *b, static struct breakpoint_ops catch_handlers_breakpoint_ops; -/* Return a newly allocated copy of the first space-separated token - in ARGSP, and then adjust ARGSP to point immediately after that - token. - - Return NULL if ARGPS does not contain any more tokens. */ - -static char * -ada_get_next_arg (const char **argsp) -{ - const char *args = *argsp; - const char *end; - char *result; - - args = skip_spaces (args); - if (args[0] == '\0') - return NULL; /* No more arguments. */ - - /* Find the end of the current argument. */ - - end = skip_to_space (args); - - /* Adjust ARGSP to point to the start of the next argument. */ - - *argsp = end; - - /* Make a copy of the current argument and return it. */ - - result = (char *) xmalloc (end - args + 1); - strncpy (result, args, end - args); - result[end - args] = '\0'; - - return result; -} - /* Split the arguments specified in a "catch exception" command. Set EX to the appropriate catchpoint type. Set EXCEP_STRING to the name of the specific exception if @@ -13096,24 +13061,20 @@ static void catch_ada_exception_command_split (const char *args, bool is_catch_handlers_cmd, enum ada_exception_catchpoint_kind *ex, - char **excep_string, - std::string &cond_string) + std::string *excep_string, + std::string *cond_string) { - struct cleanup *old_chain = make_cleanup (null_cleanup, NULL); - char *exception_name; - char *cond = NULL; + std::string exception_name; - exception_name = ada_get_next_arg (&args); - if (exception_name != NULL && strcmp (exception_name, "if") == 0) + exception_name = extract_arg (&args); + if (exception_name == "if") { /* This is not an exception name; this is the start of a condition expression for a catchpoint on all exceptions. So, "un-get" this token, and set exception_name to NULL. */ - xfree (exception_name); - exception_name = NULL; + exception_name.clear (); args -= 2; } - make_cleanup (xfree, exception_name); /* Check to see if we have a condition. */ @@ -13126,8 +13087,7 @@ catch_ada_exception_command_split (const char *args, if (args[0] == '\0') error (_("Condition missing after `if' keyword")); - cond = xstrdup (args); - make_cleanup (xfree, cond); + *cond_string = args; args += strlen (args); } @@ -13138,25 +13098,23 @@ catch_ada_exception_command_split (const char *args, if (args[0] != '\0') error (_("Junk at end of expression")); - discard_cleanups (old_chain); - if (is_catch_handlers_cmd) { /* Catch handling of exceptions. */ *ex = ada_catch_handlers; *excep_string = exception_name; } - else if (exception_name == NULL) + else if (exception_name.empty ()) { /* Catch all exceptions. */ *ex = ada_catch_exception; - *excep_string = NULL; + excep_string->clear (); } - else if (strcmp (exception_name, "unhandled") == 0) + else if (exception_name == "unhandled") { /* Catch unhandled exceptions. */ *ex = ada_catch_exception_unhandled; - *excep_string = NULL; + excep_string->clear (); } else { @@ -13164,8 +13122,6 @@ catch_ada_exception_command_split (const char *args, *ex = ada_catch_exception; *excep_string = exception_name; } - if (cond != NULL) - cond_string.assign (cond); } /* Return the name of the symbol on which we should break in order to @@ -13289,15 +13245,12 @@ ada_exception_catchpoint_cond_string (const char *excep_string, /* Return the symtab_and_line that should be used to insert an exception catchpoint of the TYPE kind. - EXCEP_STRING should contain the name of a specific exception that - the catchpoint should catch, or NULL otherwise. - ADDR_STRING returns the name of the function where the real breakpoint that implements the catchpoints is set, depending on the type of catchpoint we need to create. */ static struct symtab_and_line -ada_exception_sal (enum ada_exception_catchpoint_kind ex, char *excep_string, +ada_exception_sal (enum ada_exception_catchpoint_kind ex, const char **addr_string, const struct breakpoint_ops **ops) { const char *sym_name; @@ -13333,13 +13286,11 @@ ada_exception_sal (enum ada_exception_catchpoint_kind ex, char *excep_string, EX_KIND is the kind of exception catchpoint to be created. - If EXCEPT_STRING is NULL, this catchpoint is expected to trigger + If EXCEPT_STRING is empty, this catchpoint is expected to trigger for all exceptions. Otherwise, EXCEPT_STRING indicates the name - of the exception to which this catchpoint applies. When not NULL, - the string must be allocated on the heap, and its deallocation - is no longer the responsibility of the caller. + of the exception to which this catchpoint applies. - COND_STRING, if not NULL, is the catchpoint condition. This string + COND_STRING, if not empty, is the catchpoint condition. This string must be allocated on the heap, and its deallocation is no longer the responsibility of the caller. @@ -13351,7 +13302,7 @@ ada_exception_sal (enum ada_exception_catchpoint_kind ex, char *excep_string, void create_ada_exception_catchpoint (struct gdbarch *gdbarch, enum ada_exception_catchpoint_kind ex_kind, - char *excep_string, + const std::string &excep_string, const std::string &cond_string, int tempflag, int disabled, @@ -13359,8 +13310,7 @@ create_ada_exception_catchpoint (struct gdbarch *gdbarch, { const char *addr_string = NULL; const struct breakpoint_ops *ops = NULL; - struct symtab_and_line sal - = ada_exception_sal (ex_kind, excep_string, &addr_string, &ops); + struct symtab_and_line sal = ada_exception_sal (ex_kind, &addr_string, &ops); std::unique_ptr<ada_catchpoint> c (new ada_catchpoint ()); init_ada_exception_breakpoint (c.get (), gdbarch, sal, addr_string, @@ -13382,7 +13332,7 @@ catch_ada_exception_command (const char *arg_entry, int from_tty, struct gdbarch *gdbarch = get_current_arch (); int tempflag; enum ada_exception_catchpoint_kind ex_kind; - char *excep_string = NULL; + std::string excep_string; std::string cond_string; tempflag = get_cmd_context (command) == CATCH_TEMPORARY; @@ -13390,7 +13340,7 @@ catch_ada_exception_command (const char *arg_entry, int from_tty, if (!arg) arg = ""; catch_ada_exception_command_split (arg, false, &ex_kind, &excep_string, - cond_string); + &cond_string); create_ada_exception_catchpoint (gdbarch, ex_kind, excep_string, cond_string, tempflag, 1 /* enabled */, @@ -13407,7 +13357,7 @@ catch_ada_handlers_command (const char *arg_entry, int from_tty, struct gdbarch *gdbarch = get_current_arch (); int tempflag; enum ada_exception_catchpoint_kind ex_kind; - char *excep_string = NULL; + std::string excep_string; std::string cond_string; tempflag = get_cmd_context (command) == CATCH_TEMPORARY; @@ -13415,7 +13365,7 @@ catch_ada_handlers_command (const char *arg_entry, int from_tty, if (!arg) arg = ""; catch_ada_exception_command_split (arg, true, &ex_kind, &excep_string, - cond_string); + &cond_string); create_ada_exception_catchpoint (gdbarch, ex_kind, excep_string, cond_string, tempflag, 1 /* enabled */, diff --git a/gdb/ada-lang.h b/gdb/ada-lang.h index 09e7b403f2..a4192fc8a5 100644 --- a/gdb/ada-lang.h +++ b/gdb/ada-lang.h @@ -377,7 +377,7 @@ extern char *ada_main_name (void); extern void create_ada_exception_catchpoint (struct gdbarch *gdbarch, enum ada_exception_catchpoint_kind ex_kind, - char *excep_string, const std::string &cond_string, int tempflag, + const std::string &excep_string, const std::string &cond_string, int tempflag, int disabled, int from_tty); /* Some information about a given Ada exception. */ diff --git a/gdb/mi/mi-cmd-catch.c b/gdb/mi/mi-cmd-catch.c index 078e73ab6a..77c9f952f5 100644 --- a/gdb/mi/mi-cmd-catch.c +++ b/gdb/mi/mi-cmd-catch.c @@ -79,8 +79,8 @@ mi_cmd_catch_assert (const char *cmd, char *argv[], int argc) error (_("Invalid argument: %s"), argv[oind]); scoped_restore restore_breakpoint_reporting = setup_breakpoint_reporting (); - create_ada_exception_catchpoint (gdbarch, ada_catch_assert, - NULL, condition, temp, enabled, 0); + create_ada_exception_catchpoint (gdbarch, ada_catch_assert, std::string (), + condition, temp, enabled, 0); } /* Handler for the -catch-exception command. */ @@ -91,7 +91,7 @@ mi_cmd_catch_exception (const char *cmd, char *argv[], int argc) struct gdbarch *gdbarch = get_current_arch(); 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; @@ -148,14 +148,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 (); - /* create_ada_exception_catchpoint needs EXCEPTION_NAME to be - xstrdup'ed, and will assume control of its lifetime. */ - if (exception_name != NULL) - exception_name = xstrdup (exception_name); create_ada_exception_catchpoint (gdbarch, ex_kind, exception_name, condition, temp, enabled, 0); @@ -169,7 +165,7 @@ mi_cmd_catch_handlers (const char *cmd, char *argv[], int argc) struct gdbarch *gdbarch = get_current_arch (); std::string condition; int enabled = 1; - char *exception_name = NULL; + std::string exception_name; int temp = 0; int oind = 0; @@ -220,10 +216,6 @@ mi_cmd_catch_handlers (const char *cmd, char *argv[], int argc) scoped_restore restore_breakpoint_reporting = setup_breakpoint_reporting (); - /* create_ada_exception_catchpoint needs EXCEPTION_NAME to be - xstrdup'ed, and will assume control of its lifetime. */ - if (exception_name != NULL) - exception_name = xstrdup (exception_name); create_ada_exception_catchpoint (gdbarch, ada_catch_handlers, exception_name, condition, temp, enabled, 0); -- 2.13.6 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFA 3/3] Change ada_catchpoint::excep_string to be a std::string 2018-05-19 16:06 ` [RFA 3/3] Change ada_catchpoint::excep_string to be a std::string Tom Tromey @ 2018-05-21 1:12 ` Simon Marchi 2018-05-22 14:49 ` [pushed+RFC] C++ exception during command triggers stale cleanup internal-warning Joel Brobecker 0 siblings, 1 reply; 11+ messages in thread From: Simon Marchi @ 2018-05-21 1:12 UTC (permalink / raw) To: Tom Tromey, gdb-patches On 2018-05-19 12:06 PM, Tom Tromey wrote: > This changes ada_catchpoint::excep_string to be a std::string and then > fixes up all t he users. > > This found a memory leak in catch_ada_exception_command_split, where > "cond" was copied but never freed. > > I changed the type of the "cond_string" argument to > catch_ada_exception_command_split to follow the rule that out > parameters should be pointers and not references. It wasn't a rule yet (AFAIK), but it is now. https://sourceware.org/ml/gdb-patches/2018-05/msg00450.html The patch LGTM, with some nits. > > This patch enables the removal of some cleanups and also the function > ada_get_next_arg. You can remove the ada_catchpoint destructor I think. > @@ -13333,13 +13286,11 @@ ada_exception_sal (enum ada_exception_catchpoint_kind ex, char *excep_string, > > EX_KIND is the kind of exception catchpoint to be created. > > - If EXCEPT_STRING is NULL, this catchpoint is expected to trigger > + If EXCEPT_STRING is empty, this catchpoint is expected to trigger > for all exceptions. Otherwise, EXCEPT_STRING indicates the name > - of the exception to which this catchpoint applies. When not NULL, > - the string must be allocated on the heap, and its deallocation > - is no longer the responsibility of the caller. > + of the exception to which this catchpoint applies. > > - COND_STRING, if not NULL, is the catchpoint condition. This string > + COND_STRING, if not empty, is the catchpoint condition. This string > must be allocated on the heap, and its deallocation is no longer > the responsibility of the caller. The last part of this paragraph is probably no longer valid. Simon ^ permalink raw reply [flat|nested] 11+ messages in thread
* [pushed+RFC] C++ exception during command triggers stale cleanup internal-warning 2018-05-21 1:12 ` Simon Marchi @ 2018-05-22 14:49 ` Joel Brobecker 2018-05-22 14:50 ` [pushed/Ada] fix "stale cleanup" internal-warning when using "catch assert" command Joel Brobecker 2018-05-22 14:58 ` [pushed+RFC] C++ exception during command triggers stale cleanup internal-warning Tom Tromey 0 siblings, 2 replies; 11+ messages in thread From: Joel Brobecker @ 2018-05-22 14:49 UTC (permalink / raw) To: gdb-patches; +Cc: Tom Tromey Hello, There was a small bug in Tom's patch that caused the execution of the "catch assert" command to throw a C++ exception. I have pushed the following patch to master as an obvious fix to the immediate issue: [] fix "stale cleanup" internal-warning when using "catch assert" But at the same time, this leads me to believe we may have a weakness top.c::execute_command, which installs a cleanup, and "forgets" to discard it when C++ exceptions are raised. But maybe that's just the way it is, and we just want to make sure we never let C+ exceptions get away from us? Thoughts on that? Anyways, for now, the immediate issue is fixed. -- Joel ^ permalink raw reply [flat|nested] 11+ messages in thread
* [pushed/Ada] fix "stale cleanup" internal-warning when using "catch assert" command 2018-05-22 14:49 ` [pushed+RFC] C++ exception during command triggers stale cleanup internal-warning Joel Brobecker @ 2018-05-22 14:50 ` Joel Brobecker 2018-05-22 14:58 ` [pushed+RFC] C++ exception during command triggers stale cleanup internal-warning Tom Tromey 1 sibling, 0 replies; 11+ messages in thread From: Joel Brobecker @ 2018-05-22 14:50 UTC (permalink / raw) To: gdb-patches; +Cc: Tom Tromey Trying to insert a catchpoint on all Ada assertions now triggers the following internal warning regardless of the situation. For instance, not even debugging any program: (gdb) catch assert /[...]/gdb/common/cleanups.c:264: internal-warning: restore_my_cleanups has found a stale cleanup This is due to a small bug in the following C++-ification commit: commit bc18fbb575437dd10089ef4619e46c0b9a93097d Date: Fri May 18 15:58:50 2018 -0600 Subject: Change ada_catchpoint::excep_string to be a std::string The stale cleanup in question is the following one in top.c:execute_command: cleanup_if_error = make_bpstat_clear_actions_cleanup (); This cleanup is expected to be discarded if there are no exception. There were no GDB exception; however, a C++ exception was triggered, because we passed NULL as the excep_string argument when calling create_ada_exception_catchpoint, which is a reference to a const string. So we get a C++ exception during the std::string constructor, which propagates up, causing the cleanup to unexpectedly remain in the cleanup chain. This patch fixes the immediate issue of the incorrect call to create_ada_exception_catchpoint. gdb/ChangeLog: * ada-lang.c (catch_assert_command): Pass empty string instead of NULL for excep_string argument. Tested on x86_64-linux, fixes the following failures: * catch_assert_if.exp: insert catchpoint on failed assertions with condition * catch_ex.exp: insert catchpoint on failed assertions This also fixes about a dozen UNRESOLVED tests that are a consequence of the two tests above failing and crashing GDB. Pushed to master. --- gdb/ChangeLog | 5 +++++ gdb/ada-lang.c | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 8d425a7..653771a 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,8 @@ +2018-05-22 Joel Brobecker <brobecker@adacore.com> + + * ada-lang.c (catch_assert_command): Pass empty string instead + of NULL for excep_string argument. + 2018-05-22 Maciej W. Rozycki <macro@mips.com> * mips-linux-nat.c (mips64_linux_register_addr): Return -1 if diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c index eaf3058..64bddc2 100644 --- a/gdb/ada-lang.c +++ b/gdb/ada-lang.c @@ -13408,7 +13408,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, + "", cond_string, tempflag, 1 /* enabled */, from_tty); } -- 2.1.4 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [pushed+RFC] C++ exception during command triggers stale cleanup internal-warning 2018-05-22 14:49 ` [pushed+RFC] C++ exception during command triggers stale cleanup internal-warning Joel Brobecker 2018-05-22 14:50 ` [pushed/Ada] fix "stale cleanup" internal-warning when using "catch assert" command Joel Brobecker @ 2018-05-22 14:58 ` Tom Tromey 2018-05-22 15:48 ` Joel Brobecker 1 sibling, 1 reply; 11+ messages in thread From: Tom Tromey @ 2018-05-22 14:58 UTC (permalink / raw) To: Joel Brobecker; +Cc: gdb-patches, Tom Tromey >>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes: Joel> There was a small bug in Tom's patch that caused the execution Joel> of the "catch assert" command to throw a C++ exception. Sorry about that. Joel> But at the same time, this leads me to believe we may have a weakness Joel> top.c::execute_command, which installs a cleanup, and "forgets" to Joel> discard it when C++ exceptions are raised. My view was always that dangling cleanups are nearly always bugs; with the exceptions being functions that either mention "cleanup" in the name or return a cleanup. I'm still hoping that we can just remove all cleanups and not have to deal with the issue any more. Tom ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [pushed+RFC] C++ exception during command triggers stale cleanup internal-warning 2018-05-22 14:58 ` [pushed+RFC] C++ exception during command triggers stale cleanup internal-warning Tom Tromey @ 2018-05-22 15:48 ` Joel Brobecker 0 siblings, 0 replies; 11+ messages in thread From: Joel Brobecker @ 2018-05-22 15:48 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches > Joel> But at the same time, this leads me to believe we may have a weakness > Joel> top.c::execute_command, which installs a cleanup, and "forgets" to > Joel> discard it when C++ exceptions are raised. > > My view was always that dangling cleanups are nearly always bugs; with > the exceptions being functions that either mention "cleanup" in the name > or return a cleanup. Completely agreed. My question is: Should the function catch C++ exceptions in order to take care of the cleanup before re-throwing? Thinking things further, I am realizing that this function is most likely not alone in that situation. So there is no clear reason to be treating that function specifically. > I'm still hoping that we can just remove all cleanups and not have to > deal with the issue any more. Agree on the goal. To say that I am not a fan of cleanups would be an understatement (I used to like them, until I realized how tricky they are to maintain). -- Joel ^ permalink raw reply [flat|nested] 11+ messages in thread
* [RFA 1/3] Remove cleanup from ada-lang.c 2018-05-19 20:37 [RFA 0/3] More Ada cleanup removal Tom Tromey 2018-05-19 16:06 ` [RFA 2/3] Remove cleanup from ada_collect_symbol_completion_matches Tom Tromey 2018-05-19 16:06 ` [RFA 3/3] Change ada_catchpoint::excep_string to be a std::string Tom Tromey @ 2018-05-19 20:37 ` Tom Tromey 2018-05-21 0:06 ` Simon Marchi 2 siblings, 1 reply; 11+ messages in thread From: Tom Tromey @ 2018-05-19 20:37 UTC (permalink / raw) To: gdb-patches; +Cc: Tom Tromey This removes a cleanup from ada-lang.c by having ada_exception_message_1 return a unique_xmalloc_ptr. gdb/ChangeLog 2018-05-18 Tom Tromey <tom@tromey.com> * ada-lang.c (ada_exception_message_1, ada_exception_message): Return unique_xmalloc_ptr. (print_it_exception): Update. --- gdb/ChangeLog | 6 ++++++ gdb/ada-lang.c | 29 +++++++++-------------------- 2 files changed, 15 insertions(+), 20 deletions(-) diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c index 7cbf1ec08d..da5f75030c 100644 --- a/gdb/ada-lang.c +++ b/gdb/ada-lang.c @@ -12342,8 +12342,6 @@ ada_exception_name_addr_1 (enum ada_exception_catchpoint_kind ex, return the message which was associated to the exception, if available. Return NULL if the message could not be retrieved. - The caller must xfree the string after use. - Note: The exception message can be associated to an exception either through the use of the Raise_Exception function, or more simply (Ada 2005 and later), via: @@ -12352,13 +12350,11 @@ ada_exception_name_addr_1 (enum ada_exception_catchpoint_kind ex, */ -static char * +static gdb::unique_xmalloc_ptr<char> ada_exception_message_1 (void) { struct value *e_msg_val; - char *e_msg = NULL; int e_msg_len; - struct cleanup *cleanups; /* For runtimes that support this feature, the exception message is passed as an unbounded string argument called "message". */ @@ -12375,22 +12371,20 @@ ada_exception_message_1 (void) if (e_msg_len <= 0) return NULL; - e_msg = (char *) xmalloc (e_msg_len + 1); - cleanups = make_cleanup (xfree, e_msg); - read_memory_string (value_address (e_msg_val), e_msg, e_msg_len + 1); - e_msg[e_msg_len] = '\0'; + gdb::unique_xmalloc_ptr<char> e_msg ((char *) xmalloc (e_msg_len + 1)); + read_memory_string (value_address (e_msg_val), e_msg.get (), e_msg_len + 1); + e_msg.get ()[e_msg_len] = '\0'; - discard_cleanups (cleanups); return e_msg; } /* Same as ada_exception_message_1, except that all exceptions are contained here (returning NULL instead). */ -static char * +static gdb::unique_xmalloc_ptr<char> ada_exception_message (void) { - char *e_msg = NULL; /* Avoid a spurious uninitialized warning. */ + gdb::unique_xmalloc_ptr<char> e_msg; TRY { @@ -12398,7 +12392,7 @@ ada_exception_message (void) } CATCH (e, RETURN_MASK_ERROR) { - e_msg = NULL; + e_msg.reset (nullptr); } END_CATCH @@ -12639,7 +12633,6 @@ print_it_exception (enum ada_exception_catchpoint_kind ex, bpstat bs) { struct ui_out *uiout = current_uiout; struct breakpoint *b = bs->breakpoint_at; - char *exception_message; annotate_catchpoint (b->number); @@ -12707,16 +12700,12 @@ print_it_exception (enum ada_exception_catchpoint_kind ex, bpstat bs) break; } - exception_message = ada_exception_message (); + gdb::unique_xmalloc_ptr<char> exception_message = ada_exception_message (); if (exception_message != NULL) { - struct cleanup *cleanups = make_cleanup (xfree, exception_message); - uiout->text (" ("); - uiout->field_string ("exception-message", exception_message); + uiout->field_string ("exception-message", exception_message.get ()); uiout->text (")"); - - do_cleanups (cleanups); } uiout->text (" at "); -- 2.13.6 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFA 1/3] Remove cleanup from ada-lang.c 2018-05-19 20:37 ` [RFA 1/3] Remove cleanup from ada-lang.c Tom Tromey @ 2018-05-21 0:06 ` Simon Marchi 0 siblings, 0 replies; 11+ messages in thread From: Simon Marchi @ 2018-05-21 0:06 UTC (permalink / raw) To: Tom Tromey, gdb-patches On 2018-05-19 12:06 PM, Tom Tromey wrote: > This removes a cleanup from ada-lang.c by having > ada_exception_message_1 return a unique_xmalloc_ptr. LGTM. Simon ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-05-22 14:58 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-19 20:37 [RFA 0/3] More Ada cleanup removal Tom Tromey 2018-05-19 16:06 ` [RFA 2/3] Remove cleanup from ada_collect_symbol_completion_matches Tom Tromey 2018-05-21 0:09 ` Simon Marchi 2018-05-19 16:06 ` [RFA 3/3] Change ada_catchpoint::excep_string to be a std::string Tom Tromey 2018-05-21 1:12 ` Simon Marchi 2018-05-22 14:49 ` [pushed+RFC] C++ exception during command triggers stale cleanup internal-warning Joel Brobecker 2018-05-22 14:50 ` [pushed/Ada] fix "stale cleanup" internal-warning when using "catch assert" command Joel Brobecker 2018-05-22 14:58 ` [pushed+RFC] C++ exception during command triggers stale cleanup internal-warning Tom Tromey 2018-05-22 15:48 ` Joel Brobecker 2018-05-19 20:37 ` [RFA 1/3] Remove cleanup from ada-lang.c Tom Tromey 2018-05-21 0:06 ` Simon Marchi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).