From: Pedro Alves <palves@redhat.com>
To: Walfred Tedeschi <walfred.tedeschi@intel.com>, mark.kettenis@xs4all.nl
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCH] In MI mode -var-create --language not working.
Date: Wed, 12 Nov 2014 15:03:00 -0000 [thread overview]
Message-ID: <546376A7.8090101@redhat.com> (raw)
In-Reply-To: <1412062176-23484-1-git-send-email-walfred.tedeschi@intel.com>
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
next prev parent reply other threads:[~2014-11-12 15:03 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-09-30 7:36 Walfred Tedeschi
2014-11-12 15:03 ` Pedro Alves [this message]
2014-11-12 15:15 ` Tedeschi, Walfred
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=546376A7.8090101@redhat.com \
--to=palves@redhat.com \
--cc=gdb-patches@sourceware.org \
--cc=mark.kettenis@xs4all.nl \
--cc=walfred.tedeschi@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).