[1/3] Remove gdbarch-swapping of remote_address_masked

[1/3] Remove gdbarch-swapping of remote_address_masked

[Ulrich Weigand]

Hello,

remote.c contains a gdbarch-swapped variable "remote_address_masked".  The
swap function in fact re-sets the variable to the default address size of
the target.

This patch attempts to decouple changes to the variable by the user
(via "set remoteaddresssize") from changes due to architecture switches:

- If the user sets the variable, that value is subsequently used (even
  if the architecture switches).

- If the user never used "set remoteaddresssize" (i.e. the variable
  reads 0), remote_address_masked implicitly uses the current target's
  address size instead.


Now this is certainly a change in behaviour, but I'd think the new way
should actually be more useful ...

Any comments from users of that functionality?  Does this make sense?

Bye,
Ulrich



ChangeLog:

	* remote.c (remote_address_masked): If remote_address_size is zero,
	default to target address size.
	(build_remote_gdbarch_data): Remove.
	(_initialize_remote): Do not swap remote_address_size.

diff -urNp gdb-orig/gdb/remote.c gdb-head/gdb/remote.c
--- gdb-orig/gdb/remote.c	2007-06-08 13:48:41.294606000 +0200
+++ gdb-head/gdb/remote.c	2007-06-08 20:15:01.825476499 +0200
@@ -3919,13 +3919,18 @@ hexnumnstr (char *buf, ULONGEST num, int
 static CORE_ADDR
 remote_address_masked (CORE_ADDR addr)
 {
-  if (remote_address_size > 0
-      && remote_address_size < (sizeof (ULONGEST) * 8))
+  int address_size = remote_address_size;
+  /* If "remoteaddresssize" was not set, default to target address size.  */
+  if (!address_size)
+    address_size = TARGET_ADDR_BIT;
+
+  if (address_size > 0
+      && address_size < (sizeof (ULONGEST) * 8))
     {
       /* Only create a mask when that mask can safely be constructed
          in a ULONGEST variable.  */
       ULONGEST mask = 1;
-      mask = (mask << remote_address_size) - 1;
+      mask = (mask << address_size) - 1;
       addr &= mask;
     }
   return addr;
@@ -6347,12 +6352,6 @@ show_remote_cmd (char *args, int from_tt
   do_cleanups (showlist_chain);
 }
 
-static void
-build_remote_gdbarch_data (void)
-{
-  remote_address_size = TARGET_ADDR_BIT;
-}
-
 /* Function to be called whenever a new objfile (shlib) is detected.  */
 static void
 remote_new_objfile (struct objfile *objfile)
@@ -6372,11 +6371,6 @@ _initialize_remote (void)
   remote_g_packet_data_handle =
     gdbarch_data_register_pre_init (remote_g_packet_data_init);
 
-  /* Old tacky stuff.  NOTE: This comes after the remote protocol so
-     that the remote protocol has been initialized.  */
-  DEPRECATED_REGISTER_GDBARCH_SWAP (remote_address_size);
-  deprecated_register_gdbarch_swap (NULL, 0, build_remote_gdbarch_data);
-
   /* Initialize the per-target state.  At the moment there is only one
      of these, not one per target.  Only one target is active at a
      time.  The default buffer size is unimportant; it will be expanded
-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [1/3] Remove gdbarch-swapping of remote_address_masked

[Jim Blandy]

"Ulrich Weigand" <uweigand@de.ibm.com> writes:
> remote.c contains a gdbarch-swapped variable "remote_address_masked".  The
> swap function in fact re-sets the variable to the default address size of
> the target.
>
> This patch attempts to decouple changes to the variable by the user
> (via "set remoteaddresssize") from changes due to architecture switches:
>
> - If the user sets the variable, that value is subsequently used (even
>   if the architecture switches).
>
> - If the user never used "set remoteaddresssize" (i.e. the variable
>   reads 0), remote_address_masked implicitly uses the current target's
>   address size instead.
>
>
> Now this is certainly a change in behaviour, but I'd think the new way
> should actually be more useful ...
>
> Any comments from users of that functionality?  Does this make sense?

It seems to me that this makes the remoteaddresssize setting behave
much like language settings: there's 'auto (currently blargh)' and
'blargh'.  To be honest, I don't think it's worthwhile making this
variable actually behave that way.  I don't even want to know what it
was added for.

Re: [1/3] Remove gdbarch-swapping of remote_address_masked

[Daniel Jacobowitz]

On Sat, Jun 09, 2007 at 01:22:35AM +0200, Ulrich Weigand wrote:
> Hello,
> 
> remote.c contains a gdbarch-swapped variable "remote_address_masked".  The
> swap function in fact re-sets the variable to the default address size of
> the target.
> 
> This patch attempts to decouple changes to the variable by the user
> (via "set remoteaddresssize") from changes due to architecture switches:
> 
> - If the user sets the variable, that value is subsequently used (even
>   if the architecture switches).
> 
> - If the user never used "set remoteaddresssize" (i.e. the variable
>   reads 0), remote_address_masked implicitly uses the current target's
>   address size instead.
> 
> 
> Now this is certainly a change in behaviour, but I'd think the new way
> should actually be more useful ...
> 
> Any comments from users of that functionality?  Does this make sense?

Seems right to me.  I can't tell if there are any users, because there
seems to be at least one IDE or set of instructions on the internet
that recommends setting it; I found several places where it was set to
the default value, but I'm not sure about others.  If we have to keep
it, I think this is the right way to do so.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [1/3] Remove gdbarch-swapping of remote_address_masked

[Ulrich Weigand]

Jim Blandy wrote:

> It seems to me that this makes the remoteaddresssize setting behave
> much like language settings: there's 'auto (currently blargh)' and
> 'blargh'.

Yes, that was the intent.

> To be honest, I don't think it's worthwhile making this
> variable actually behave that way.

Well, we have to change *something* so we can remove the
gdbarch_swap call.  This change seemed the most straight-
forward to me ...  How would you suggest the variable
should behave?

> I don't even want to know what it was added for.

Well, me neither ;-)

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [1/3] Remove gdbarch-swapping of remote_address_masked

[Joel Brobecker]

Ulrich,

> -static void
> -build_remote_gdbarch_data (void)
> -{
> -  remote_address_size = TARGET_ADDR_BIT;
> -}
> -

I just noticed by accident that there is a declaration of this
function that should be removed as well:

    static void build_remote_gdbarch_data (void);

Cheers,
-- 
Joel

Re: [1/3] Remove gdbarch-swapping of remote_address_masked

[Ulrich Weigand]

Joel Brobecker wrote:

> I just noticed by accident that there is a declaration of this
> function that should be removed as well:
> 
>     static void build_remote_gdbarch_data (void);

Ah, thanks.  I've updated my patch accordingly.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [1/3] Remove gdbarch-swapping of remote_address_masked

[Ulrich Weigand]

> 	* remote.c (remote_address_masked): If remote_address_size is zero,
> 	default to target address size.
> 	(build_remote_gdbarch_data): Remove.
> 	(_initialize_remote): Do not swap remote_address_size.

I've now committed the following version of this patch.

Bye,
Ulrich


diff -urNp gdb-orig/gdb/remote.c gdb-head/gdb/remote.c
--- gdb-orig/gdb/remote.c	2007-06-14 14:56:59.063426000 +0200
+++ gdb-head/gdb/remote.c	2007-06-14 15:01:54.131761491 +0200
@@ -85,8 +85,6 @@ static void handle_remote_sigint_twice (
 static void async_remote_interrupt (gdb_client_data);
 void async_remote_interrupt_twice (gdb_client_data);
 
-static void build_remote_gdbarch_data (void);
-
 static void remote_files_info (struct target_ops *ignore);
 
 static void remote_prepare_to_store (struct regcache *regcache);
@@ -3926,13 +3924,18 @@ hexnumnstr (char *buf, ULONGEST num, int
 static CORE_ADDR
 remote_address_masked (CORE_ADDR addr)
 {
-  if (remote_address_size > 0
-      && remote_address_size < (sizeof (ULONGEST) * 8))
+  int address_size = remote_address_size;
+  /* If "remoteaddresssize" was not set, default to target address size.  */
+  if (!address_size)
+    address_size = gdbarch_addr_bit (current_gdbarch);
+
+  if (address_size > 0
+      && address_size < (sizeof (ULONGEST) * 8))
     {
       /* Only create a mask when that mask can safely be constructed
          in a ULONGEST variable.  */
       ULONGEST mask = 1;
-      mask = (mask << remote_address_size) - 1;
+      mask = (mask << address_size) - 1;
       addr &= mask;
     }
   return addr;
@@ -6408,11 +6411,6 @@ show_remote_cmd (char *args, int from_tt
   do_cleanups (showlist_chain);
 }
 
-static void
-build_remote_gdbarch_data (void)
-{
-  remote_address_size = gdbarch_addr_bit (current_gdbarch);
-}
 
 /* Function to be called whenever a new objfile (shlib) is detected.  */
 static void
@@ -6433,11 +6431,6 @@ _initialize_remote (void)
   remote_g_packet_data_handle =
     gdbarch_data_register_pre_init (remote_g_packet_data_init);
 
-  /* Old tacky stuff.  NOTE: This comes after the remote protocol so
-     that the remote protocol has been initialized.  */
-  DEPRECATED_REGISTER_GDBARCH_SWAP (remote_address_size);
-  deprecated_register_gdbarch_swap (NULL, 0, build_remote_gdbarch_data);
-
   /* Initialize the per-target state.  At the moment there is only one
      of these, not one per target.  Only one target is active at a
      time.  The default buffer size is unimportant; it will be expanded


-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com