From patchwork Thu Oct 20 12:13:13 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 16691 Received: (qmail 44296 invoked by alias); 20 Oct 2016 12:13:19 -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 44281 invoked by uid 89); 20 Oct 2016 12:13:18 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=2.5 required=5.0 tests=BAYES_50, RP_MATCHES_RCVD, SPF_HELO_PASS, UNSUBSCRIBE_BODY autolearn=no version=3.3.2 spammy=dc, d.c, 17, 8, Storage 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; Thu, 20 Oct 2016 12:13:16 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 889DC624A2; Thu, 20 Oct 2016 12:13:15 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u9KCDDlC001381; Thu, 20 Oct 2016 08:13:14 -0400 Subject: Re: [PATCH v2 21/31] Use ui_file_as_string in gdb/compile/ To: Simon Marchi References: <1476839539-8374-1-git-send-email-palves@redhat.com> <1476839539-8374-22-git-send-email-palves@redhat.com> <8a267925d9af7f5b01c75a81da15b664@simark.ca> <7a6e4bde-9ca2-4163-bdd7-0fd1f88a516b@redhat.com> <616c3e9e394bd7b449cce98204fc2c50@simark.ca> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: <40d72683-7915-66b7-b17b-edb2d25ac253@redhat.com> Date: Thu, 20 Oct 2016 13:13:13 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <616c3e9e394bd7b449cce98204fc2c50@simark.ca> On 10/20/2016 04:17 AM, Simon Marchi wrote: > On 2016-10-19 19:48, Pedro Alves wrote: >> Well, there's std::pair in C++03, and std::tuple in C++11. >> And C++17 has structured bindings: >> >> auto [source_file, object_file] = compile_to_object (); >> >> :-) >> >> But I think returning a real named struct is clearer than all that. >> See patch below showing the idea. WDYT? > > Well, your last example looks really nice, since it avoids the overhead > of writing a class just for one return value, but since we don't have > access to C++17 quite yet, a class will have to do. A very simple struct with public fields like this: struct compile_file_names { std::string source_file; std::string objfile_file; }; would have worked too. But I wrote it as a full blown class with a couple accessors that return const char *, because I thought that while at it, that would avoid adding several str.c_str() calls where we're actually using the file names. Looking a bit deeper, I think that we can make use of the new class a bit more, and it ends up clarifying the code further. See new version below. Note the point about the ordering. > >>> >>> Btw, would it be good to use references for those strings? >> >> Maybe. I had kept it using pointers to make it clearer at the >> call site that these are output parameters. Maybe that's >> old-school C++ mentality? That moot with this version of the >> patch though, so I can dodge discussing that for now. :-) > > I understand, I don't know which one is best. What's nice with > references is that you can't call it with something wrong. > > The patch looks good to me... just one missing space... > >> -static void >> -get_new_file_names (char **source_file, char **object_file) >> +class compile_file_names >> +{ >> +public: >> + compile_file_names(std::string source_file, std::string object_file) > > ...here? Fixed now. Also added comments and updated ChangeLog this time. WDYT? From 0e7a586e53f91159c49c81f82068dba6c6be6d51 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Thu, 20 Oct 2016 12:50:10 +0100 Subject: [PATCH] Use ui_file_as_string in gdb/compile/ Using ui_file_as_string would imply changing a few prototypes to pass around source and object file names as std::string. Instead of that, wrap those two in a new class. This ends up eliminating a small wrinkle: get_new_file_names and compile_object_load have swapped parameters. The former takes "source, objfile", while the latter takes "objfile, source". gdb/ChangeLog: yyyy-mm-yy Pedro Alves * c-lang.h (c_compute_program): Now returns std::string. * compile/compile-internal.h (class compile_file_names): New class. * compile/compile-object-load.c (compile_object_load): Replace object_file and source_file parameters with a compile_file_names parameter. Adjust. * compile-object-load.h: Include "compile-internal.h". (compile_object_load): Replace object_file and source_file parameters with a compile_file_names parameter. * compile/compile-c-support.c (c_compute_program): Now returns a std::string. Use ui_file_as_string. * compile/compile.c (get_new_file_names): Remove parameters and return a compile_file_names instead. (compile_to_object): Now returns a compile_file_names. Use ui_file_as_string. (eval_compile_command): Use compile_file_names. * language.h (struct language_defn) : Now returns std::string. --- gdb/c-lang.h | 14 ++--- gdb/compile/compile-c-support.c | 6 +-- gdb/compile/compile-internal.h | 25 +++++++++ gdb/compile/compile-object-load.c | 18 +++---- gdb/compile/compile-object-load.h | 4 +- gdb/compile/compile.c | 104 +++++++++++++++++--------------------- gdb/language.h | 14 ++--- 7 files changed, 99 insertions(+), 86 deletions(-) diff --git a/gdb/c-lang.h b/gdb/c-lang.h index 12be8bf..8378d4f 100644 --- a/gdb/c-lang.h +++ b/gdb/c-lang.h @@ -152,16 +152,16 @@ extern int c_textual_element_type (struct type *, char); extern struct compile_instance *c_get_compile_context (void); -/* This takes the user-supplied text and returns a newly malloc'd bit - of code to compile. +/* This takes the user-supplied text and returns a new bit of code to + compile. This is used as the la_compute_program language method; see that for a description of the arguments. */ -extern char *c_compute_program (struct compile_instance *inst, - const char *input, - struct gdbarch *gdbarch, - const struct block *expr_block, - CORE_ADDR expr_pc); +extern std::string c_compute_program (struct compile_instance *inst, + const char *input, + struct gdbarch *gdbarch, + const struct block *expr_block, + CORE_ADDR expr_pc); #endif /* !defined (C_LANG_H) */ diff --git a/gdb/compile/compile-c-support.c b/gdb/compile/compile-c-support.c index c42daba..641b0fd 100644 --- a/gdb/compile/compile-c-support.c +++ b/gdb/compile/compile-c-support.c @@ -326,7 +326,7 @@ generate_register_struct (struct ui_file *stream, struct gdbarch *gdbarch, to the inferior when the expression was created, and EXPR_PC indicates the value of $PC. */ -char * +std::string c_compute_program (struct compile_instance *inst, const char *input, struct gdbarch *gdbarch, @@ -334,7 +334,7 @@ c_compute_program (struct compile_instance *inst, CORE_ADDR expr_pc) { struct ui_file *buf, *var_stream = NULL; - char *code; + std::string code; struct cleanup *cleanup; struct compile_c_instance *context = (struct compile_c_instance *) inst; @@ -435,7 +435,7 @@ c_compute_program (struct compile_instance *inst, fputs_unfiltered ("}\n", buf); add_code_footer (inst->scope, buf); - code = ui_file_xstrdup (buf, NULL); + code = ui_file_as_string (buf); do_cleanups (cleanup); return code; } diff --git a/gdb/compile/compile-internal.h b/gdb/compile/compile-internal.h index 66ea864..5a7bf14 100644 --- a/gdb/compile/compile-internal.h +++ b/gdb/compile/compile-internal.h @@ -152,4 +152,29 @@ extern const char *c_get_mode_for_size (int size); struct dynamic_prop; extern char *c_get_range_decl_name (const struct dynamic_prop *prop); +/* Type used to hold and pass around the source and object file names + to use for compilation. */ +class compile_file_names +{ +public: + compile_file_names (std::string source_file, std::string object_file) + : m_source_file (source_file), m_object_file (object_file) + {} + + /* Provide read-only views only. Return 'const char *' instead of + std::string to avoid having to use c_str() everywhere in client + code. */ + + const char *source_file () const + { return m_source_file.c_str (); } + + const char *object_file () const + { return m_object_file.c_str (); } + +private: + /* Storage for the file names. */ + std::string m_source_file; + std::string m_object_file; +}; + #endif /* GDB_COMPILE_INTERNAL_H */ diff --git a/gdb/compile/compile-object-load.c b/gdb/compile/compile-object-load.c index 9fc6c02..c08aa2b 100644 --- a/gdb/compile/compile-object-load.c +++ b/gdb/compile/compile-object-load.c @@ -601,16 +601,14 @@ store_regs (struct type *regs_type, CORE_ADDR regs_base) } } -/* Load OBJECT_FILE into inferior memory. Throw an error otherwise. - Caller must fully dispose the return value by calling compile_object_run. - SOURCE_FILE's copy is stored into the returned object. - Caller should free both OBJECT_FILE and SOURCE_FILE immediatelly after this - function returns. - Function returns NULL only for COMPILE_I_PRINT_ADDRESS_SCOPE when - COMPILE_I_PRINT_VALUE_SCOPE should have been used instead. */ +/* Load the object file specified in FILE_NAMES into inferior memory. + Throw an error otherwise. Caller must fully dispose the return + value by calling compile_object_run. Returns NULL only for + COMPILE_I_PRINT_ADDRESS_SCOPE when COMPILE_I_PRINT_VALUE_SCOPE + should have been used instead. */ struct compile_module * -compile_object_load (const char *object_file, const char *source_file, +compile_object_load (const compile_file_names &file_names, enum compile_i_scope_types scope, void *scope_data) { struct cleanup *cleanups, *cleanups_free_objfile; @@ -633,7 +631,7 @@ compile_object_load (const char *object_file, const char *source_file, struct type *expect_return_type; struct munmap_list *munmap_list_head = NULL; - filename = tilde_expand (object_file); + filename = tilde_expand (file_names.object_file ()); cleanups = make_cleanup (xfree, filename); abfd = gdb_bfd_open (filename, gnutarget, -1); @@ -824,7 +822,7 @@ compile_object_load (const char *object_file, const char *source_file, retval = XNEW (struct compile_module); retval->objfile = objfile; - retval->source_file = xstrdup (source_file); + retval->source_file = xstrdup (file_names.source_file ()); retval->func_sym = func_sym; retval->regs_addr = regs_addr; retval->scope = scope; diff --git a/gdb/compile/compile-object-load.h b/gdb/compile/compile-object-load.h index 39c1a90..b9cf307 100644 --- a/gdb/compile/compile-object-load.h +++ b/gdb/compile/compile-object-load.h @@ -17,6 +17,8 @@ #ifndef GDB_COMPILE_OBJECT_LOAD_H #define GDB_COMPILE_OBJECT_LOAD_H +#include "compile-internal.h" + struct munmap_list; struct compile_module @@ -53,7 +55,7 @@ struct compile_module }; extern struct compile_module *compile_object_load - (const char *object_file, const char *source_file, + (const compile_file_names &fnames, enum compile_i_scope_types scope, void *scope_data); extern void munmap_list_free (struct munmap_list *head); diff --git a/gdb/compile/compile.c b/gdb/compile/compile.c index 0c4a738..dd854d8 100644 --- a/gdb/compile/compile.c +++ b/gdb/compile/compile.c @@ -255,18 +255,18 @@ get_compile_file_tempdir (void) return tempdir_name; } -/* Compute the names of source and object files to use. The names are - allocated by malloc and should be freed by the caller. */ +/* Compute the names of source and object files to use. */ -static void -get_new_file_names (char **source_file, char **object_file) +static compile_file_names +get_new_file_names () { static int seq; const char *dir = get_compile_file_tempdir (); ++seq; - *source_file = xstrprintf ("%s%sout%d.c", dir, SLASH_STRING, seq); - *object_file = xstrprintf ("%s%sout%d.o", dir, SLASH_STRING, seq); + + return compile_file_names (string_printf ("%s%sout%d.c", dir, SLASH_STRING, seq), + string_printf ("%s%sout%d.o", dir, SLASH_STRING, seq)); } /* Get the block and PC at which to evaluate an expression. */ @@ -456,18 +456,13 @@ print_callback (void *ignore, const char *message) } /* Process the compilation request. On success it returns the object - file name and *SOURCE_FILEP is set to source file name. On an - error condition, error () is called. The caller is responsible for - freeing both strings. */ + and source file names. On an error condition, error () is + called. */ -static char * +static compile_file_names compile_to_object (struct command_line *cmd, const char *cmd_string, - enum compile_i_scope_types scope, - char **source_filep) + enum compile_i_scope_types scope) { - char *code; - const char *input; - char *source_file, *object_file; struct compile_instance *compiler; struct cleanup *cleanup, *inner_cleanup; const struct block *expr_block; @@ -503,11 +498,14 @@ compile_to_object (struct command_line *cmd, const char *cmd_string, /* From the provided expression, build a scope to pass to the compiler. */ + + std::string input_buf; + const char *input; + if (cmd != NULL) { struct ui_file *stream = mem_fileopen (); struct command_line *iter; - char *stream_buf; make_cleanup_ui_file_delete (stream); for (iter = cmd->body_list[0]; iter; iter = iter->next) @@ -516,20 +514,19 @@ compile_to_object (struct command_line *cmd, const char *cmd_string, fputs_unfiltered ("\n", stream); } - stream_buf = ui_file_xstrdup (stream, NULL); - make_cleanup (xfree, stream_buf); - input = stream_buf; + input_buf = ui_file_as_string (stream); + input = input_buf.c_str (); } else if (cmd_string != NULL) input = cmd_string; else error (_("Neither a simple expression, or a multi-line specified.")); - code = current_language->la_compute_program (compiler, input, gdbarch, - expr_block, expr_pc); - make_cleanup (xfree, code); + std::string code + = current_language->la_compute_program (compiler, input, gdbarch, + expr_block, expr_pc); if (compile_debug) - fprintf_unfiltered (gdb_stdlog, "debug output:\n\n%s", code); + fprintf_unfiltered (gdb_stdlog, "debug output:\n\n%s", code.c_str ()); os_rx = osabi_triplet_regexp (gdbarch_osabi (gdbarch)); arch_rx = gdbarch_gnu_triplet_regexp (gdbarch); @@ -560,37 +557,36 @@ compile_to_object (struct command_line *cmd, const char *cmd_string, argi, argv[argi]); } - get_new_file_names (&source_file, &object_file); - inner_cleanup = make_cleanup (xfree, source_file); - make_cleanup (xfree, object_file); + compile_file_names fnames = get_new_file_names (); - src = gdb_fopen_cloexec (source_file, "w"); + src = gdb_fopen_cloexec (fnames.source_file (), "w"); if (src == NULL) perror_with_name (_("Could not open source file for writing")); - make_cleanup (cleanup_unlink_file, source_file); - if (fputs (code, src) == EOF) + inner_cleanup = make_cleanup (cleanup_unlink_file, + (void *) fnames.source_file ()); + if (fputs (code.c_str (), src) == EOF) perror_with_name (_("Could not write to source file")); fclose (src); if (compile_debug) fprintf_unfiltered (gdb_stdlog, "source file produced: %s\n\n", - source_file); + fnames.source_file ()); /* Call the compiler and start the compilation process. */ - compiler->fe->ops->set_source_file (compiler->fe, source_file); + compiler->fe->ops->set_source_file (compiler->fe, fnames.source_file ()); - if (!compiler->fe->ops->compile (compiler->fe, object_file, + if (!compiler->fe->ops->compile (compiler->fe, fnames.object_file (), compile_debug)) error (_("Compilation failed.")); if (compile_debug) fprintf_unfiltered (gdb_stdlog, "object file produced: %s\n\n", - object_file); + fnames.object_file ()); discard_cleanups (inner_cleanup); do_cleanups (cleanup); - *source_filep = source_file; - return object_file; + + return fnames; } /* The "compile" prefix command. */ @@ -609,32 +605,24 @@ void eval_compile_command (struct command_line *cmd, const char *cmd_string, enum compile_i_scope_types scope, void *scope_data) { - char *object_file, *source_file; + struct cleanup *cleanup_unlink; + struct compile_module *compile_module; + + compile_file_names fnames = compile_to_object (cmd, cmd_string, scope); - object_file = compile_to_object (cmd, cmd_string, scope, &source_file); - if (object_file != NULL) + cleanup_unlink = make_cleanup (cleanup_unlink_file, + (void *) fnames.object_file ()); + make_cleanup (cleanup_unlink_file, (void *) fnames.source_file ()); + compile_module = compile_object_load (fnames, scope, scope_data); + if (compile_module == NULL) { - struct cleanup *cleanup_xfree, *cleanup_unlink; - struct compile_module *compile_module; - - cleanup_xfree = make_cleanup (xfree, object_file); - make_cleanup (xfree, source_file); - cleanup_unlink = make_cleanup (cleanup_unlink_file, object_file); - make_cleanup (cleanup_unlink_file, source_file); - compile_module = compile_object_load (object_file, source_file, - scope, scope_data); - if (compile_module == NULL) - { - gdb_assert (scope == COMPILE_I_PRINT_ADDRESS_SCOPE); - do_cleanups (cleanup_xfree); - eval_compile_command (cmd, cmd_string, - COMPILE_I_PRINT_VALUE_SCOPE, scope_data); - return; - } - discard_cleanups (cleanup_unlink); - do_cleanups (cleanup_xfree); - compile_object_run (compile_module); + gdb_assert (scope == COMPILE_I_PRINT_ADDRESS_SCOPE); + eval_compile_command (cmd, cmd_string, + COMPILE_I_PRINT_VALUE_SCOPE, scope_data); + return; } + discard_cleanups (cleanup_unlink); + compile_object_run (compile_module); } /* See compile/compile-internal.h. */ diff --git a/gdb/language.h b/gdb/language.h index d6f932e..758f265 100644 --- a/gdb/language.h +++ b/gdb/language.h @@ -406,8 +406,8 @@ struct language_defn If 'la_get_gcc_context' is not defined, then this method is ignored. - This takes the user-supplied text and returns a newly malloc'd - bit of code to compile. The caller owns the result. + This takes the user-supplied text and returns a new bit of code + to compile. INST is the compiler instance being used. INPUT is the user's input text. @@ -416,11 +416,11 @@ struct language_defn parsed. EXPR_PC is the PC at which the expression is being parsed. */ - char *(*la_compute_program) (struct compile_instance *inst, - const char *input, - struct gdbarch *gdbarch, - const struct block *expr_block, - CORE_ADDR expr_pc); + std::string (*la_compute_program) (struct compile_instance *inst, + const char *input, + struct gdbarch *gdbarch, + const struct block *expr_block, + CORE_ADDR expr_pc); /* Add fields above this point, so the magic number is always last. */ /* Magic number for compat checking. */