Re: bitpos expansion patches summary

[Siddhesh Poyarekar <siddhesh@redhat.com>]

On Tue, 4 Sep 2012 17:03:25 +0200, Jan wrote:
> here is the last part of this first phase.

PHEW!
 
> FIXED(Expand i. Callers could be expanded separately):
> (valprint.c:1607):	CMP:	(ULONGEST to UINT)	[i <
> len]
>  - Callers should be expanded because of splint warnings there.
>    But for example ada-valprint.c:700 is not reported in this file at
> all, why? 

Because callers will be sending in a smaller size, which does not
count as a problem.

> SAFE: (valprint.c:2079):
> CMP:	(int to ULONGEST)	[length == -1]
>  - But as LENGTH is just a length of character the expansion
>    'unsigned int'->ULONGEST should be reverted.

length would be the length of the string; width is the length of
character.

> SAFE:
> (value.c:949):	FUNC(memcpy):	(LONGEST to size_t)
> [length]
>  - The expansion of int dst_offset, src_offset and length could be
> just ssize_t (instead of LONGEST). 

OK. Likewise for value_contents_copy then?

> SAFE: (value.c:2648):	ASSIGN:	(LONGEST to int)
> [ v->bitpos = bitpos % container_bitsize]
>  - The local variable container_bitsize does not need to be LONGEST.
>    bitfield cannot be too large, at most about 64 bits or so.
>    'type' there is the containing the of the bitfield (such as long
> long), not the whole struct the bitfield is contained in.
>    There could be some ENSURE instead as some bogus DWARF could
> violate that.

OK.

> = (type)->length] ENSURED_SIZET(allocate_value_lazy):
> (value.c:3144):	FUNC(memcpy):	(ULONGEST to
> size_t)	[(type)->length]
>  - I do not see any need and any change of source code here, it is
> protected by ENSURE in allocate_value_contents. 

It could be called independently of allocate_value_contents.

> UNRELATED(children needs to be expanded maybe later as arrays fix):
> (varobj.c:3153):	ASSIGN:	(ULONGEST to int)
> [ children = (type)->length / (target)->length]
>  - You already made some such patch, this issue should be addressed.
>    Currently the implementation cannot cope with high number of
> children but that is a different problem, GDB would overflow host
> memory.

Right, I did, but I wanted to stop myself from entering this
rabbit-hole too :)

> [ slacklen = typelen & 1] UNSAFE_ALLOCA:
> (xstormy16-tdep.c:288):	FUNC(C_alloca):	(LONGEST to
> size_t)	[typelen + slacklen]
>  - Yes, fix it, please.

Already fixed and committed.

> FIXED(Expand n):
> (xtensa-tdep.c:1886):	VARINIT(n):	(LONGEST to
> int)	[info->length]
>  - info->length could be just ssize_t; +swap the initialization lines:
>      info->length = TYPE_LENGTH (arg_type);
>      info->contents = value_contents (arg);
>    Then also this n would be just ssize_t.
>    Just a nitpick.

OK.

> LOC breakpoint.c:15874 (breakpoint.c:15873)
>  Function observer_attach_memory_changed expects arg 1 to be
> observer_memory_changed_ftype * gets [function (CORE_ADDR, LONGEST,
> bfd_byte *) returns void]: invalidate_bp_value_on_memory_change ***
> SAFE
>  - Again LONGEST len could be just ssize_t.

OK
 
> LOC findcmd.c:184
>  Conditional clauses are not of same type: (val_bytes) (LONGEST),
> (sizeof(int64_t)) (size_t) *** SAFE
>  - Missing some ENSURE.

OK.

> LOC i386-nat.c:531 (i386-nat.c:553)
>  Conditional clauses are not of same type: (max_wp_len - 1) (int),
> len - 1 (LONGEST) *** WATCHPOINT
>  - I see no problem there now.  i386 does not support large HW
> watchpoints, it just needs to reject gigantic (>4GB) LEN requests.

The separate watchpoints patch will have done this already.


Regards,
Siddhesh