[RFC] Fix for gdb.parameter('architecture') returning empty string

[RFC] Fix for gdb.parameter('architecture') returning empty string

[Siva Chandra]

[-- Attachment #1: Type: text/plain, Size: 1481 bytes --]

Hello,

Currently, gdb.parameter ('architecture') and gdb.parameter ('endian')
return an empty string.  Attached is a patch which fixes this.  I am
not very sure if this is the cleanest fix, but I could not come up
with a better one.

2012-10-13  Siva Chandra Reddy  <sivachandra@google.com>

        * arch-utils.c: Rename variables 'set_endian_string' and
        'set_architecture_string' to 'endian_string' and
        'architecture_string' respectively.
        (show_endian): Use 'endian_string' instead of evaluating the
        endian string value from the current architecture.
        (show_architecture): Use 'architecture_string' instead of
        evaluating the architecture name from the current architecture.
        (set_endiani, _initialize_gdbarch_utils): Use the new
        'endian_string' variable.
        (selected_architecture_name, set_architecture): Use the new
        'architecture_string' variable.
        (set_endian_string): New convenience function to set
        'endian_string'.
        (on_architecture_change): Callback for the architecture change
        event.
        (initialize_current_architecture): Initialize
        'architecture_string' and 'endian_string'. Attach the callback
        on_architecture_change to the architecture change observer.

testsuite/

2012-10-13  Siva Chandra Reddy  <sivachandra@google.com>

        gdb.python/py-parameter.exp: Add a new test to test
        gdb.parameter ('endian').

Thanks,
Siva Chandra

[-- Attachment #2: param_patch_v1.txt --]
[-- Type: text/plain, Size: 6986 bytes --]

Index: arch-utils.c
===================================================================
RCS file: /cvs/src/src/gdb/arch-utils.c,v
retrieving revision 1.204
diff -u -p -r1.204 arch-utils.c
--- arch-utils.c	8 Jun 2012 14:24:56 -0000	1.204
+++ arch-utils.c	12 Oct 2012 18:25:33 -0000
@@ -32,6 +32,7 @@
 #include "target-descriptions.h"
 #include "objfiles.h"
 #include "language.h"
+#include "observer.h"
 
 #include "version.h"
 
@@ -259,7 +260,7 @@ static const char *const endian_enum[] =
   endian_auto,
   NULL,
 };
-static const char *set_endian_string;
+static const char *endian_string;
 
 enum bfd_endian
 selected_byte_order (void)
@@ -274,19 +275,13 @@ show_endian (struct ui_file *file, int f
 	     const char *value)
 {
   if (target_byte_order_user == BFD_ENDIAN_UNKNOWN)
-    if (gdbarch_byte_order (get_current_arch ()) == BFD_ENDIAN_BIG)
       fprintf_unfiltered (file, _("The target endianness is set automatically "
-				  "(currently big endian)\n"));
-    else
-      fprintf_unfiltered (file, _("The target endianness is set automatically "
-				  "(currently little endian)\n"));
+				  "(currently %s endian)\n"),
+                          endian_string);
   else
-    if (target_byte_order_user == BFD_ENDIAN_BIG)
-      fprintf_unfiltered (file,
-			  _("The target is assumed to be big endian\n"));
-    else
       fprintf_unfiltered (file,
-			  _("The target is assumed to be little endian\n"));
+			  _("The target is assumed to be %s endian\n"),
+                          endian_string);
 }
 
 static void
