This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: bitpos expansion patches summary
- From: Siddhesh Poyarekar <siddhesh at redhat dot com>
- To: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Fri, 7 Sep 2012 16:38:59 +0530
- Subject: Re: bitpos expansion patches summary
- References: <20120805005350.150e5b74@spoyarek> <20120904150325.GA24664@host2.jankratochvil.net>
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