This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
RE: [PATCH] In MI mode -var-create --language not working.
- From: "Tedeschi, Walfred" <walfred dot tedeschi at intel dot com>
- To: Pedro Alves <palves at redhat dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Wed, 12 Nov 2014 15:09:24 +0000
- Subject: RE: [PATCH] In MI mode -var-create --language not working.
- Authentication-results: sourceware.org; auth=none
- References: <1412062176-23484-1-git-send-email-walfred dot tedeschi at intel dot com> <546376A7 dot 8090101 at redhat dot com>
Hi Pedro,
Thanks a lot for your review!!!
I will address all the comments and send to another round.
Thanks and regards,
-Fred
-----Original Message-----
From: Pedro Alves [mailto:palves@redhat.com]
Sent: Wednesday, November 12, 2014 4:03 PM
To: Tedeschi, Walfred; mark.kettenis@xs4all.nl
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCH] In MI mode -var-create --language not working.
Hi Walfred,
On 09/30/2014 08:29 AM, Walfred Tedeschi wrote:
> Trying to use --language to create a variable showed that --language
> was not always working. In some cases GDB understands that the
> language is automatic and cannot parse the language set while executing the command.
> In order to fix this just before executing the command language mode
> should be set to manual. In this way GDB can parse the expression
> using the language given in the command.
> To do so mode has temporarily to be changed to manual expressing that
> any evaluation done using the language parameter has priority over the
> automatic one.
> Tests are also included doing evaluations when the default language is
> c/c++ and when default language is Fortran.
>
> 2014-07-25 Walfred Tedeschi <walfred.tedeschi@intel.com>
>
> * utils.c (saved_language_and_mode): new struct to accommodate
> the language mode and the current language for cleanup.
> (do_restore_current_language): Add the language and mode
> to be restored. (make_cleanup_restore_current_language):
> pass the mode and language to the do_restore_current_language.
Start sentences with uppercase. Don't explain what the struct is for here. Line break before "(make_...". "to the do_restore_current_language"
sounds a little strange. I suggest this:
* utils.c (struct saved_language_and_mode): New.
(do_restore_current_language): Add the language and mode
to be restored.
(make_cleanup_restore_current_language): Pass the mode and
language to do_restore_current_language.
>
> gdb/mi
> * mi-main.c (mi_cmd_execute): Set to manual the language mode
> to execute an mi command when --language is present.
* mi-main.c (mi_cmd_execute): When --language is present, set
the language mode to manual.
> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c index
> 59717ca..0198981 100644
> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c
> @@ -2236,6 +2236,7 @@ mi_cmd_execute (struct mi_parse *parse)
> if (parse->language != language_unknown)
> {
> make_cleanup_restore_current_language ();
> + language_mode = language_mode_manual;
> set_language (parse->language);
> }
>
Did you audit all set_language callers, to see what other callers might need the same fix? It might be better to add a variant of set_language that does the forcing of language_mode to manual. Say, something like set_language_manual, or override_language, or some such, and call that in all places that might need this.
> +set lineno [gdb_get_line_number "only bp."]
> +
> +mi_create_breakpoint "$srcfile:$lineno" "add mi-language1 bp" keep {main\(\)} \
> + ".*mi-language1.cc" $lineno $hex "break in main"
Use $srcfile instead of "mi-language1.cc". "break in main" seems stale?
> +
> +mi_execute_to "exec-continue" "breakpoint-hit" \
> + "main" "" ".*" ".*" {"" "disp=\"keep\""} \
> + "continue to mi-language1 bp"
No need to say "mi-language1" here. The test messages are always prefixed with the test file name.
> +
> +gdb_exit
> \ No newline at end of file
Please make sure new files end with a new line.
> diff --git a/gdb/testsuite/gdb.mi/mi-language2.exp
> b/gdb/testsuite/gdb.mi/mi-language2.exp
How about renaming these to mi-language-c.exp, mi-language-fortran.exp ?
mi-language1.exp and mi-language2.exp seem a bit arbitrary.
> +set bp_lineno [gdb_get_line_number "only bp"]
> +
> +mi_create_breakpoint "-t mi-language2.f90:$bp_lineno" "add
> +mi-language2 bp"\
> + "del" "struct_test" ".*mi-language2.f90" $bp_lineno $hex \
> + "MI-language2 insert breakpoint at line $bp_lineno (only bp)"
> +
No need for this "MI-language2" prefix. Please don't put line numbers in the test message, as those will change when we add something to the test source. Write instead:
"insert breakpoint at only bp"
or some such.
> +mi_run_cmd
> +
> +mi_expect_stop "breakpoint-hit" "struct_test" "" ".*mi-language2.f90" \
> + "$bp_lineno" { "" "disp=\"del\"" } "mi-language2 run to breakpoint at line $bp_lineno"
> +
> +mi_gdb_test "-var-create --language c alpha_1 * (p.c)" \
> + "\\^done,name=\"alpha_1\",numchild=\"0\",value=\"1\",type=\"integer\\(kind=4\\)\",thread-id=\"\[0-9\]+\",has_more=\"0\"" \
> + "-var-create from fortran using fortran language"
> +
> +mi_gdb_test "-var-create alpha_2 * (p%c)" \
> + "\\^done,name=\"alpha_2\",numchild=\"0\",value=\"1\",type=\"integer\\(kind=4\\)\",thread-id=\"\[0-9\]+\",has_more=\"0\"" \
> + "-var-create from fortran using default language"
> +gdb_exit
> \ No newline at end of file
Add a newline.
> diff --git a/gdb/testsuite/gdb.mi/mi-language2.f90
> b/gdb/testsuite/gdb.mi/mi-language2.f90
> new file mode 100644
> index 0000000..ec49f2b
> --- /dev/null
> +++ b/gdb/testsuite/gdb.mi/mi-language2.f90
> @@ -0,0 +1,40 @@
> +! Copyright 2014 Free Software Foundation, Inc.
...
> +!
> +! Ihis file is the Fortran source file for derived-type.exp. It was
> +written ! by Wu Zhou. (woodzltc@cn.ibm.com)
This reference to derived-type.exp is confusing now. I think it's best to just remove the whole paragraph. Also, if this file is mostly copied from another, please preserve the copyright years of the original file.
> diff --git a/gdb/utils.c b/gdb/utils.c index 4da9501..96a87ca 100644
> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -444,24 +444,38 @@ make_cleanup_free_so (struct so_list *so)
>
> /* Helper for make_cleanup_restore_current_language. */
>
> +struct saved_language_and_mode
> +{
> + enum language lang;
> + enum language_mode mode;
> +};
> +
> static void
> do_restore_current_language (void *p) {
> - enum language saved_lang = (uintptr_t) p;
> + struct saved_language_and_mode *lang_and_mode =
> + (struct saved_language_and_mode*) p;
struct saved_language_and_mode *lang_and_mode
= (struct saved_language_and_mode*) p;
Though in C you don't really need the cast.
> + enum language saved_lang = lang_and_mode->lang; language_mode =
> + lang_and_mode->mode;
>
> + xfree (p);
This xfree should be done in the cleanup's destructor, so that we don't leak p when discard_cleanups is called. That is, use make_cleanup_dtor instead of make_cleanup in make_cleanup_restore_current_language.
> set_language (saved_lang);
> }
>
> -/* Remember the current value of CURRENT_LANGUAGE and make it restored when
> - the cleanup is run. */
> +/* Remember the current value of CURRENT_LANGUAGE and
> + LANGUAGE_MODE restoring them when the cleanup is run. */
>
> struct cleanup *
> make_cleanup_restore_current_language (void) {
> - enum language saved_lang = current_language->la_language;
> + struct saved_language_and_mode *lang_and_mode
> + = XNEW (struct saved_language_and_mode);
Indentation:
struct saved_language_and_mode *lang_and_mode
= XNEW (struct saved_language_and_mode);
Thanks,
Pedro Alves
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052