[PATCH] Fix length calculation in aarch64_linux_set_debug_regs

[PATCH] Fix length calculation in aarch64_linux_set_debug_regs

[Simon Marchi]

There is this build failure when building in C++:

In file included from build-gnulib/import/stddef.h:45:0,
                 from /usr/include/time.h:37,
                 from build-gnulib/import/time.h:41,
                 from build-gnulib/import/sys/stat.h:44,
                 from ../bfd/bfd.h:44,
                 from /home/simark/src/binutils-gdb/gdb/common/common-types.h:35,
                 from /home/simark/src/binutils-gdb/gdb/common/common-defs.h:44,
                 from /home/simark/src/binutils-gdb/gdb/nat/aarch64-linux-hw-point.c:19:
/home/simark/src/binutils-gdb/gdb/nat/aarch64-linux-hw-point.c: In function ‘void aarch64_linux_set_debug_regs(const aarch64_debug_reg_state*, int, int)’:
/home/simark/src/binutils-gdb/gdb/nat/aarch64-linux-hw-point.c:564:64: error: ‘count’ cannot appear in a constant-expression
   iov.iov_len = (offsetof (struct user_hwdebug_state, dbg_regs[count - 1])
                                                                ^

I don't really understand the length computation done here.

From what I understand, the dbg_regs array in the user_hwdebug_state structure
is 16 elements long, but we don't use all of them.  We want iov_len to reflect
only the used bytes.  If that's true, I don't think that the current code is
correct.  Instead, it can be computed simply with:

  offsetof (struct user_hwdebug_state, dbg_regs) + count * sizeof (regs.dbg_reg[0]);

Does it make sense?

gdb/ChangeLog:

	* nat/aarch64-linux-hw-point.c (aarch64_linux_set_debug_regs): Fix
	iov_len computation.
---
 gdb/nat/aarch64-linux-hw-point.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/nat/aarch64-linux-hw-point.c b/gdb/nat/aarch64-linux-hw-point.c
index 1a5fa6a..dcbfa98 100644
--- a/gdb/nat/aarch64-linux-hw-point.c
+++ b/gdb/nat/aarch64-linux-hw-point.c
@@ -561,8 +561,8 @@ aarch64_linux_set_debug_regs (const struct aarch64_debug_reg_state *state,
   ctrl = watchpoint ? state->dr_ctrl_wp : state->dr_ctrl_bp;
   if (count == 0)
     return;
-  iov.iov_len = (offsetof (struct user_hwdebug_state, dbg_regs[count - 1])
-		 + sizeof (regs.dbg_regs [count - 1]));
+  iov.iov_len = (offsetof (struct user_hwdebug_state, dbg_regs)
+		 + count * sizeof (regs.dbg_regs[0]));
 
   for (i = 0; i < count; i++)
     {
-- 
2.5.1

Re: [PATCH] Fix length calculation in aarch64_linux_set_debug_regs

[Pedro Alves]

On 11/02/2015 02:48 PM, Simon Marchi wrote:
> There is this build failure when building in C++:
> 
> In file included from build-gnulib/import/stddef.h:45:0,
>                  from /usr/include/time.h:37,
>                  from build-gnulib/import/time.h:41,
>                  from build-gnulib/import/sys/stat.h:44,
>                  from ../bfd/bfd.h:44,
>                  from /home/simark/src/binutils-gdb/gdb/common/common-types.h:35,
>                  from /home/simark/src/binutils-gdb/gdb/common/common-defs.h:44,
>                  from /home/simark/src/binutils-gdb/gdb/nat/aarch64-linux-hw-point.c:19:
> /home/simark/src/binutils-gdb/gdb/nat/aarch64-linux-hw-point.c: In function ‘void aarch64_linux_set_debug_regs(const aarch64_debug_reg_state*, int, int)’:
> /home/simark/src/binutils-gdb/gdb/nat/aarch64-linux-hw-point.c:564:64: error: ‘count’ cannot appear in a constant-expression
>    iov.iov_len = (offsetof (struct user_hwdebug_state, dbg_regs[count - 1])
>                                                                 ^
> 
> I don't really understand the length computation done here.
> 
> From what I understand, the dbg_regs array in the user_hwdebug_state structure
> is 16 elements long, but we don't use all of them.  We want iov_len to reflect
> only the used bytes.  If that's true, I don't think that the current code is
> correct.
> Instead, it can be computed simply with:
> 
>   offsetof (struct user_hwdebug_state, dbg_regs) + count * sizeof (regs.dbg_reg[0]);
> 
> Does it make sense?

IIUYC, you're pointing out two issues:

#1 - the offsetof that doesn't work in C++.

#2 - an off-by-one.

I don't know enough about Aarch64 to judge #1, but it does sound right to me.

On #2, I saw the same on x86.  See my fix here:

  https://sourceware.org/ml/gdb-patches/2015-02/msg00213.html

I think it's a little nicer to hide away the offsetof+sizeof.

Thanks,
Pedro Alves

Re: [PATCH] Fix length calculation in aarch64_linux_set_debug_regs

[Simon Marchi]

I looked a bit more into the issue and did some testing, and it appears the
current code is correct (as Yao mentioned).  It's probably not as clear as it
could be though.  I think it would be nicer if expressed as

  (size of fixed part) + (size of variable part)


On 15-11-02 11:00 AM, Pedro Alves wrote:
> IIUYC, you're pointing out two issues:
> #1 - the offsetof that doesn't work in C++.

This actually appears to be a bug in g++.

See this, especially towards the end: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=14932

The expression is accepted by gcc and clang++, but not g++ (test with 4.8 and 5.2).

> #2 - an off-by-one.

Never mind, I thought wrongly at first that sizeof (regs.dbg_regs [count - 1])
would give the size for "count - 1" elements, but it's actually the size of a
single element.  So scratch that, the current code works fine.

So the only reason to change the code would be to circumvent a bug in g++, which
is probably a valid one (otherwise we can't build).

> I don't know enough about Aarch64 to judge #1, but it does sound right to me.
> 
> On #2, I saw the same on x86.  See my fix here:
> 
>   https://sourceware.org/ml/gdb-patches/2015-02/msg00213.html
> 
> I think it's a little nicer to hide away the offsetof+sizeof.

You mean hide in in a function?  This expression is only used at one place and I think it's
reasonably straightforward if expressed correctly, but if you think it will make the code
clearer I don't mind.

Re: [PATCH] Fix length calculation in aarch64_linux_set_debug_regs

[Pedro Alves]

On 11/02/2015 05:09 PM, Simon Marchi wrote:
> I looked a bit more into the issue and did some testing, and it appears the
> current code is correct (as Yao mentioned).  It's probably not as clear as it
> could be though.  I think it would be nicer if expressed as
> 
>   (size of fixed part) + (size of variable part)

Looking a bit more at the code in question, I agree.

> On 15-11-02 11:00 AM, Pedro Alves wrote:
>> IIUYC, you're pointing out two issues:
>> #1 - the offsetof that doesn't work in C++.
> 
> This actually appears to be a bug in g++.
> 
> See this, especially towards the end: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=14932
> 
> The expression is accepted by gcc and clang++, but not g++ (test with 4.8 and 5.2).

OK.  Still not sure whether it's a bug or not.  It may still be invalid C++ that
happens to be accepted by clang++.


>> I think it's a little nicer to hide away the offsetof+sizeof.
> 
> You mean hide in in a function?  This expression is only used at one place and I think it's
> reasonably straightforward if expressed correctly, but if you think it will make the code
> clearer I don't mind.

You convinced me.  This is not really the same as the x86 case, where
we wanted the offset of a register field.

Thanks,
Pedro Alves

Re: [PATCH] Fix length calculation in aarch64_linux_set_debug_regs

[Simon Marchi]

On 15-11-02 12:57 PM, Pedro Alves wrote:
> OK.  Still not sure whether it's a bug or not.  It may still be invalid C++ that
> happens to be accepted by clang++.

Looking at the standard (well, the latest draft of it), section 18.2, my
understanding is that it should behave the same way as C.  How do you suggest
we find out?

Re: [PATCH] Fix length calculation in aarch64_linux_set_debug_regs

[Pedro Alves]

On 11/02/2015 06:17 PM, Simon Marchi wrote:
> On 15-11-02 12:57 PM, Pedro Alves wrote:
>> OK.  Still not sure whether it's a bug or not.  It may still be invalid C++ that
>> happens to be accepted by clang++.
> 
> Looking at the standard (well, the latest draft of it), section 18.2, my
> understanding is that it should behave the same way as C.  How do you suggest
> we find out?

Maybe ask on gcc-help@.

Thanks,
Pedro Alves

Re: [PATCH] Fix length calculation in aarch64_linux_set_debug_regs

[Andreas Schwab]

Simon Marchi <simon.marchi@ericsson.com> writes:

> Looking at the standard (well, the latest draft of it), section 18.2, my
> understanding is that it should behave the same way as C.

In C it is not allowed either.

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: [PATCH] Fix length calculation in aarch64_linux_set_debug_regs

[Yao Qi]

Hi Simon,

On 02/11/15 14:48, Simon Marchi wrote:
>  From what I understand, the dbg_regs array in the user_hwdebug_state structure
> is 16 elements long, but we don't use all of them.  We want iov_len to reflect
> only the used bytes.  If that's true, I don't think that the current code is
> correct.  Instead, it can be computed simply with:

Your understand is correct, but the code is correct too.

>
>    offsetof (struct user_hwdebug_state, dbg_regs) + count * sizeof (regs.dbg_reg[0]);
>

Your code must get the same value as the old code does.

> Does it make sense?

Yes, but I think Pedro's suggestion is better.

-- 
Yao (齐尧)

Re: [PATCH] Fix length calculation in aarch64_linux_set_debug_regs

[Yao Qi]

Simon Marchi <simon.marchi@ericsson.com> writes:

> correct.  Instead, it can be computed simply with:
>
>   offsetof (struct user_hwdebug_state, dbg_regs) + count * sizeof (regs.dbg_reg[0]);
>
> Does it make sense?
>
> gdb/ChangeLog:
>
> 	* nat/aarch64-linux-hw-point.c (aarch64_linux_set_debug_regs): Fix
> 	iov_len computation.

The old code is correct, but in order to make G++ happy, we still need
this patch, is that right?

-- 
Yao (齐尧)

Re: [PATCH] Fix length calculation in aarch64_linux_set_debug_regs

[Simon Marchi]

On 15-11-19 06:52 AM, Yao Qi wrote:
> The old code is correct, but in order to make G++ happy, we still need
> this patch, is that right?

Yes.  Do you want me to push it, or I let you do it with the other aarch64
patches?

Re: [PATCH] Fix length calculation in aarch64_linux_set_debug_regs

[Yao Qi]

On 19/11/15 13:32, Simon Marchi wrote:
> Yes.  Do you want me to push it, or I let you do it with the other aarch64
> patches?

You can push it in, with some commit log adjustments, because the old
code is correct.

-- 
Yao (齐尧)

Re: [PATCH] Fix length calculation in aarch64_linux_set_debug_regs

[Simon Marchi]

On 15-11-19 08:50 AM, Yao Qi wrote:
> You can push it in, with some commit log adjustments, because the old
> code is correct.

Ok I pushed it.  Again, sorry for saying the code was not correct :).


From bb82e93484cdd56c67efd52b869a6123b2623f6c Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Thu, 19 Nov 2015 10:17:36 -0500
Subject: [PATCH] Fix iov_len calculation in aarch64_linux_set_debug_regs
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

There is this build failure when building in C++:

/home/simark/src/binutils-gdb/gdb/nat/aarch64-linux-hw-point.c: In function ‘void aarch64_linux_set_debug_regs(const aarch64_debug_reg_state*, int, int)’:
/home/simark/src/binutils-gdb/gdb/nat/aarch64-linux-hw-point.c:564:64: error: ‘count’ cannot appear in a constant-expression
   iov.iov_len = (offsetof (struct user_hwdebug_state, dbg_regs[count - 1])
                                                                ^
We can simplify the computation and make g++ happy at the same time by
formulating as:

  size of fixed part + size of variable part

thus...

  size of fixed part + count * size of one variable part element

thus...

  offsetof (struct user_hwdebug_state, dbg_regs) + count * sizeof (regs.dbg_reg[0]);

gdb/ChangeLog:

	* nat/aarch64-linux-hw-point.c (aarch64_linux_set_debug_regs): Change
	form of iov_len computation.
---
 gdb/ChangeLog                    | 5 +++++
 gdb/nat/aarch64-linux-hw-point.c | 4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 0539d96..94721a0 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2015-11-19  Simon Marchi  <simon.marchi@ericsson.com>
+
+	* nat/aarch64-linux-hw-point.c (aarch64_linux_set_debug_regs): Change
+	form of iov_len computation.
+
 2015-11-19  Pedro Alves  <palves@redhat.com>

 	* configure.ac (ERROR_ON_WARNING): Don't check whether in C++
diff --git a/gdb/nat/aarch64-linux-hw-point.c b/gdb/nat/aarch64-linux-hw-point.c
index 1a5fa6a..dcbfa98 100644
--- a/gdb/nat/aarch64-linux-hw-point.c
+++ b/gdb/nat/aarch64-linux-hw-point.c
@@ -561,8 +561,8 @@ aarch64_linux_set_debug_regs (const struct aarch64_debug_reg_state *state,
   ctrl = watchpoint ? state->dr_ctrl_wp : state->dr_ctrl_bp;
   if (count == 0)
     return;
-  iov.iov_len = (offsetof (struct user_hwdebug_state, dbg_regs[count - 1])
-		 + sizeof (regs.dbg_regs [count - 1]));
+  iov.iov_len = (offsetof (struct user_hwdebug_state, dbg_regs)
+		 + count * sizeof (regs.dbg_regs[0]));

   for (i = 0; i < count; i++)
     {
-- 
2.5.1