[PATCH] Fix build on sparc

[PATCH] Fix build on sparc

[David Miller]

sprintf_vma() expands to a printf format string appropriate for
a bfd_vma type, so the only proper cast is to that exact type.

This fixes the build for me on sparc-*-linux*.

Ok to commit?

gdb/

2011-11-26  David S. Miller  <davem@davemloft.net>

	* remote.c (remote_get_tracepoint_status): Fix sprintf_vma() arg cast.

diff --git a/gdb/remote.c b/gdb/remote.c
index 8fa5c1a..18a3580 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -10249,7 +10249,7 @@ remote_get_tracepoint_status (struct breakpoint *bp,
     {
       utp->hit_count = 0;
       utp->traceframe_usage = 0;
-      sprintf_vma (addrbuf, (long unsigned int) utp->addr);
+      sprintf_vma (addrbuf, (bfd_vma) utp->addr);
       sprintf (rs->buf, "qTP:%x:%s", utp->number, addrbuf);
       putpkt (rs->buf);
       reply = remote_get_noisy_reply (&target_buf, &target_buf_size);

Re: [PATCH] Fix build on sparc (remote.c/tracepoints)

[Joel Brobecker]

> sprintf_vma() expands to a printf format string appropriate for
> a bfd_vma type, so the only proper cast is to that exact type.

Actually, I think the problem in this case comes from the fact
that we're calling sprintf_vma with a ULONGEST.  What we probably
want is to use phex (or phex_nz).

Attached is a patch that implements that, but only tested as far
as compiling goes. I don't have time to test it.  I decided to use
phex_nz instead of phex, because I don't think we need the leading
zeros and it seems we're trying to avoid increasing the size of
the packets unnecessarily (probably for slow links). Similarly,
for loc->address value, I used phex_nz instead of paddr because
that's what we already do elsewhere, and avoids the use of the
leading "0x" prefix.

gdb/ChangeLog:

        * remote.c (remote_get_tracepoint_status): Delete addrbuf
        local variable.  Avoid use of sprintf_vma.

PS: It looks like we're using sprintf_vma everywhere else for
    printing breakpoint location addresses, and I thought that
    we should change them as well. Except that CORE_ADDR is
    a typedef of bfd_vma, so I guess that's OK. So maybe we
    shouldn't commit the first half of my patch, as it avoids
    the use of target_gdbarch, which always makes me uncomfortable.
    But on the other hand, we avoid the use of that local bounded
    buffer. Stan's choice.

-- 
Joel

Re: [PATCH] Fix build on sparc (remote.c/tracepoints)

[David Miller]

From: Joel Brobecker <brobecker@adacore.com>
Date: Sat, 26 Nov 2011 12:14:04 -0800

> Attached is a patch that implements that, but only tested as far
> as compiling goes. I don't have time to test it.

I don't see the patch :-)

Re: [PATCH] Fix build on sparc

[Jan Kratochvil]

On Sat, 26 Nov 2011 20:38:55 +0100, David Miller wrote:
> sprintf_vma() expands to a printf format string appropriate for
> a bfd_vma type, so the only proper cast is to that exact type.
> 
> This fixes the build for me on sparc-*-linux*.

It affects also i686-linux-gnu.  It was discussed with a patch promise in:
	Re: [PATCH v2] Tracing notes and metadata
	http://sourceware.org/ml/gdb-patches/2011-11/msg00608.html


Jan

Re: [PATCH] Fix build on sparc (remote.c/tracepoints)

[Joel Brobecker]

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

> gdb/ChangeLog:
> 
>         * remote.c (remote_get_tracepoint_status): Delete addrbuf
>         local variable.  Avoid use of sprintf_vma.

Lucky me I didn't delete the patch before David called me on it!

-- 
Joel

[-- Attachment #2: remote.c.diff --]
[-- Type: text/x-diff, Size: 1368 bytes --]

diff --git a/gdb/remote.c b/gdb/remote.c
index 8fa5c1a..21561e5 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -10219,7 +10219,6 @@ remote_get_tracepoint_status (struct breakpoint *bp,
 			      struct uploaded_tp *utp)
 {
   struct remote_state *rs = get_remote_state ();
-  char addrbuf[40];
   char *reply;
   struct bp_location *loc;
   struct tracepoint *tp = (struct tracepoint *) bp;
@@ -10234,8 +10233,9 @@ remote_get_tracepoint_status (struct breakpoint *bp,
 	     any status.  */
 	  if (tp->number_on_target == 0)
 	    continue;
-	  sprintf_vma (addrbuf, loc->address);
-	  sprintf (rs->buf, "qTP:%x:%s", tp->number_on_target, addrbuf);
+	  sprintf (rs->buf, "qTP:%x:%s", tp->number_on_target,
+		   phex_nz (loc->address,
+			    gdbarch_addr_bit (target_gdbarch) / 8));
 	  putpkt (rs->buf);
 	  reply = remote_get_noisy_reply (&target_buf, &target_buf_size);
 	  if (reply && *reply)
@@ -10249,8 +10249,7 @@ remote_get_tracepoint_status (struct breakpoint *bp,
     {
       utp->hit_count = 0;
       utp->traceframe_usage = 0;
-      sprintf_vma (addrbuf, (long unsigned int) utp->addr);
-      sprintf (rs->buf, "qTP:%x:%s", utp->number, addrbuf);
+      sprintf (rs->buf, "qTP:%x:%s", utp->number, phex_nz (utp->addr, 0));
       putpkt (rs->buf);
       reply = remote_get_noisy_reply (&target_buf, &target_buf_size);
       if (reply && *reply)

Re: [PATCH] Fix build on sparc (remote.c/tracepoints)

[Stan Shebs]

On 11/26/11 12:51 PM, Joel Brobecker wrote:
>> gdb/ChangeLog:
>>
>>          * remote.c (remote_get_tracepoint_status): Delete addrbuf
>>          local variable.  Avoid use of sprintf_vma.
> Lucky me I didn't delete the patch before David called me on it!
>

Heh, I sit down from around-the-house tasks thinking to commit my fix, 
and Joel has it in hand already!  There is one tweak I would suggest:

-	  sprintf_vma (addrbuf, loc->address);
-	  sprintf (rs->buf, "qTP:%x:%s", tp->number_on_target, addrbuf);
+	  sprintf (rs->buf, "qTP:%x:%s", tp->number_on_target,
+		   phex_nz (loc->address,
+			    gdbarch_addr_bit (target_gdbarch) / 8));


Just use phex_nz (loc->address, 0) here, its builtin sizeof calculation should be satisfactory.

Stan

Re: [PATCH] Fix build on sparc (remote.c/tracepoints)

[Joel Brobecker]

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

> Heh, I sit down from around-the-house tasks thinking to commit my
> fix, and Joel has it in hand already!  There is one tweak I would
> suggest:
> 
> -	  sprintf_vma (addrbuf, loc->address);
> -	  sprintf (rs->buf, "qTP:%x:%s", tp->number_on_target, addrbuf);
> +	  sprintf (rs->buf, "qTP:%x:%s", tp->number_on_target,
> +		   phex_nz (loc->address,
> +			    gdbarch_addr_bit (target_gdbarch) / 8));
> 
> 
> Just use phex_nz (loc->address, 0) here, its builtin sizeof
> calculation should be satisfactory.

Hmm, yes, you're right. Thanks for the review.

This is the patch that I just checked in.

-- 
Joel

[-- Attachment #2: remote.c.diff --]
[-- Type: text/x-diff, Size: 2281 bytes --]

commit d39c89d92ee7ed6afbf9f11398a566836e52bb17
Author: Joel Brobecker <brobecker@adacore.com>
Date:   Sun Nov 27 09:35:17 2011 -0800

    remove use of sprintf_vma in remote_get_tracepoint_status
    
    This function takes a VMA and was forcing us to perform a cast to
    compile without warning on one platform. Except that the cast caused
    a warning on sparc-solaris.  This patch fixes the problem by using
    phex_nz instead.
    
    gdb/ChangeLog:
    
            * remote.c (remote_get_tracepoint_status): Delete addrbuf
            local variable.  Avoid use of sprintf_vma.

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 6fe2a5c..d6551d6 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2011-11-27  Joel Brobecker  <brobecker@acacore.com>
+
+	* remote.c (remote_get_tracepoint_status): Delete addrbuf
+	local variable.  Avoid use of sprintf_vma.
+
 2011-11-27  Sanjoy Das  <sdas@igalia.com>
 
 	Fix regression in jit.exp.
diff --git a/gdb/remote.c b/gdb/remote.c
index 8fa5c1a..6e37f69 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -10219,7 +10219,6 @@ remote_get_tracepoint_status (struct breakpoint *bp,
 			      struct uploaded_tp *utp)
 {
   struct remote_state *rs = get_remote_state ();
-  char addrbuf[40];
   char *reply;
   struct bp_location *loc;
   struct tracepoint *tp = (struct tracepoint *) bp;
@@ -10234,8 +10233,8 @@ remote_get_tracepoint_status (struct breakpoint *bp,
 	     any status.  */
 	  if (tp->number_on_target == 0)
 	    continue;
-	  sprintf_vma (addrbuf, loc->address);
-	  sprintf (rs->buf, "qTP:%x:%s", tp->number_on_target, addrbuf);
+	  sprintf (rs->buf, "qTP:%x:%s", tp->number_on_target,
+		   phex_nz (loc->address, 0));
 	  putpkt (rs->buf);
 	  reply = remote_get_noisy_reply (&target_buf, &target_buf_size);
 	  if (reply && *reply)
@@ -10249,8 +10248,7 @@ remote_get_tracepoint_status (struct breakpoint *bp,
     {
       utp->hit_count = 0;
       utp->traceframe_usage = 0;
-      sprintf_vma (addrbuf, (long unsigned int) utp->addr);
-      sprintf (rs->buf, "qTP:%x:%s", utp->number, addrbuf);
+      sprintf (rs->buf, "qTP:%x:%s", utp->number, phex_nz (utp->addr, 0));
       putpkt (rs->buf);
       reply = remote_get_noisy_reply (&target_buf, &target_buf_size);
       if (reply && *reply)