From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id t1jhM0zMiGC/FwAAWB0awg (envelope-from ) for ; Tue, 27 Apr 2021 22:45:32 -0400 Received: by simark.ca (Postfix, from userid 112) id C53811F11C; Tue, 27 Apr 2021 22:45:32 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-1.1 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id ECFAE1E940 for ; Tue, 27 Apr 2021 22:45:30 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 5545D3844023; Wed, 28 Apr 2021 02:45:30 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 5545D3844023 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1619577930; bh=FvLjDJfAw34NzFsvNhrfK4iVgX1wpqDO6B97uW4khJI=; h=Subject:To:References:Date:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=lei7n6C1pZWPADQSVyo+WwwJ13MeCXJ9jB0jLRxaStEwJrwrSxqILTGfDpYMQtBk7 QHQkRjlTV6ebqSjt74An4vylua5tQNXH4zf5ZkXLhsBHbjlTKgJhYooA/utfVvbwfM f4cQT0Wq2HnNiXxqUBtlodQz+t3CHDLaUSvVn3UU= Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 12187385481D for ; Wed, 28 Apr 2021 02:45:27 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 12187385481D 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 13S2jKJc003975 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 27 Apr 2021 22:45:25 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 13S2jKJc003975 Received: from [10.0.0.11] (192-222-157-6.qc.cable.ebox.net [192.222.157.6]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 5652E1E940; Tue, 27 Apr 2021 22:45:20 -0400 (EDT) Subject: Re: [PATCH 16/43] Simplify dwarf_expr_context class interface To: Zoran Zaric , gdb-patches@sourceware.org References: <20210301144620.103016-1-Zoran.Zaric@amd.com> <20210301144620.103016-17-Zoran.Zaric@amd.com> Message-ID: Date: Tue, 27 Apr 2021 22:45:20 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: <20210301144620.103016-17-Zoran.Zaric@amd.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Wed, 28 Apr 2021 02:45:20 +0000 X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Simon Marchi via Gdb-patches Reply-To: Simon Marchi Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" On 2021-03-01 9:45 a.m., Zoran Zaric via Gdb-patches wrote: > From: Zoran Zaric > > Idea of this patch is to get a clean and simple public interface for > the dwarf_expr_context class, looking like: > > - constructor, > - destructor, > - push_address method and > - evaluate method. > > Where constructor should only ever require a target architecture > information. This information is held in per object file > (dwarf2_per_objfile) structure, so it makes sense to keep that > structure as a constructor argument. It also makes sense to get the > address size from that structure, but unfortunately that interface > doesn’t exist at the moment, so the dwarf_expr_context class user > needs to provide that information. > > The push_address method is used to push a CORE_ADDR as a value on > top of the DWARF stack before the evaluation. This method can be > later changed to push any struct value object on the stack. > > The evaluate method is the method that evaluates a DWARF expression > and provides the evaluation result, in a form of a single struct > value object that describes a location. To do this, the method requires > a context of the evaluation, as well as expected result type > information. If the type information is not provided, the DWARF generic > type will be used instead. > > gdb/ChangeLog: > > * dwarf2/expr.c (dwarf_expr_context::dwarf_expr_context): Add > address size argument. > (dwarf_expr_context::read_mem): Change to use property_addr_info > structure. > (dwarf_expr_context::evaluate): New function. > (dwarf_expr_context::execute_stack_op): Change to use > property_addr_info structure. > * dwarf2/expr.h (struct dwarf_expr_context): New evaluate > declaration. Change eval and fetch_result method to private. > * dwarf2/frame.c (execute_stack_op): Change to call evaluate > method. > * dwarf2/loc.c (dwarf2_evaluate_loc_desc_full): Change to call > evaluate method. > (dwarf2_locexpr_baton_eval): Change to call evaluate method. > --- > gdb/dwarf2/expr.c | 49 ++++++++++++++++++++++++++++++++++++---------- > gdb/dwarf2/expr.h | 44 +++++++++++++++++++++++++++-------------- > gdb/dwarf2/frame.c | 18 ++++------------- > gdb/dwarf2/loc.c | 41 ++++++++++++-------------------------- > 4 files changed, 85 insertions(+), 67 deletions(-) > > diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c > index b2248681899..1645f477d74 100644 > --- a/gdb/dwarf2/expr.c > +++ b/gdb/dwarf2/expr.c > @@ -699,8 +699,11 @@ dwarf_expr_context::address_type () const > > /* Create a new context for the expression evaluator. */ > > -dwarf_expr_context::dwarf_expr_context (dwarf2_per_objfile *per_objfile) > -: per_objfile (per_objfile) > +dwarf_expr_context::dwarf_expr_context (dwarf2_per_objfile *per_objfile, > + int addr_size) > +: gdbarch (per_objfile->objfile->arch ()), If gdbarch is always obtained from the objfile, we could maybe avoid storing it. > + addr_size (addr_size), > + per_objfile (per_objfile) > { > } > > @@ -825,13 +828,17 @@ dwarf_expr_context::read_mem (gdb_byte *buf, CORE_ADDR addr, > return; > > /* Prefer the passed-in memory, if it exists. */ > - CORE_ADDR offset = addr - obj_address; > + if (addr_info != nullptr) > + { > + CORE_ADDR offset = addr - addr_info->addr; > > - if (offset < data_view.size () && offset + length <= data_view.size ()) > + if (offset < addr_info->valaddr.size () > + && offset + length <= addr_info->valaddr.size ()) > { > - memcpy (buf, data_view.data (), length); > + memcpy (buf, addr_info->valaddr.data (), length); Not related to this patch, but: I find it odd that the field `valaddr` contains the object's value. Why is it called like that? > return; > } > + } The indentation here is not right. > > read_memory (addr, buf, length); > } > @@ -874,8 +881,8 @@ dwarf_expr_context::push_dwarf_reg_entry_value > caller_frame); > scoped_restore save_per_cu = make_scoped_restore (&this->per_cu, > caller_per_cu); > - scoped_restore save_obj_addr = make_scoped_restore (&this->obj_address, > - (CORE_ADDR) 0); > + scoped_restore save_addr_info = make_scoped_restore (&this->addr_info, > + nullptr); > scoped_restore save_per_objfile = make_scoped_restore (&this->per_objfile, > caller_per_objfile); > > @@ -1043,6 +1050,28 @@ dwarf_expr_context::fetch_result (struct type *type, > return retval; > } > > +/* See expr.h. */ > + > +struct value * > +dwarf_expr_context::evaluate (const gdb_byte *addr, size_t len, > + struct dwarf2_per_cu_data *per_cu, > + struct frame_info *frame, > + const struct property_addr_info *addr_info, > + struct type *type, > + struct type *subobj_type, > + LONGEST subobj_offset) > +{ > + this->per_cu = per_cu; > + this->frame = frame; > + this->addr_info = addr_info; > + > + if (per_cu != nullptr) > + this->ref_addr_size = per_cu->ref_addr_size (); As mentioned in a previous message, I have the feeling that we could get rid of the ref_addr_size field and just call per_cu->ref_addr_size when we need it. It could always be a follow-up cleanup. > + > + eval (addr, len); > + return fetch_result (type, subobj_type, subobj_offset); This is just me, not an officiel style rule, when I like to use `this->` when accessing fields and methods, to make it clear what they are (not global variables or free functions). Although for fields prefixed with `m_`, I don't, because it's already obvious by the naming. > diff --git a/gdb/dwarf2/expr.h b/gdb/dwarf2/expr.h > index a0ac21f2ed1..d1374068732 100644 > --- a/gdb/dwarf2/expr.h > +++ b/gdb/dwarf2/expr.h > @@ -119,19 +119,30 @@ struct dwarf_stack_value > its current state and its callbacks. */ > struct dwarf_expr_context > { > - dwarf_expr_context (dwarf2_per_objfile *per_objfile); > + /* We should ever only pass in the PER_OBJFILE, while the ADDR_SIZE > + information should be retrievable from there. The PER_OBJFILE > + contains a pointer to the PER_BFD information anyway and the > + address size information must be the same for the whole BFD. */ > + dwarf_expr_context (struct dwarf2_per_objfile *per_objfile, > + int addr_size); I don't really understand the comment. What do you mean by "We should ever only pass..."? In general, I don't find that the comment helps me understand what the parameters are and do. Try to change it to be more concrete and straight to the point. > virtual ~dwarf_expr_context () = default; > > void push_address (CORE_ADDR value, bool in_stack_memory); > - void eval (const gdb_byte *addr, size_t len); > > - /* Fetch the result of the expression evaluation in a form of > - a struct value, where TYPE, SUBOBJ_TYPE and SUBOBJ_OFFSET > - describe the source level representation of that result. */ > - struct value *fetch_result (struct type *type = nullptr, > - struct type *subobj_type = nullptr, > - LONGEST subobj_offset = 0); > + /* Evaluate the expression at ADDR (LEN bytes long) in a given PER_CU > + FRAME context. Where TYPE, SUBOBJ_TYPE and SUBOBJ_OFFSET describe > + expected struct value representation of the evaluation result. > + The ADDR_INFO property can be specified to override the range of > + memory addresses with the passed in buffer. */ It sounds like you are missing a word between PER_CU and FRAME. The sentence starting with "Where" sounds strange, I'd remove the "Where" to make it a simple statement. It sounds like you are missing a "the" between "describe" and "expected". > + struct value *evaluate (const gdb_byte *addr, size_t len, > + struct dwarf2_per_cu_data *per_cu, > + struct frame_info *frame, > + const struct property_addr_info *addr_info = nullptr, > + struct type *type = nullptr, > + struct type *subobj_type = nullptr, > + LONGEST subobj_offset = 0); Note to self that ADDR + LEN would be good candidates to become a gdb::array_view later. > > +private: > /* The stack of values. */ > std::vector stack; If all these fields become private, it would be good to rename them to add the `m_` prefix. It could be done at the end (if not already done later in the series), to avoid the conflicts. Simon