Fix length calculation in aarch64_linux_set_debug_regs

  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(-)
  

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
  

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.
  

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.
  

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
  

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?
  

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
  

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.
  

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?
  

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?
  

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.