Re: bitpos expansion patches summary

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Mon, 13 Aug 2012 04:51:24 +0200, Siddhesh Poyarekar wrote:
> > i] ENSURED_SIZET: (avr-tdep.c:1184):	FUNC(xmalloc):
> > (LONGEST to size_t)	[len]
> >  - You do not, you have changed parameter len to ssize_t, you did
> > ENSURED_SIZET on line 1298.
> >  - But that ENSURED_SIZET is redundant there as the code already
> 
> OK, do you want me to add the check within the function?

I was requesting only to remove the redundant line
	avr-tdep.c:1298:ensure_type_fits_sizet (type);

as on this line GDB already executed line
	const bfd_byte *contents = value_contents (arg);

which would already abort for too large TYPE_LENGTH.


Unrelated to this specific case but to ensure_type_fits_sizet in general:

Could you change these calls of ensure_type_fits_sizet where we already deal
with the length itself and no longer the type?

          ensure_type_fits_sizet (type);
          si = push_stack_item (si, contents, len);

I would find it easier as:
          ensure_length_fits_sizet (len);
          si = push_stack_item (si, contents, len);

Here only LEN matters.  LEN is LONGEST so we can verify whether LONGEST LEN
fits into SSIZE_T.  Reintroducing here a dependency on TYPE is a needless
little complication of the code IMO.


Moreover even when we have TYPE we could use:
          ensure_length_fits_sizet (TYPE_LENGTH (type));

ensure_type_fits_sizet uses only TYPE->LENGTH so it does not need to be passed
TYPE at all.  It would make sense if ensure_type_fits_sizet prints TYPE_NAME
in the error case etc. - but it does not use TYPE_NAME anyway..


> I could just remove the type_is_size_t_or_error call here.

Yes.


> Likewise for other tdep instances.

I do not see how it can be generalized, I may not just see what do you mean.


> > (breakpoint.c:4624):	FUNC():	(LONGEST to int)
> > [loc->length]
> >  - That's not true, s390x can set arbitrary hardware watchpoint
> > memory range, see s390_fix_watch_points. WPFIXED(Expand
> 
> Oh, then my assumption about watchpoint sizes would be wrong then
> wouldn't it? We then need to expand the ok_for_watchpoint as well as
> {insert|remove}_watchpoint. We could do it as a different changeset
> since only s390x would need it.

You are right.  Sorry I did not remember this s390x feature when we discussed
it before.


Thanks,
Jan