From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5437 invoked by alias); 14 Apr 2014 17:13:17 -0000 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 Received: (qmail 5423 invoked by uid 89); 14 Apr 2014 17:13:16 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Mon, 14 Apr 2014 17:13:15 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 4F0D3116102; Mon, 14 Apr 2014 13:13:13 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id Kmw4x-QIoXyH; Mon, 14 Apr 2014 13:13:13 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 2299A1160DD; Mon, 14 Apr 2014 13:13:13 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 8F241E02C0; Mon, 14 Apr 2014 10:13:18 -0700 (PDT) From: Joel Brobecker To: gdb-patches@sourceware.org Cc: Sanimir Agovic Subject: [vla v7 pushed] Re: [PATCH v6 00/15] Please have a final look Date: Mon, 14 Apr 2014 17:13:00 -0000 Message-Id: <1397495596-25364-1-git-send-email-brobecker@adacore.com> In-Reply-To: <0377C58828D86C4588AEEC42FC3B85A71D8507B7@IRSMSX105.ger.corp.intel.com> References: <0377C58828D86C4588AEEC42FC3B85A71D8507B7@IRSMSX105.ger.corp.intel.com> X-SW-Source: 2014-04/txt/msg00260.txt.bz2 > Keith, Joel, all, sorry for causing any inconvenience and thanks to > you both for taking care of the issue on Friday. No worries - it can happen to anyone. > The root cause was this code fragment below. It was in my repo and I > git-amend it accidently as I was playing around with an alternative to > solve the pointer-to-vla issue. > > @@ -2985,7 +2984,6 @@ evaluate_subexp_with_coercion (struct expression *exp, > { > (*pos) += 4; > val = address_of_variable (var, exp->elts[pc + 1].block); > - type = value_type (val); > return value_cast (lookup_pointer_type (TYPE_TARGET_TYPE (type)), > val); OK. That indeed solved the regressions outside of gdb.ada, so thanks a lot for that. It saved me a ton of time not having to investigate this issue. This did not solve, on the other hand, the regressions with gdb.ada, which turned out to be caused by a discrepancy between is_dynamic_type which considers references to dynamic arrays as being dynamic, and resolve_dynamic_bounds, which was handling the case of typedefs OK, but otherwise expected the type to be an array type via the following assert (which allowed us to find the bug, btw, so well placed one!): gdb_assert (TYPE_CODE (type) == TYPE_CODE_ARRAY); So, when you have code like the following (resolve_dynamic_type)... if (!is_dynamic_type (real_type)) return type; resolved_type = resolve_dynamic_bounds (type, addr); ... it is possible for a reference to dynamic array to get through the check and then be passed to resolve_dynamic_bounds. I ammended your patch as follow, essentially making resolve_dynamic_bounds handle reference types as well: | --- a/gdb/gdbtypes.c | +++ b/gdb/gdbtypes.c | @@ -40,6 +40,7 @@ | #include "cp-support.h" | #include "bcache.h" | #include "dwarf2loc.h" | +#include "gdbcore.h" | | /* Initialize BADNESS constants. */ | | @@ -1665,6 +1666,16 @@ resolve_dynamic_bounds (struct type *type, CORE_ADDR addr) | return copy; | } | | + if (TYPE_CODE (type) == TYPE_CODE_REF) | + { | + struct type *copy = copy_type (type); | + CORE_ADDR target_addr = read_memory_typed_address (addr, type); | + | + TYPE_TARGET_TYPE (copy) | + = resolve_dynamic_bounds (TYPE_TARGET_TYPE (type), target_addr); | + return copy; | + } | + | gdb_assert (TYPE_CODE (type) == TYPE_CODE_ARRAY); | | elt_type = type; I re-tested everything after making those changes, and everything seems to be clean, now, so I pushed your patch series. In the process, I had to fix a number of little nits, mostly related to the ChangeLog file and the revision logs. From memory: - ChangeLog: The entries, when still part of the commit, were not located at the start of the ChangeLog, and the date still refered to the 11th. It's easier, IMO, to manipulate commits without ChangeLog entries, and to add them at the very last minute, when just about to push to the official repo. But some tips on how to management have been also shared on the gdb@ mailing-list as well as on various blogs. - Some testsuite/ChangeLog entries were incorrect, missing the name of the gdb.[...] subdirectory in the file name, of mentioning it as the path to the (nonexistant) ChangeLog file. - The revision logs had traces of "Conflict:" sections, which are automatically added when you do a cherry-pick, I think, which results in a conflict that you resolve before commit. I removed those. I think that's about it. Let's hope that we're in the clear now! :) For the record, patch 01 and 02... [PATCH 01/12] type: add c99 variable length array support [PATCH 02/12] vla: enable sizeof operator to work with variable ... when individually tested on x86_64-linux, and then I only ran the testsuite after applying the remaining commits. But I did test each one of them in sequence last Friday, so I didn't see a reason to do it again. > > In addition I refactored the code for handling the sizeof operator to > match Keith latest fix for PR c++/16675 (245a5f0) in: > > #02 vla: enable sizeof operator to work with variable length arrays > > I used Jans diffgdb script to compare two runs (pre-/post vla) of 'make > check' and now the patch series is free of regression as it should be. > > You will find the latest patch series at: > > https://github.com/intel-gdb/vla/tree/vla-c99