From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 120654 invoked by alias); 14 Sep 2017 16:13:32 -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 120643 invoked by uid 89); 14 Sep 2017 16:13:32 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=pushes X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 14 Sep 2017 16:13:31 +0000 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4D84EC0587ED; Thu, 14 Sep 2017 16:13:30 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 4D84EC0587ED Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=palves@redhat.com Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 78DC85D9C1; Thu, 14 Sep 2017 16:13:29 +0000 (UTC) Subject: Re: [PATCH] Make dwarf_expr_context::stack an std::vector To: Simon Marchi , gdb-patches@sourceware.org References: <1505404760-11844-1-git-send-email-simon.marchi@ericsson.com> From: Pedro Alves Message-ID: <4470557a-be38-3188-9076-2b70cc253b7a@redhat.com> Date: Thu, 14 Sep 2017 16:13:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1505404760-11844-1-git-send-email-simon.marchi@ericsson.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-09/txt/msg00388.txt.bz2 On 09/14/2017 04:59 PM, Simon Marchi wrote: > Replace the manually managed array with a vector. It is mostly > straightforward, except maybe one thing in execute_stack_op, in the > handling of DW_OP_fbreg. When the code stumbles on that opcode while > evaluating an expression, it needs to evaluate a subexpression to find > where the fb reg has been saved. Rather than creating a new context, it > reuses the current context. It saves the size of the stack before and > restores the stack to that size after. > > I think we can do a little bit better by saving the current stack > locally and installing a new empty stack. This way, if the > subexpression is malformed and underflows, we'll get an exception. > Before, it would have overwitten the top elements of the top-level "overwritten" > expression. The evaluation of the top-level expression would have then > resumed with the same stack size, but possibly some corrupted elements. One difference this causes is that before we're reuse the vector's internal memory buffer for the recursion, which may have been reallocated sufficiently already and not require any further reallocation, while with the patch, we must always heap-allocate the new vector's internal buffer when recursion pushes a value, and release it when recursion unwinds. I assume that it doesn't cause an observable timing difference, but, mentioning for completeness. A couple nits and this is fine with me. > /* True if the piece is in memory and is known to be on the program's stack. > @@ -114,7 +118,7 @@ struct dwarf_stack_value > struct dwarf_expr_context > { > dwarf_expr_context (); > - virtual ~dwarf_expr_context (); > + virtual ~dwarf_expr_context () = default; Couldn't we just remove the dtor? > > void push_address (CORE_ADDR value, bool in_stack_memory); > void eval (const gdb_byte *addr, size_t len); > @@ -123,11 +127,7 @@ struct dwarf_expr_context > bool fetch_in_stack_memory (int n); > > /* The stack of values, allocated with xmalloc. */ "xmalloc" reference is stale. Thanks, Pedro Alves