Re: bitpos expansion patches summary

Jan Kratochvil
2012-08-13T13:49:42Z

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