From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27297 invoked by alias); 16 Mar 2012 11:06:03 -0000 Received: (qmail 27229 invoked by uid 22791); 16 Mar 2012 11:06:00 -0000 X-SWARE-Spam-Status: No, hits=-6.7 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD 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, 16 Mar 2012 11:05:38 +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 q2GB5Flu017937 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 16 Mar 2012 07:05:15 -0400 Received: from host2.jankratochvil.net (ovpn-116-16.ams2.redhat.com [10.36.116.16]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q2GB5BAR022775 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Fri, 16 Mar 2012 07:05:13 -0400 Date: Fri, 16 Mar 2012 11:06:00 -0000 From: Jan Kratochvil To: Joel Brobecker Cc: gdb-patches@sourceware.org Subject: Re: [RFA/Ada] Crash when trying to set value of packed array element Message-ID: <20120316110510.GA25193@host2.jankratochvil.net> References: <1330556589-3594-1-git-send-email-brobecker@adacore.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1330556589-3594-1-git-send-email-brobecker@adacore.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes 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-03/txt/msg00598.txt.bz2 Hi Joel, On Thu, 01 Mar 2012 00:03:09 +0100, Joel Brobecker wrote: > Still, this was not sufficient, because it caused a regression when > trying to perform an aggregate assignment of a packed array of packed > record. The key element here is the nesting of packed entities. > Looking at the way ada_value_primitive_packed_val creates the value > of each sub-component, one can see that the value's offset is set > to the offset compared to the start of the parent. This was meant to > match what value_primitive_field does as well. > > So, with our array of records, if the record offset was 2, and if > the field we're interested in that record is at offset 1, the record > value's offset would be set to 2, and the field value's offset would > be set to 1. But the address for both values would be left to the > array's address. This is where things start breaking down, because > the value_address function for our field value would return the > address of the array + 1, instead of + 3. > > This is what causes the final issue, here, because ada-lang.c's > value_assign_to_component needs to compute the offset of the > subcomponent compared to the top-level aggregate's start address > (the array in our case). It depends on how is defined functionality of ada_value_primitive_packed_val. Whether its parameter OFFSET is relative to the start of the array or whether it is relative to the element. I believe you want the latter and your patch also implements the latter, otherwise I cannot imagine how callers would handle it correctly. In the latter case the parameter name OFFSET is confusing and it should be renamed as it is _added_ to value_offset (OBJ). In general I do not think we can ever fix it all on top of the current infrastructure. These OFFSETs, EMBEDDED_OFFSETs etc. should be dropped, there should be support for discontiguos set of memory snapshot associated with struct value (so that it supports for example even virtual method tables or slices of multidimensional Fortran arrays). I should find time for archer-jankratochvil-vla. > @@ -2310,18 +2309,22 @@ ada_value_primitive_packed_val (struct value *obj, const gdb_byte *valaddr, > > if (obj != NULL) > { > - CORE_ADDR new_addr; > + long new_offset = offset; Nitpick - LONGEST (Siddhesh Poyarekar is working on a more thorough fix). > } > > +void > +set_value_parent (struct value *value, struct value *parent) Missing function comment. Thanks, Jan