From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 124284 invoked by alias); 27 Nov 2017 14:17:01 -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 122958 invoked by uid 89); 27 Nov 2017 14:17:00 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.7 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KB_WAM_FROM_NAME_SINGLEWORD,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= 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, 27 Nov 2017 14:16:57 +0000 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1BE3F61B8C; Mon, 27 Nov 2017 14:16:56 +0000 (UTC) Received: from localhost (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by smtp.corp.redhat.com (Postfix) with ESMTPS id D858B60BE5; Mon, 27 Nov 2017 14:16:55 +0000 (UTC) From: Sergio Durigan Junior To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [RFA] C++-ify parser_state References: <20171126174047.23943-1-tom@tromey.com> Date: Mon, 27 Nov 2017 14:17:00 -0000 In-Reply-To: <20171126174047.23943-1-tom@tromey.com> (Tom Tromey's message of "Sun, 26 Nov 2017 10:40:47 -0700") Message-ID: <87k1ybst60.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2017-11/txt/msg00686.txt.bz2 On Sunday, November 26 2017, Tom Tromey wrote: > This mildly C++-ifies parser_state and stap_parse_info -- just enough > to remove some cleanups. Thanks for the patch, Tom. This was on my TODO list for a while now. I looked it over and it seems good to me. > Regression tested by the buildbot. > > ChangeLog > 2017-11-26 Tom Tromey > > * stap-probe.h (struct stap_parse_info): Add constructor, > destructor. > * stap-probe.c (stap_parse_argument): Update. > * rust-exp.y (rust_lex_tests): Update. > * parser-defs.h (struct parser_state): Add constructor, > destructor, release method. > (initialize_expout, reallocate_expout): Remove. > * parse.c (parser_state::parser_state): Rename from > initialize_expout. > (parser_state::~parser_state): New. > (parser_state::release): Rename from reallocate_expout. > (parse_exp_in_context_1): Update. > * dtrace-probe.c (dtrace_probe::build_arg_exprs): Update. > --- > gdb/ChangeLog | 16 ++++++++++++++ > gdb/dtrace-probe.c | 11 ++-------- > gdb/parse.c | 63 ++++++++++++++++++++++++++++++------------------------ > gdb/parser-defs.h | 32 +++++++++++++-------------- > gdb/rust-exp.y | 3 +-- > gdb/stap-probe.c | 15 ++----------- > gdb/stap-probe.h | 17 +++++++++++++++ > 7 files changed, 88 insertions(+), 69 deletions(-) > > diff --git a/gdb/dtrace-probe.c b/gdb/dtrace-probe.c > index 1c88f89054..c0134dcac5 100644 > --- a/gdb/dtrace-probe.c > +++ b/gdb/dtrace-probe.c > @@ -626,21 +626,15 @@ dtrace_probe::build_arg_exprs (struct gdbarch *gdbarch) > value of the argument when executed at the PC of the probe. */ > for (dtrace_probe_arg &arg : m_args) > { > - struct cleanup *back_to; > - struct parser_state pstate; > - > /* Initialize the expression buffer in the parser state. The > language does not matter, since we are using our own > parser. */ > - initialize_expout (&pstate, 10, current_language, gdbarch); > - back_to = make_cleanup (free_current_contents, &pstate.expout); > + parser_state pstate (10, current_language, gdbarch); > > /* The argument value, which is ABI dependent and casted to > `long int'. */ > gdbarch_dtrace_parse_probe_argument (gdbarch, &pstate, argc); > > - discard_cleanups (back_to); > - > /* Casting to the expected type, but only if the type was > recognized at probe load time. Otherwise the argument will > be evaluated as the long integer passed to the probe. */ > @@ -651,8 +645,7 @@ dtrace_probe::build_arg_exprs (struct gdbarch *gdbarch) > write_exp_elt_opcode (&pstate, UNOP_CAST); > } > > - reallocate_expout (&pstate); > - arg.expr = expression_up (pstate.expout); > + arg.expr = pstate.release (); > prefixify_expression (arg.expr.get ()); > ++argc; > } > diff --git a/gdb/parse.c b/gdb/parse.c > index 506efa38fb..3a7b7af4e1 100644 > --- a/gdb/parse.c > +++ b/gdb/parse.c > @@ -152,34 +152,43 @@ end_arglist (void) > > /* See definition in parser-defs.h. */ > > -void > -initialize_expout (struct parser_state *ps, size_t initial_size, > - const struct language_defn *lang, > - struct gdbarch *gdbarch) > +parser_state::parser_state (size_t initial_size, > + const struct language_defn *lang, > + struct gdbarch *gdbarch) > + : expout_size (initial_size), > + expout_ptr (0) > { > - ps->expout_size = initial_size; > - ps->expout_ptr = 0; > - ps->expout > + expout > = (struct expression *) xmalloc (sizeof (struct expression) > - + EXP_ELEM_TO_BYTES (ps->expout_size)); > - ps->expout->language_defn = lang; > - ps->expout->gdbarch = gdbarch; > + + EXP_ELEM_TO_BYTES (expout_size)); > + expout->language_defn = lang; > + expout->gdbarch = gdbarch; > } > > /* See definition in parser-defs.h. */ > > -void > -reallocate_expout (struct parser_state *ps) > +parser_state::~parser_state () > +{ > + xfree (expout); > +} > + > +expression_up > +parser_state::release () > { > /* Record the actual number of expression elements, and then > reallocate the expression memory so that we free up any > excess elements. */ > > - ps->expout->nelts = ps->expout_ptr; > - ps->expout = (struct expression *) > - xrealloc (ps->expout, > + expout->nelts = expout_ptr; > + expout = (struct expression *) > + xrealloc (expout, > sizeof (struct expression) > - + EXP_ELEM_TO_BYTES (ps->expout_ptr)); > + + EXP_ELEM_TO_BYTES (expout_ptr)); > + > + expression_up result (expout); > + /* Ensure that we don't free it in the destructor. */ > + expout = nullptr; > + return result; > } > > /* This page contains the functions for adding data to the struct expression > @@ -1118,7 +1127,6 @@ parse_exp_in_context_1 (const char **stringptr, CORE_ADDR pc, > int comma, int void_context_p, int *out_subexp) > { > const struct language_defn *lang = NULL; > - struct parser_state ps; > int subexp; > > lexptr = *stringptr; > @@ -1194,7 +1202,7 @@ parse_exp_in_context_1 (const char **stringptr, CORE_ADDR pc, > and others called from *.y) ensure CURRENT_LANGUAGE gets restored > to the value matching SELECTED_FRAME as set by get_current_arch. */ > > - initialize_expout (&ps, 10, lang, get_current_arch ()); > + parser_state ps (10, lang, get_current_arch ()); > > scoped_restore_current_language lang_saver; > set_language (lang->la_language); > @@ -1207,33 +1215,32 @@ parse_exp_in_context_1 (const char **stringptr, CORE_ADDR pc, > CATCH (except, RETURN_MASK_ALL) > { > if (! parse_completion) > - { > - xfree (ps.expout); > - throw_exception (except); > - } > + throw_exception (except); > } > END_CATCH > > - reallocate_expout (&ps); > + /* We have to operate on an "expression *", due to la_post_parser, > + which explains this funny-looking double release. */ > + struct expression *result = ps.release ().release (); > > /* Convert expression from postfix form as generated by yacc > parser, to a prefix form. */ > > if (expressiondebug) > - dump_raw_expression (ps.expout, gdb_stdlog, > + dump_raw_expression (result, gdb_stdlog, > "before conversion to prefix form"); > > - subexp = prefixify_expression (ps.expout); > + subexp = prefixify_expression (result); > if (out_subexp) > *out_subexp = subexp; > > - lang->la_post_parser (&ps.expout, void_context_p); > + lang->la_post_parser (&result, void_context_p); > > if (expressiondebug) > - dump_prefix_expression (ps.expout, gdb_stdlog); > + dump_prefix_expression (result, gdb_stdlog); > > *stringptr = lexptr; > - return expression_up (ps.expout); > + return expression_up (result); > } > > /* Parse STRING as an expression, and complain if this fails > diff --git a/gdb/parser-defs.h b/gdb/parser-defs.h > index f43fb75df2..262b81b056 100644 > --- a/gdb/parser-defs.h > +++ b/gdb/parser-defs.h > @@ -37,6 +37,21 @@ extern int parser_debug; > > struct parser_state > { > + /* Constructor. INITIAL_SIZE is the initial size of the expout > + array. LANG is the language used to parse the expression. And > + GDBARCH is the gdbarch to use during parsing. */ > + > + parser_state (size_t initial_size, const struct language_defn *lang, > + struct gdbarch *gdbarch); > + ~parser_state (); > + > + DISABLE_COPY_AND_ASSIGN (parser_state); > + > + /* Resize the allocated expression to the correct size, and return > + it as an expression_up -- passing ownership to the caller. */ > + expression_up release (); > + > + > /* The expression related to this parser state. */ > > struct expression *expout; > @@ -156,23 +171,6 @@ struct type_stack > int size; > }; > > -/* Helper function to initialize the expout, expout_size, expout_ptr > - trio inside PS before it is used to store expression elements created > - during the parsing of an expression. INITIAL_SIZE is the initial size of > - the expout array. LANG is the language used to parse the expression. > - And GDBARCH is the gdbarch to use during parsing. */ > - > -extern void initialize_expout (struct parser_state *ps, > - size_t initial_size, > - const struct language_defn *lang, > - struct gdbarch *gdbarch); > - > -/* Helper function that reallocates the EXPOUT inside PS in order to > - eliminate any unused space. It is generally used when the expression > - has just been parsed and created. */ > - > -extern void reallocate_expout (struct parser_state *ps); > - > /* Reverse an expression from suffix form (in which it is constructed) > to prefix form (in which we can conveniently print or execute it). > Ordinarily this always returns -1. However, if EXPOUT_LAST_STRUCT > diff --git a/gdb/rust-exp.y b/gdb/rust-exp.y > index e372a6ea1e..8f1455bbf0 100644 > --- a/gdb/rust-exp.y > +++ b/gdb/rust-exp.y > @@ -2670,8 +2670,7 @@ rust_lex_tests (void) > &test_obstack); > > // Set up dummy "parser", so that rust_type works. > - struct parser_state ps; > - initialize_expout (&ps, 0, &rust_language_defn, target_gdbarch ()); > + struct parser_state ps (0, &rust_language_defn, target_gdbarch ()); > rust_parser parser (&ps); > > rust_lex_test_one ("", 0); > diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c > index 9f4e00845a..d8a67c750a 100644 > --- a/gdb/stap-probe.c > +++ b/gdb/stap-probe.c > @@ -1146,25 +1146,17 @@ static expression_up > stap_parse_argument (const char **arg, struct type *atype, > struct gdbarch *gdbarch) > { > - struct stap_parse_info p; > - struct cleanup *back_to; > - > /* We need to initialize the expression buffer, in order to begin > our parsing efforts. We use language_c here because we may need > to do pointer arithmetics. */ > - initialize_expout (&p.pstate, 10, language_def (language_c), gdbarch); > - back_to = make_cleanup (free_current_contents, &p.pstate.expout); > + struct stap_parse_info p (10, language_def (language_c), gdbarch); > > p.saved_arg = *arg; > p.arg = *arg; > p.arg_type = atype; > - p.gdbarch = gdbarch; > - p.inside_paren_p = 0; > > stap_parse_argument_1 (&p, 0, STAP_OPERAND_PREC_NONE); > > - discard_cleanups (back_to); > - > gdb_assert (p.inside_paren_p == 0); > > /* Casting the final expression to the appropriate type. */ > @@ -1172,13 +1164,10 @@ stap_parse_argument (const char **arg, struct type *atype, > write_exp_elt_type (&p.pstate, atype); > write_exp_elt_opcode (&p.pstate, UNOP_CAST); > > - reallocate_expout (&p.pstate); > - > p.arg = skip_spaces (p.arg); > *arg = p.arg; > > - /* We can safely return EXPOUT here. */ > - return expression_up (p.pstate.expout); > + return p.pstate.release (); > } > > /* Implementation of 'parse_arguments' method. */ > diff --git a/gdb/stap-probe.h b/gdb/stap-probe.h > index c3327e6999..af24a380e0 100644 > --- a/gdb/stap-probe.h > +++ b/gdb/stap-probe.h > @@ -28,6 +28,23 @@ > > struct stap_parse_info > { > + /* Constructor. Arguments are passed to the parser_state > + constructor. */ > + stap_parse_info (size_t initial_size, const struct language_defn *lang, > + struct gdbarch *gdbarch) > + : arg (nullptr), > + pstate (initial_size, lang, gdbarch), > + saved_arg (nullptr), > + arg_type (nullptr), > + gdbarch (gdbarch), > + inside_paren_p (0) > + { > + } > + > + ~stap_parse_info () = default; > + > + DISABLE_COPY_AND_ASSIGN (stap_parse_info); > + > /* The probe's argument in a string format. */ > const char *arg; > > -- > 2.13.6 -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/