From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6151 invoked by alias); 7 Sep 2012 11:10:01 -0000 Received: (qmail 6136 invoked by uid 22791); 7 Sep 2012 11:10:00 -0000 X-SWARE-Spam-Status: No, hits=-7.6 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,RP_MATCHES_RCVD,SPF_HELO_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 07 Sep 2012 11:09:44 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q87B9hS5004262 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 7 Sep 2012 07:09:43 -0400 Received: from spoyarek (spoyarek.pnq.redhat.com [10.65.192.188]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q87B9f29025013; Fri, 7 Sep 2012 07:09:42 -0400 Date: Fri, 07 Sep 2012 11:10:00 -0000 From: Siddhesh Poyarekar To: Jan Kratochvil Cc: gdb-patches@sourceware.org Subject: Re: bitpos expansion patches summary Message-ID: <20120907163859.7bf6a982@spoyarek> In-Reply-To: <20120904150325.GA24664@host2.jankratochvil.net> References: <20120805005350.150e5b74@spoyarek> <20120904150325.GA24664@host2.jankratochvil.net> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-09/txt/msg00064.txt.bz2 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