Re: [PATCH] In MI mode -var-create --language not working.

[Pedro Alves <palves at redhat dot 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