Re: bitpos expansion patches summary

Siddhesh Poyarekar
2012-08-27T08:10:28Z

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