From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 38399 invoked by alias); 14 Sep 2017 16:31:14 -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 38388 invoked by uid 89); 14 Sep 2017 16:31:14 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 14 Sep 2017 16:31:13 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id v8EGV6kq018449 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Thu, 14 Sep 2017 12:31:10 -0400 Received: by simark.ca (Postfix, from userid 112) id 04AF11EB48; Thu, 14 Sep 2017 12:31:06 -0400 (EDT) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id 296471E5B9; Thu, 14 Sep 2017 12:31:05 -0400 (EDT) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Thu, 14 Sep 2017 16:31:00 -0000 From: Simon Marchi To: Pedro Alves Cc: Simon Marchi , gdb-patches@sourceware.org Subject: Re: [PATCH] Make dwarf_expr_context::stack an std::vector In-Reply-To: <4470557a-be38-3188-9076-2b70cc253b7a@redhat.com> References: <1505404760-11844-1-git-send-email-simon.marchi@ericsson.com> <4470557a-be38-3188-9076-2b70cc253b7a@redhat.com> Message-ID: <5027066a221a87007aa58fe495973258@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.0 X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Thu, 14 Sep 2017 16:31:06 +0000 X-IsSubscribed: yes X-SW-Source: 2017-09/txt/msg00389.txt.bz2 On 2017-09-14 18:13, Pedro Alves wrote: > 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" Done. >> 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. Good point. Though in this case, my opinion is that the correctness and safety is well-worth the cost. > 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? I thought that this was necessary because there are subclasses that might have destructors. But now that I think about it more, I suppose it would only be relevant if we destroyed instances of subclasses through pointers to dwarf_expr_context? All the instances of these classes are static. >> >> 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. Ok. Thanks, Simon