Re: bitpos expansion patches summary

[Siddhesh Poyarekar <siddhesh at redhat dot com>]

On Sun, 12 Aug 2012 19:57:30 +0200, Jan wrote:
> (arm-tdep.c:3425):	RET:	(ULONGEST to int)
> [(t)->length / unitlen]
>  - this is correct but arm-tdep.c:3442 is wrong, I guess you did not
> rerun splint after the changes to check for new reported regressions.

I did, but I only went through it superficially.

> 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? It seemed
pointless to me since it's only used at the caller and it doesn't look
like any other function will have use of it. I could just remove the
type_is_size_t_or_error call here. Likewise for other tdep instances.

> (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.

> [buffer_size < this_size] SAFE: (dwarf2loc.c:1598):
> ASSIGN:	(ULONGEST to size_t)	[ buffer_size = this_size]
>  - I do not see how this can be safe, some ENSURED_SIZET is needed

OK, I'll make this part of the patch 2/3.

> FIXED(Expand this_size_bits): (dwarf2loc.c:2002):	CMP:
> (size_t to LONGEST)	[bit_offset >= this_size_bits]
>  - I do not see this_size_bits expanded, there is still: size_t
> this_size_bits = p->size;

Sorry, will fix.

>  +      ensure_type_fits_sizet (arg_type);
> +xENSURED_SIZET: (alpha-tdep.c:402):      ASSIGN: (LONGEST to
> int)        [ accumulate_size = (accumulate_size + m_arg->len + 7) &
> ~7] + +Just move m_arg->contents = value_contents (arg); here and you
> can drop this +call of ensure_type_fits_sizet.

OK.

>  +      if (accumulate_size < 0)
>  +        error (_("Total size of arguments too large for host GDB
> memory.")); +xENSURED_SIZET: (alpha-tdep.c:402):      ASSIGN:
> (LONGEST to int)        [ accumulate_size = (accumulate_size +
> m_arg->len + 7) & ~7] + +If you make accumulate_size LONGEST you can
> drop this check.  Then just verify +you do not underflow SP in the
> line below:

OK.


Thanks,
Siddhesh