This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: bitpos expansion patches summary
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