Re: bitpos expansion patches summary

Siddhesh Poyarekar
2012-09-07T11:10:01Z

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