Re: bitpos expansion patches summary

[Siddhesh Poyarekar <siddhesh@redhat.com>]

On Sun, 26 Aug 2012 19:43:35 +0200, Jan wrote:
> (i386-nat.c:613):
> FUNC(i386_length_and_rw_bits):	(LONGEST to int)	[len]
>  - To be re-checked with updated watchpoints patch.

This is safe, since ok_for_watchpoint will restrict the incoming LEN in
insert_watchpoint.

> WPREVERTED(Likewise): (i386-nat.c:647):
> FUNC(i386_length_and_rw_bits):	(LONGEST to int)	[len]
>  - To be re-checked with updated watchpoints patch.

Likewise for remove_watchpoint.

> SAFE:
> (ia64-tdep.c:3299):	VARINIT(n):	(ULONGEST to
> int)	[(type)->length / (float_elt_type)->length]
>  - You did expand n in ia64_extract_return_value, it can be array of
> floats. I did not investigate what happens if the array of floats is
> too large and it does not fit into ia64 registers.  But I find safe
> to just expand it.

OK, I misread the HFA definition as an array/struct that *only* has a
float. I think I confused this with some other function that has this
definition.

> REVERTED(siginfo type should fit into
> size_t): (infrun.c:6677):	FUNC(xmalloc):	(ULONGEST to
> size_t)	[len]
>  - I agree. So why have you changed 'size_t->ULONGEST len =
> TYPE_LENGTH (type);' ? That change should be therefore reverted.

Right, will revert.

> FIXED(Expand param for get_frame_register_bytes):
> (m68k-tdep.c:225):	FUNC(get_frame_register_bytes):
> (ULONGEST to int)	[(type)->length]
>  - I am not against expanding LEN of get_frame_register_bytes, it has
> no use but it is a good sanity check instead of checking all the
> callers. But in this case BTW it is not needed, only TYPE_CODE_FLT
> can be passed to get_frame_register_bytes, TYPE_CODE_FLT is always
> small enough.

Yes, you had suggested this expansion as a sanity check in an earlier
review.

> FIXED(Expand to size_t)|ENSURED_SIZET: (m88k-tdep.c:311):
> VARINIT(len):	(ULONGEST to int)	[(type)->length]
>  - ENSURED_SIZET is redundant, there is above already:
>    const bfd_byte *valbuf = value_contents (args[i]);

Yes, all of the ENSURED_SIZET for -tdep have been removed for similar
reasons.

> SAFE:
> (mdebugread.c:1238):	ASSIGN:	(bfd_vma to
> LONGEST)	[ ((*f).loc.bitpos) = (sh->value)]
>  - Not so clear to me, --disable-64-bit-bfd cannot support >=2GB
> inferiors but it should support >256MB structs.  Still when it also
> affects only STABS I do not think it needs a fix.

Either ways, the point is that sizeof (bfd_vma) would always be less or
equal to than sizeof (LONGEST) and hence this ought to be safe.

Regards,
Siddhesh