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

["Tedeschi, Walfred" <walfred dot tedeschi at intel 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