From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27431 invoked by alias); 7 Sep 2012 10:52:57 -0000 Received: (qmail 27422 invoked by uid 22791); 7 Sep 2012 10:52:56 -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 10:52:42 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q87Aqg0o031977 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Fri, 7 Sep 2012 06:52:42 -0400 Received: from spoyarek (spoyarek.pnq.redhat.com [10.65.192.188]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q87Aqekv026919; Fri, 7 Sep 2012 06:52:41 -0400 Date: Fri, 07 Sep 2012 10:52:00 -0000 From: Siddhesh Poyarekar To: Jan Kratochvil Cc: gdb-patches@sourceware.org Subject: Re: bitpos expansion patches summary Message-ID: <20120907162158.0aee8e85@spoyarek> In-Reply-To: <20120902181515.GA9913@host2.jankratochvil.net> References: <20120805005350.150e5b74@spoyarek> <20120902181515.GA9913@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/msg00063.txt.bz2 On Sun, 2 Sep 2012 20:15:15 +0200, Jan wrote: > I was thinking about some updating of file:lineno records in > '.locdiff.report' file to their new location and then diffing the > old+reviewed '.locdiff.report' against new file. But I have not > tried it yet in practice. This will be heuristic to some extent, which is why I avoided it. > FIXED(Expand len): > (mips-tdep.c:4701): VARINIT(len): (ULONGEST to > int) [(arg_type)->length] > - You have fixed line 4646 but I do not see line 4701 to be fixed. Probably a case of different line numbers due to using a different updated version? I was rebasing every now and then, which gave me different line numbers for some warnings. I see this on line 4695 now. > FIXED(expand offset): (mips-tdep.c:5040): CMP: > (ULONGEST to int) [offset + xfer > (type)->length] > - I do not see a need for this change, it is all limited by: > TYPE_LENGTH (type) <= 2 * MIPS64_REGSIZE Right, reverted. > SAFE(stack_dest should > be CORE_ADDR, but safe for now): (mt-tdep.c:786): > VARINIT(stack_dest): (CORE_ADDR to LONGEST) [sp] > - There should be checked overflow of sp - length in general in these > *_push_dummy_call functions but passing >2GB structs by value on > stack is probably not worth fixing. > But I do not see why you did not choose CORE_ADDR for stack_dest Because it would change the sign, which is something we agreed we don't want to do for this patch. > UNSAFE_ALLOCA: (mt-tdep.c:852): > FUNC(C_alloca): (LONGEST to size_t) [typelen + slacklen] > - Just drop alloca, two write_memory calls are enough. This has already been replaced with xmalloc/xfree in cvs. But yes, two write_memory calls should have been enough. > UNSAFE_ALLOCA: (mt-tdep.c:853): FUNC(memcpy): (LONGEST > to size_t) [typelen] > - Just drop alloca, two write_memory calls are enough. Likewise. > FIXED(Expand startrest): > (opencl-lang.c:253): VARINIT(startrest): (LONGEST to > int) [offset % elsize] > - 'elsize' line is longer than 80 columns. One should check the > whole patch. (In these cases a new helper variable makes it easier to > conform to the GNU indentation style.) OK, fixed. > SAFE: (opencl-lang.c:428): FUNC(memcpy): (ULONGEST to > size_t) [(elm_type)->length] > - I miss here FIX of: opencl-lang.c:450 > int src_len; LONGEST lowb, highb; src_len = highb - lowb + 1; > It will affect also at least create_value(). This is called only for a vector, so highb - lowb should be bound by vector size. You made a point later about gcc possibly generating incorrect code and hence giving us a large value. Do you want me to expand this to cater for such a situation? > SAFE: (opencl-lang.c:681): > FUNC(memset): (ULONGEST to size_t) [(eltype1)->length] > - I miss here opencl-lang.c:895 where variable "i" should have been > expanded. int i; LONGEST lowb1, highb1; for (i = 0; i < highb1 - > lowb1 + 1; i++) Again, this should be limited by vector size. > SAFE: > (ppc-sysv-tdep.c:162): FUNC(align_up): (LONGEST to > int) [len] > - I would say at ppc-sysv-tdep.c:124 'len' it should be int->ssize_t > instead of int->LONGEST as there is already value_contents making the > checks somehow easier. OK, fixed. > SAFE(Vectors may not be larger than int): > (ppc-sysv-tdep.c:447): CMP: (LONGEST to int) [i > < len / 16] > - This is dependency on external producer (gcc). > GDB should verify it, such as ENSURE_SIZET or a proper handling. > External producer could incorrectly mark DW_AT_GNU_vector some > large type and GDB would process it incorrectly. OK, fixed. > REVERTED(vectors < 16 bytes): > (ppc-sysv-tdep.c:852): VARINIT(regnum): (LONGEST to > int) [tdep->ppc_fp0_regnum + 1 + i] > - it still could REVERT ppc-sysv-tdep.c:844 and ppc-sysv-tdep.c:848. OK. > SAFE(int > sufficient for OpenCL vectors): (ppc-sysv-tdep.c:897): > VARINIT(n_regs): (ULONGEST to int) [(type)->length / 16] > - While true again it is dependency on external producer (gcc). OK. > SAFE(OpenCL Vector): > (ppc-sysv-tdep.c:1448): CMP: (ULONGEST to int) > [i < (type)->length / 16] > - While true again it is dependency on external producer (gcc). OK. > FIXED(Expand byte): > (ppc-sysv-tdep.c:1561): CMP: (ULONGEST to int) > [byte < (type)->length] > - OK although ssize_t would be enough here. OK. > FIXED(Expand len): (ppc-sysv-tdep.c:1567): > VARINIT(len): (ULONGEST to int) [(type)->length - byte] > - OK although ssize_t would be enough here. OK. > SAFE: (ppc-sysv-tdep.c:1597): FUNC(write_memory): > (LONGEST to ssize_t) [len] > - OK although ssize_t would be enough here. OK. > > [(valtype)->length] SAFE: (printcmd.c:2269): > > VARINIT(param_len): (ULONGEST to UINT) > > [(param_type)->length] > - While it is technically right I would expand it here, at the point > where it is computed it is still not clear the type is > TYPE_CODE_DECFLOAT. I am actually more inclined to eliminate the single-use variable and use TYPE_LENGTH directly. What do you think? > SAFE: (p-valprint.c:212): > FUNC(xmalloc): (LONGEST to size_t) [length_size] > - Missing ENSURED_SIZET. Normally it cannot happen but broken > compiler can produce arbitrarily long length-of-string field which > this way could be incorrectly processed by GDB without an error. > - extract_unsigned_integer may also need similar handling as size_t > may be > int. OK, will do. > SAFE: (p-valprint.c:323): ASSIGN: (ULONGEST to > UINT) [ len = extract_unsigned_integer(valaddr + > embedded_offset + length_pos, length_size, byte_order)] > - Again, like above. OK. > [(type)->length] FIXED(Expand n): (s390-tdep.c:2516): > VARINIT(length): (ULONGEST to UINT) [(type)->length] > - I do not see 'n' anywhere, you have modified Looks like I missed this due to some misplaced comment. length needs to be expanded instead here. Fixed. > FIXED(Expand len, sh_justify_value_in_reg arg): > (sh-tdep.c:1097): ASSIGN: (ULONGEST to int) > [ len = (type)->length] > - Excessive ensure_type_fits_sizet in your patch at sh-tdep.c:1129 > because there is sh_justify_value_in_reg above it. I think I have removed this in a later version of the patch. > [(type)->length == 2 * reg_size] FIXED(Expand len, > sh_justify_value_in_reg arg): (sh-tdep.c:1234): ASSIGN: > (ULONGEST to int) [ len = (type)->length] > - Excessive ensure_type_fits_sizet in your patch at sh-tdep.c:1258 > because there is sh_justify_value_in_reg above it. Likewise. > - len : 0] FIXED(Expand len): (spu-tdep.c:1296): > VARINIT(len): (ULONGEST to int) [(type)->length] > - This is not needed, it has to fit in inferior registers. spu_push_dummy_call also calls spu_value_to_regcache with a type without checking the size of the type. > FIXED(Likewise): (spu-tdep.c:1321): VARINIT(len): > (ULONGEST to int) [(type)->length] > - Likewise. OK, fixed. > FIXED(Expand highest_offset, start, > print_frame_nameless_args arg 2): (stack.c:548): > ASSIGN: (LONGEST to LONG) [ highest_offset = > current_offset] > - current_offset/highest_offset could be split out, it is a > different bug. But fine with merged it here. I don't know what you mean by splitting it out - could you please elaborate? > FIXED(Expand len): > (tic6x-tdep.c:974): VARINIT(len): (ULONGEST to > int) [(arg_type)->length] > - references_offset is incorrectly not expanded. OK. > FIXED(Expand len, i): > (tilegx-tdep.c:221): VARINIT(len): (ULONGEST to > int) [(type)->length] > - not needed, it is used only if !tilegx_use_struct_convention OK. > FIXED(Expand len, i): > (tilegx-tdep.c:246): VARINIT(len): (ULONGEST to > int) [(type)->length] > - not needed, it is used only if !tilegx_use_struct_convention OK. > FIXED(Expand typelen): (tilegx-tdep.c:308): ASSIGN: > (ULONGEST to int) [ typelen = > (value_enclosing_type(args[i]))->length] > - For example stacklen and alignlen are incorrectly not expanded. OK. > UNSAFE_ALLOCA: (tilegx-tdep.c:347): ASSIGN: (ULONGEST > to int) [ typelen = (value_enclosing_type(args[j]))->length] > - Please fix as an unrelated patch; it should be also ENSURED Already done. > FIXED: (tracepoint.c:1485): FUNC(add_memrange): > (ULONGEST to ULONG) [len] > - Expansion of 'addr' is correct but unrelated. OK. I'll keep it as is since it is trivial anyway. > FIXED(Expand len): (v850-tdep.c:851): ASSIGN: (ULONGEST > to int) [ len = (value_type(*args))->length] > - Not needed to expand v850-tdep.c:893 due > to !v850_use_struct_convention by its caller. > - Not needed to expand v850-tdep.c:920 due > to !v850_use_struct_convention by its caller. OK. > SAFE: > (valarith.c:757): ASSIGN: (ULONGEST to int) > [ inval1len = (type1)->length] > - TYPE_CODE_STRING can be also >2GB, it is in fact a sort of array. > For example Fortran uses these. > Also it needs alloca expansion. OK will do. > SAFE: (valarith.c:758): ASSIGN: (ULONGEST to > int) [ inval2len = (type2)->length] > - Likewise. OK. > FIXED(Expanded put_frame_register_bytes arg 4): > (valops.c:1373): FUNC(put_frame_register_bytes): > (ULONGEST to int) [(type)->length] > - I do not see any expansion and I do not see why. > lval_register values can never be too large. OK, I missed it and I was right ;) > FIXED(Expand value_cstring arg > 2, highbound to LONGEST): (valops.c:1849): > VARINIT(highbound): (ULONGEST to int) [len / > (char_type)->length] > - value_cstring's parameter 'int->LONGEST len' should have been only > ssize_t. It cannot be larger than 'char *ptr' memory. OK. > size_t) [len] FIXED: (valops.c:1872): > VARINIT(highbound): (ULONGEST to int) [len / > (char_type)->length] > - Likewise for the 'len' parameter. OK. > SAFE: (valops.c:1891): > FUNC(memcpy): (ULONGEST to size_t) [(type)->length] > - While TYPE_CODE_BITSTRING is not used in practice I do not see why > it should not be expanded(=fixed). I don't get this. I think I have the wrong line numbers and hence not able to see what you're seeing. Regards, Siddhesh