@@ -296,14 +291,14 @@ set_endian (char *ignore_args, int from_
 
   gdbarch_info_init (&info);
 
-  if (set_endian_string == endian_auto)
+  if (endian_string == endian_auto)
     {
       target_byte_order_user = BFD_ENDIAN_UNKNOWN;
       if (! gdbarch_update_p (info))
 	internal_error (__FILE__, __LINE__,
 			_("set_endian: architecture update failed"));
     }
-  else if (set_endian_string == endian_little)
+  else if (endian_string == endian_little)
     {
       info.byte_order = BFD_ENDIAN_LITTLE;
       if (! gdbarch_update_p (info))
@@ -311,7 +306,7 @@ set_endian (char *ignore_args, int from_
       else
 	target_byte_order_user = BFD_ENDIAN_LITTLE;
     }
-  else if (set_endian_string == endian_big)
+  else if (endian_string == endian_big)
     {
       info.byte_order = BFD_ENDIAN_BIG;
       if (! gdbarch_update_p (info))
@@ -416,7 +411,7 @@ enum set_arch { set_arch_auto, set_arch_
 
 static const struct bfd_arch_info *target_architecture_user;
 
-static const char *set_architecture_string;
+static const char *architecture_string;
 
 const char *
 selected_architecture_name (void)
@@ -424,7 +419,7 @@ selected_architecture_name (void)
   if (target_architecture_user == NULL)
     return NULL;
   else
-    return set_architecture_string;
+    return architecture_string;
 }
 
 /* Called if the user enters ``show architecture'' without an
@@ -437,10 +432,10 @@ show_architecture (struct ui_file *file,
   if (target_architecture_user == NULL)
     fprintf_filtered (file, _("The target architecture is set "
 			      "automatically (currently %s)\n"),
-		      gdbarch_bfd_arch_info (get_current_arch ())->printable_name);
+		      architecture_string);
   else
     fprintf_filtered (file, _("The target architecture is assumed to be %s\n"),
-		      set_architecture_string);
+		      architecture_string);
 }
 
 
@@ -454,7 +449,7 @@ set_architecture (char *ignore_args, int
 
   gdbarch_info_init (&info);
 
-  if (strcmp (set_architecture_string, "auto") == 0)
+  if (strcmp (architecture_string, "auto") == 0)
     {
       target_architecture_user = NULL;
       if (!gdbarch_update_p (info))
@@ -463,7 +458,7 @@ set_architecture (char *ignore_args, int
     }
   else
     {
-      info.bfd_arch_info = bfd_scan_arch (set_architecture_string);
+      info.bfd_arch_info = bfd_scan_arch (architecture_string);
       if (info.bfd_arch_info == NULL)
 	internal_error (__FILE__, __LINE__,
 			_("set_architecture: bfd_scan_arch failed"));
@@ -471,7 +466,7 @@ set_architecture (char *ignore_args, int
 	target_architecture_user = info.bfd_arch_info;
       else
 	printf_unfiltered (_("Architecture `%s' not recognized.\n"),
-			   set_architecture_string);
+			   architecture_string);
     }
   show_architecture (gdb_stdout, from_tty, NULL, NULL);
 }
@@ -579,6 +574,22 @@ static const bfd_target *default_bfd_vec
 
 static int default_byte_order = BFD_ENDIAN_UNKNOWN;
 
+static void
+set_endian_string (struct gdbarch *arch)
+{
+  if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG)
+    endian_string = endian_big;
+  else
+    endian_string = endian_little;
+}
+
+static void
+on_architecture_change (struct gdbarch *arch)
+{
+  architecture_string = gdbarch_bfd_arch_info (arch)->printable_name;
+  set_endian_string (arch);
+}
+
 void
 initialize_current_architecture (void)
 {
@@ -655,19 +666,26 @@ initialize_current_architecture (void)
   /* Create the ``set architecture'' command appending ``auto'' to the
      list of architectures.  */
   {
-    /* Append ``auto''.  */
     int nr;
+    struct gdbarch *arch = get_current_arch ();
+
+    /* Append ``auto''.  */
     for (nr = 0; arches[nr] != NULL; nr++);
     arches = xrealloc (arches, sizeof (char*) * (nr + 2));
     arches[nr + 0] = "auto";
     arches[nr + 1] = NULL;
+
+    architecture_string = gdbarch_bfd_arch_info (arch)->printable_name;
+    set_endian_string (arch);
+
     add_setshow_enum_cmd ("architecture", class_support,
-			  arches, &set_architecture_string, 
+			  arches, &architecture_string, 
 			  _("Set architecture of target."),
 			  _("Show architecture of target."), NULL,
 			  set_architecture, show_architecture,
 			  &setlist, &showlist);
     add_alias_cmd ("processor", "architecture", class_support, 1, &setlist);
+    observer_attach_architecture_changed (on_architecture_change);
   }
 }
 
@@ -813,7 +831,7 @@ void
 _initialize_gdbarch_utils (void)
 {
   add_setshow_enum_cmd ("endian", class_support,
-			endian_enum, &set_endian_string, 
+			endian_enum, &endian_string, 
 			_("Set endianness of target."),
 			_("Show endianness of target."),
 			NULL, set_endian, show_endian,
Index: testsuite/gdb.python/py-parameter.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.python/py-parameter.exp,v
retrieving revision 1.6
diff -u -p -r1.6 py-parameter.exp
--- testsuite/gdb.python/py-parameter.exp	16 Jan 2012 16:21:52 -0000	1.6
+++ testsuite/gdb.python/py-parameter.exp	12 Oct 2012 18:25:33 -0000
@@ -26,6 +26,8 @@ gdb_reinitialize_dir $srcdir/$subdir
 # Skip all tests if Python scripting is not enabled.
 if { [skip_python_tests] } { continue }
 
+gdb_py_test_silent_cmd "python import sys" "Import sys" ""
+gdb_test "python print gdb.parameter ('endian') == sys.byteorder" "True"
 # We use "." here instead of ":" so that this works on win32 too.
 gdb_test "python print gdb.parameter ('directories')" "$srcdir/$subdir.\\\$cdir.\\\$cwd"
 

Re: [RFC] Fix for gdb.parameter('architecture') returning empty string

[Andreas Schwab]

Siva Chandra <sivachandra@google.com> writes:

> @@ -274,19 +275,13 @@ show_endian (struct ui_file *file, int f
>  	     const char *value)
>  {
>    if (target_byte_order_user == BFD_ENDIAN_UNKNOWN)
> -    if (gdbarch_byte_order (get_current_arch ()) == BFD_ENDIAN_BIG)
>        fprintf_unfiltered (file, _("The target endianness is set automatically "
> -				  "(currently big endian)\n"));
> -    else
> -      fprintf_unfiltered (file, _("The target endianness is set automatically "
> -				  "(currently little endian)\n"));
> +				  "(currently %s endian)\n"),
> +                          endian_string);
>    else
> -    if (target_byte_order_user == BFD_ENDIAN_BIG)
> -      fprintf_unfiltered (file,
> -			  _("The target is assumed to be big endian\n"));
> -    else
>        fprintf_unfiltered (file,
> -			  _("The target is assumed to be little endian\n"));
> +			  _("The target is assumed to be %s endian\n"),
> +                          endian_string);

This is bad for i18n, since big/little can no longer be translated.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [RFC] Fix for gdb.parameter('architecture') returning empty string

[Siva Chandra]

[-- Attachment #1: Type: text/plain, Size: 2786 bytes --]

Based on Andreas Schwab's feedback, I am now printing 'big' and
'little' explicitly.

2012-10-14  Siva Chandra Reddy  <sivachandra@google.com>

        * arch-utils.c: Rename variables 'set_endian_string' and
        'set_architecture_string' to 'endian_string' and
        'architecture_string' respectively.
        (show_endian): Use 'endian_string' instead of evaluating the
        endianness from the current architecture.
        (show_architecture): Use 'architecture_string' instead of
        evaluating the architecture name from the current architecture.
        (set_endian, _initialize_gdbarch_utils): Use the new
        'endian_string' variable.
        (selected_architecture_name, set_architecture): Use the new
        'architecture_string' variable.
        (set_endian_string): New convenience function to set
        'endian_string'.
        (on_architecture_change): Callback for the architecture change
        event.
        (initialize_current_architecture): Initialize
        'architecture_string' and 'endian_string'. Attach the callback
        on_architecture_change to the architecture change observer.

testsuite/

2012-10-14  Siva Chandra Reddy  <sivachandra@google.com>

        gdb.python/py-parameter.exp: Add a new test to test
        gdb.parameter ('endian').

Thanks,
Siva Chandra

On Sun, Oct 14, 2012 at 12:53 AM, Andreas Schwab <schwab@linux-m68k.org> wrote:
> Siva Chandra <sivachandra@google.com> writes:
>
>> @@ -274,19 +275,13 @@ show_endian (struct ui_file *file, int f
>>            const char *value)
>>  {
>>    if (target_byte_order_user == BFD_ENDIAN_UNKNOWN)
>> -    if (gdbarch_byte_order (get_current_arch ()) == BFD_ENDIAN_BIG)
>>        fprintf_unfiltered (file, _("The target endianness is set automatically "
>> -                               "(currently big endian)\n"));
>> -    else
>> -      fprintf_unfiltered (file, _("The target endianness is set automatically "
>> -                               "(currently little endian)\n"));
>> +                               "(currently %s endian)\n"),
>> +                          endian_string);
>>    else
>> -    if (target_byte_order_user == BFD_ENDIAN_BIG)
>> -      fprintf_unfiltered (file,
>> -                       _("The target is assumed to be big endian\n"));
>> -    else
>>        fprintf_unfiltered (file,
>> -                       _("The target is assumed to be little endian\n"));
>> +                       _("The target is assumed to be %s endian\n"),
>> +                          endian_string);
>
> This is bad for i18n, since big/little can no longer be translated.
>
> Andreas.
>
> --
> Andreas Schwab, schwab@linux-m68k.org
> GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
> "And now for something completely different."

[-- Attachment #2: param_patch_v2.txt --]
[-- Type: text/plain, Size: 6805 bytes --]

Index: arch-utils.c
===================================================================
RCS file: /cvs/src/src/gdb/arch-utils.c,v
retrieving revision 1.204
diff -u -p -r1.204 arch-utils.c
--- arch-utils.c	8 Jun 2012 14:24:56 -0000	1.204
+++ arch-utils.c	14 Oct 2012 08:24:04 -0000
@@ -32,6 +32,7 @@
 #include "target-descriptions.h"
 #include "objfiles.h"
 #include "language.h"
+#include "observer.h"
 
 #include "version.h"
 
@@ -259,7 +260,7 @@ static const char *const endian_enum[] =
   endian_auto,
   NULL,
 };
-static const char *set_endian_string;
+static const char *endian_string;
 
 enum bfd_endian
 selected_byte_order (void)
@@ -274,14 +275,14 @@ show_endian (struct ui_file *file, int f
 	     const char *value)
 {
   if (target_byte_order_user == BFD_ENDIAN_UNKNOWN)
-    if (gdbarch_byte_order (get_current_arch ()) == BFD_ENDIAN_BIG)
+    if (strcmp (endian_string, endian_big) == 0)
       fprintf_unfiltered (file, _("The target endianness is set automatically "
 				  "(currently big endian)\n"));
     else
       fprintf_unfiltered (file, _("The target endianness is set automatically "
 				  "(currently little endian)\n"));
   else
-    if (target_byte_order_user == BFD_ENDIAN_BIG)
+    if (strcmp (endian_string, endian_big) == 0)
       fprintf_unfiltered (file,
 			  _("The target is assumed to be big endian\n"));
     else
@@ -296,14 +297,14 @@ set_endian (char *ignore_args, int from_
 
   gdbarch_info_init (&info);
 
-  if (set_endian_string == endian_auto)
+  if (endian_string == endian_auto)
     {
       target_byte_order_user = BFD_ENDIAN_UNKNOWN;
       if (! gdbarch_update_p (info))
 	internal_error (__FILE__, __LINE__,
 			_("set_endian: architecture update failed"));
     }
-  else if (set_endian_string == endian_little)
+  else if (endian_string == endian_little)
     {
       info.byte_order = BFD_ENDIAN_LITTLE;
       if (! gdbarch_update_p (info))
@@ -311,7 +312,7 @@ set_endian (char *ignore_args, int from_
       else
 	target_byte_order_user = BFD_ENDIAN_LITTLE;
     }
-  else if (set_endian_string == endian_big)
+  else if (endian_string == endian_big)
     {
       info.byte_order = BFD_ENDIAN_BIG;
       if (! gdbarch_update_p (info))
@@ -416,7 +417,7 @@ enum set_arch { set_arch_auto, set_arch_
 
 static const struct bfd_arch_info *target_architecture_user;
 
-static const char *set_architecture_string;
+static const char *architecture_string;
 
 const char *
 selected_architecture_name (void)
@@ -424,7 +425,7 @@ selected_architecture_name (void)
   if (target_architecture_user == NULL)
     return NULL;
   else
-    return set_architecture_string;
+    return architecture_string;
 }
 
 /* Called if the user enters ``show architecture'' without an
@@ -437,10 +438,10 @@ show_architecture (struct ui_file *file,
   if (target_architecture_user == NULL)
     fprintf_filtered (file, _("The target architecture is set "
 			      "automatically (currently %s)\n"),
-		      gdbarch_bfd_arch_info (get_current_arch ())->printable_name);
+		      architecture_string);
   else
     fprintf_filtered (file, _("The target architecture is assumed to be %s\n"),
-		      set_architecture_string);
+		      architecture_string);
 }
 
 
@@ -454,7 +455,7 @@ set_architecture (char *ignore_args, int
 
   gdbarch_info_init (&info);
 
-  if (strcmp (set_architecture_string, "auto") == 0)
+  if (strcmp (architecture_string, "auto") == 0)
     {
       target_architecture_user = NULL;
       if (!gdbarch_update_p (info))
@@ -463,7 +464,7 @@ set_architecture (char *ignore_args, int
     }
   else
     {
-      info.bfd_arch_info = bfd_scan_arch (set_architecture_string);
+      info.bfd_arch_info = bfd_scan_arch (architecture_string);
       if (info.bfd_arch_info == NULL)
 	internal_error (__FILE__, __LINE__,
 			_("set_architecture: bfd_scan_arch failed"));
@@ -471,7 +472,7 @@ set_architecture (char *ignore_args, int
 	target_architecture_user = info.bfd_arch_info;
       else
 	printf_unfiltered (_("Architecture `%s' not recognized.\n"),
-			   set_architecture_string);
+			   architecture_string);
     }
   show_architecture (gdb_stdout, from_tty, NULL, NULL);
 }
@@ -579,6 +580,22 @@ static const bfd_target *default_bfd_vec
 
 static int default_byte_order = BFD_ENDIAN_UNKNOWN;
 
+static void
+set_endian_string (struct gdbarch *arch)
+{
+  if (gdbarch_byte_order (arch) == BFD_ENDIAN_BIG)
+    endian_string = endian_big;
+  else
+    endian_string = endian_little;
+}
+
+static void
+on_architecture_change (struct gdbarch *arch)
+{
+  architecture_string = gdbarch_bfd_arch_info (arch)->printable_name;
+  set_endian_string (arch);
+}
+
 void
 initialize_current_architecture (void)
 {
@@ -655,19 +672,26 @@ initialize_current_architecture (void)
   /* Create the ``set architecture'' command appending ``auto'' to the
      list of architectures.  */
   {
-    /* Append ``auto''.  */
     int nr;
+    struct gdbarch *arch = get_current_arch ();
+
+    /* Append ``auto''.  */
     for (nr = 0; arches[nr] != NULL; nr++);
     arches = xrealloc (arches, sizeof (char*) * (nr + 2));
     arches[nr + 0] = "auto";
     arches[nr + 1] = NULL;
+
+    architecture_string = gdbarch_bfd_arch_info (arch)->printable_name;
+    set_endian_string (arch);
+
     add_setshow_enum_cmd ("architecture", class_support,
-			  arches, &set_architecture_string, 
+			  arches, &architecture_string, 
 			  _("Set architecture of target."),
 			  _("Show architecture of target."), NULL,
 			  set_architecture, show_architecture,
 			  &setlist, &showlist);
     add_alias_cmd ("processor", "architecture", class_support, 1, &setlist);
+    observer_attach_architecture_changed (on_architecture_change);
   }
 }
 
@@ -813,7 +837,7 @@ void
 _initialize_gdbarch_utils (void)
 {
   add_setshow_enum_cmd ("endian", class_support,
-			endian_enum, &set_endian_string, 
+			endian_enum, &endian_string, 
 			_("Set endianness of target."),
 			_("Show endianness of target."),
 			NULL, set_endian, show_endian,
Index: testsuite/gdb.python/py-parameter.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.python/py-parameter.exp,v
retrieving revision 1.6
diff -u -p -r1.6 py-parameter.exp
--- testsuite/gdb.python/py-parameter.exp	16 Jan 2012 16:21:52 -0000	1.6
+++ testsuite/gdb.python/py-parameter.exp	14 Oct 2012 08:24:04 -0000
@@ -26,6 +26,8 @@ gdb_reinitialize_dir $srcdir/$subdir
 # Skip all tests if Python scripting is not enabled.
 if { [skip_python_tests] } { continue }
 
+gdb_py_test_silent_cmd "python import sys" "Import sys" ""
+gdb_test "python print gdb.parameter ('endian') == sys.byteorder" "True"
 # We use "." here instead of ":" so that this works on win32 too.
 gdb_test "python print gdb.parameter ('directories')" "$srcdir/$subdir.\\\$cdir.\\\$cwd"
 

Re: [RFC] Fix for gdb.parameter('architecture') returning empty string

[Siva Chandra]

PING

On Sun, Oct 14, 2012 at 1:36 AM, Siva Chandra <sivachandra@google.com> wrote:
> Based on Andreas Schwab's feedback, I am now printing 'big' and
> 'little' explicitly.
>
> 2012-10-14  Siva Chandra Reddy  <sivachandra@google.com>
>
>         * arch-utils.c: Rename variables 'set_endian_string' and
>         'set_architecture_string' to 'endian_string' and
>         'architecture_string' respectively.
>         (show_endian): Use 'endian_string' instead of evaluating the
>         endianness from the current architecture.
>         (show_architecture): Use 'architecture_string' instead of
>         evaluating the architecture name from the current architecture.
>         (set_endian, _initialize_gdbarch_utils): Use the new
>         'endian_string' variable.
>         (selected_architecture_name, set_architecture): Use the new
>         'architecture_string' variable.
>         (set_endian_string): New convenience function to set
>         'endian_string'.
>         (on_architecture_change): Callback for the architecture change
>         event.
>         (initialize_current_architecture): Initialize
>         'architecture_string' and 'endian_string'. Attach the callback
>         on_architecture_change to the architecture change observer.
>
> testsuite/
>
> 2012-10-14  Siva Chandra Reddy  <sivachandra@google.com>
>
>         gdb.python/py-parameter.exp: Add a new test to test
>         gdb.parameter ('endian').
>
> Thanks,
> Siva Chandra
>
> On Sun, Oct 14, 2012 at 12:53 AM, Andreas Schwab <schwab@linux-m68k.org> wrote:
>> Siva Chandra <sivachandra@google.com> writes:
>>
>>> @@ -274,19 +275,13 @@ show_endian (struct ui_file *file, int f
>>>            const char *value)
>>>  {
>>>    if (target_byte_order_user == BFD_ENDIAN_UNKNOWN)
>>> -    if (gdbarch_byte_order (get_current_arch ()) == BFD_ENDIAN_BIG)
>>>        fprintf_unfiltered (file, _("The target endianness is set automatically "
>>> -                               "(currently big endian)\n"));
>>> -    else
>>> -      fprintf_unfiltered (file, _("The target endianness is set automatically "
>>> -                               "(currently little endian)\n"));
>>> +                               "(currently %s endian)\n"),
>>> +                          endian_string);
>>>    else
>>> -    if (target_byte_order_user == BFD_ENDIAN_BIG)
>>> -      fprintf_unfiltered (file,
>>> -                       _("The target is assumed to be big endian\n"));
>>> -    else
>>>        fprintf_unfiltered (file,
>>> -                       _("The target is assumed to be little endian\n"));
>>> +                       _("The target is assumed to be %s endian\n"),
>>> +                          endian_string);
>>
>> This is bad for i18n, since big/little can no longer be translated.
>>
>> Andreas.
>>
>> --
>> Andreas Schwab, schwab@linux-m68k.org
>> GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
>> "And now for something completely different."

Re: [RFC] Fix for gdb.parameter('architecture') returning empty string

[Tom Tromey]

>>>>> "Siva" == Siva Chandra <sivachandra@google.com> writes:

Siva> Currently, gdb.parameter ('architecture') and gdb.parameter ('endian')
Siva> return an empty string.  Attached is a patch which fixes this.  I am
Siva> not very sure if this is the cleanest fix, but I could not come up
Siva> with a better one.

Thanks for the patch.

I'm not certain that this patch is correct.  Also I tend to think (but
of course I am willing to be convinced otherwise) that a different
direction would be preferable.

Siva> +static void
Siva> +on_architecture_change (struct gdbarch *arch)
Siva> +{
Siva> +  architecture_string = gdbarch_bfd_arch_info (arch)->printable_name;
Siva> +  set_endian_string (arch);
Siva> +}

Siva> +    observer_attach_architecture_changed (on_architecture_change);

I suspect this means that gdb.parameter('architecture') will not always
show the same result as "show architecture".

Right now, in the auto case show_architecture will call
get_current_arch:

    struct gdbarch *
    get_current_arch (void)
    {
      if (has_stack_frames ())
        return get_frame_arch (get_selected_frame (NULL));
      else
        return target_gdbarch;
    }

But from what I can tell, the architecture-change observer is not
notified in every situation that might cause that function to return a
different result.


I think a better approach might be to address the problem of "auto" gdb
parameters more globally.  For example, we could add a method to the
appropriate CLI object so that the Python could call this to get the
correct current value.

What do you think of that?

I don't think we'd have to convert all auto parameters at once.
However, this approach would make it easy to convert them as needed.

Tom

Re: [RFC] Fix for gdb.parameter('architecture') returning empty string

[Siva Chandra]

On Thu, Nov 1, 2012 at 1:18 PM, Tom Tromey <tromey@redhat.com> wrote:

> I'm not certain that this patch is correct.  Also I tend to think (but
> of course I am willing to be convinced otherwise) that a different
> direction would be preferable.
>
> Siva> +static void
> Siva> +on_architecture_change (struct gdbarch *arch)
> Siva> +{
> Siva> +  architecture_string = gdbarch_bfd_arch_info (arch)->printable_name;
> Siva> +  set_endian_string (arch);
> Siva> +}
>
> Siva> +    observer_attach_architecture_changed (on_architecture_change);
>
> I suspect this means that gdb.parameter('architecture') will not always
> show the same result as "show architecture".

I do not understand this completely. In my patch, show_architecture
only prints 'architecture_string'. Hence,
gdb.parameter('architecture') and "show architecture" should always
show the same result. Did I miss something?

> Right now, in the auto case show_architecture will call
> get_current_arch:
>
>     struct gdbarch *
>     get_current_arch (void)
>     {
>       if (has_stack_frames ())
>         return get_frame_arch (get_selected_frame (NULL));
>       else
>         return target_gdbarch;
>     }
>
> But from what I can tell, the architecture-change observer is not
> notified in every situation that might cause that function to return a
> different result.

Pardon my ignorance here, does target arch change between frames?

> I think a better approach might be to address the problem of "auto" gdb
> parameters more globally.  For example, we could add a method to the
> appropriate CLI object so that the Python could call this to get the
> correct current value.

Do you mean that the CLI object should have a method (which can
evaluate the property value on the go), rather than a property like
'architecture_string' and 'endian_string'? If yes, do you envision
that Python make use of the method if it exists, otherwise use the
property?

Thanks,
Siva Chandra

Re: [RFC] Fix for gdb.parameter('architecture') returning empty string

[Tom Tromey]

>>>>> "Siva" == Siva Chandra <sivachandra@google.com> writes:

Tom> I suspect this means that gdb.parameter('architecture') will not always
Tom> show the same result as "show architecture".

Siva> I do not understand this completely. In my patch, show_architecture
Siva> only prints 'architecture_string'. Hence,
Siva> gdb.parameter('architecture') and "show architecture" should always
Siva> show the same result. Did I miss something?

Ok, I see.  Your patch removes the call to get_current_arch.
I missed that somehow before, sorry about that.

I think this just means that "show architecture" will now show incorrect
results, though.

Tom> But from what I can tell, the architecture-change observer is not
Tom> notified in every situation that might cause that function to return a
Tom> different result.

Siva> Pardon my ignorance here, does target arch change between frames?

I'm reasonably ignorant here myself.  I believe the answer is "yes",
albeit AFAIK just on SPU systems.

Tom> I think a better approach might be to address the problem of "auto" gdb
Tom> parameters more globally.  For example, we could add a method to the
Tom> appropriate CLI object so that the Python could call this to get the
Tom> correct current value.

Siva> Do you mean that the CLI object should have a method (which can
Siva> evaluate the property value on the go), rather than a property like
Siva> 'architecture_string' and 'endian_string'? If yes, do you envision
Siva> that Python make use of the method if it exists, otherwise use the
Siva> property?

Yes.  In this example, the method would resemble the current
show_architecture, but it would just return the name of the current
architecture, rather than all the surrounding text.

This method is only needed for "auto-capable" parameters.

One wrinkle in this plan is that the method in question would have to
return a union type, since parameters have different underlying
representations.


BTW, what is it you are using this for?  I think in many cases it is
preferable for us to expose more API directly rather than rely on CLI
settings.  E.g., if your code needs the architecture, find a way to
intelligently expose gdbarch on frames, types, etc.

Tom

Re: [RFC] Fix for gdb.parameter('architecture') returning empty string

[Siva Chandra]

On Fri, Nov 2, 2012 at 9:58 AM, Tom Tromey <tromey@redhat.com> wrote:

> Siva> I do not understand this completely. In my patch, show_architecture
> Siva> only prints 'architecture_string'. Hence,
> Siva> gdb.parameter('architecture') and "show architecture" should always
> Siva> show the same result. Did I miss something?
>
> Ok, I see.  Your patch removes the call to get_current_arch.
> I missed that somehow before, sorry about that.
>
> I think this just means that "show architecture" will now show incorrect
> results, though.

This is for my understanding: Can you give an example where this can
give incorrect results. Is it only for the case when the arch can
change from frame to frame? I have tested for all cases that I could
think of and it seems fine.

> BTW, what is it you are using this for?  I think in many cases it is
> preferable for us to expose more API directly rather than rely on CLI
> settings.  E.g., if your code needs the architecture, find a way to
> intelligently expose gdbarch on frames, types, etc.

For now, I have been using gdb.execute("show architecture",
to_string=True) and getting the value from the resulting string. I am
trying to tidy that up by using gdb.parameter(...). And, for my work,
exposing arch from frames will work as I need the arch during an
active frame only. I see two items here then:

1. Fix gdb.parameter by the way of using methods on CLI objects.
2. Expose current frame arch through the gdb.Frame class.

Do they seem reasonable?

Thanks,
Siva Chandra

Re: [RFC] Fix for gdb.parameter('architecture') returning empty string

[Tom Tromey]

>>>>> "Siva" == Siva Chandra <sivachandra@google.com> writes:

Tom> I think this just means that "show architecture" will now show incorrect
Tom> results, though.

Siva> This is for my understanding: Can you give an example where this can
Siva> give incorrect results. Is it only for the case when the arch can
Siva> change from frame to frame? I have tested for all cases that I could
Siva> think of and it seems fine.

Yeah, if two frames have different architectures and the user does "show
arch" in each one.

I could be wrong.  I don't have a setup like this to actually test it.
I just didn't see what notifies the architecture-changed observer in
this scenario.

Similarly, I think if the inferior starts or exits, and it has a
different architecture from the target_gdbarch, then observer will not
be notified, yielding wrong results.

Siva> For now, I have been using gdb.execute("show architecture",
Siva> to_string=True) and getting the value from the resulting string. I am
Siva> trying to tidy that up by using gdb.parameter(...). And, for my work,
Siva> exposing arch from frames will work as I need the arch during an
Siva> active frame only. I see two items here then:

Siva> 1. Fix gdb.parameter by the way of using methods on CLI objects.
Siva> 2. Expose current frame arch through the gdb.Frame class.

Siva> Do they seem reasonable?

I think so.

Tom

Re: [RFC] Fix for gdb.parameter('architecture') returning empty string

[Phil Muldoon]

On 11/02/2012 06:45 PM, Siva Chandra wrote:
 
> 1. Fix gdb.parameter by the way of using methods on CLI objects.
> 2. Expose current frame arch through the gdb.Frame class.

Do you plan on modeling the architecture with its own object? (IE gdb.Frame.arch() returns a gdb.Arch object, with methods/attributes etc).

If so, can you please write up your plan first? I think architectures are strictly internal components of GDB, and may not be "baked" enough to for modeling, or may need adjustment first.  We had a discussion similar to this with some elements of breakpoints a few months back.

Cheers

Phil