From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id ncA+Iq50h2AedgAAWB0awg (envelope-from ) for ; Mon, 26 Apr 2021 22:19:26 -0400 Received: by simark.ca (Postfix, from userid 112) id 7CF891F11C; Mon, 26 Apr 2021 22:19:26 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-0.7 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,RDNS_DYNAMIC,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (ip-8-43-85-97.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 630AD1E01F for ; Mon, 26 Apr 2021 22:19:25 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 00CD03844012; Tue, 27 Apr 2021 02:19:25 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 00CD03844012 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1619489965; bh=slB35NhlCuTjRWpgrPJuTPy49HYFsqqSKkvAxjwsvBU=; 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=Iwu8hCcHXx57Qyxjwz88JU2nbxyZGXdDnYpSlWI8occhjKEliWytRDelJ9TvthL66 qobmTVfCjUcoGDQSgneC4dlhx6ByyXW7Shbq38HlU4ikmrdZ6f2Yp6IxcM+FuBOP02 7blIF/gOhItHXW/2dL04J+FG5Vj0ULPDVP0PnJxA= Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id F220F3844012 for ; Tue, 27 Apr 2021 02:19:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org F220F3844012 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 13R2JF4r013708 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 26 Apr 2021 22:19:19 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 13R2JF4r013708 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 B0A8E1E01F; Mon, 26 Apr 2021 22:19:14 -0400 (EDT) Subject: Re: [PATCH 03/43] Move frame context info to dwarf_expr_context To: Zoran Zaric , gdb-patches@sourceware.org References: <20210301144620.103016-1-Zoran.Zaric@amd.com> <20210301144620.103016-4-Zoran.Zaric@amd.com> Message-ID: <7f00dde3-c7dc-f839-bf8d-035f18d65ce1@polymtl.ca> Date: Mon, 26 Apr 2021 22:19:14 -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-4-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 Tue, 27 Apr 2021 02:19:15 +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" Hi Zoran, This patch was already ok'ed by Tom, but I have some more suggestions below. Nothing that compromises the general idea of the patch though. On 2021-03-01 9:45 a.m., Zoran Zaric via Gdb-patches wrote: > From: Zoran Zaric > > Following 15 patches in this patch series is cleaning up the design of > the DWARF expression evaluator (dwarf_expr_context) to make future > extensions of that evaluator easier and cleaner to implement. > > There are three subclasses of the dwarf_expr_context class > (dwarf_expr_executor, dwarf_evaluate_loc_desc and > evaluate_for_locexpr_baton). Here is a short description of each class: > > - dwarf_expr_executor is evaluating a DWARF expression in a context > of a Call Frame Information. The overridden methods of this subclass > report an error if a specific DWARF operation, represented by that > method, is not allowed in a CFI context. The source code of this > subclass lacks the support for composite as well as implicit pointer > location description. > > - dwarf_evaluate_loc_desc can evaluate any expression with no > restrictions. All of the methods that this subclass overrides are > actually doing what they are intended to do. This subclass contains > a full support for all location description types. > > - evaluate_for_locexpr_baton subclass is a specialization of the > dwarf_evaluate_loc_desc subclass and it’s function is to add > support for passed in buffers. This seems to be a way to go around > the fact that DWARF standard lacks a bit offset support for memory > location descriptions as well as using any location description for > the push object address functionality. > > It all comes down to this question: what is a function of a DWARF > expression evaluator? > > Is it to evaluate the expression in a given context or to check the > correctness of that expression in that context? > > Currently, the only reason why there is a dwarf_expr_executor subclass > is to report an invalid DWARF expression in a context of a CFI, but is > that what the evaluator is supposed to do considering that the evaluator > is not tied to a given DWARF version? > > There are more and more vendor and GNU extensions that are not part of > the DWARF standard, so is it that impossible to expect that some of the > extensions could actually lift the previously imposed restrictions of > the CFI context? Not to mention that every new DWARF version is lifting > some restrictions anyway. > > The thing that makes more sense for an evaluator to do, is to take the > context of an evaluation and checks the requirements of every operation > evaluated against that context. With this approach, the evaluator would > report an error only if parts of the context, necessary for the > evaluation, are missing. > > If this approach is taken, then the unification of the > dwarf_evaluate_loc_desc, dwarf_expr_executor and dwarf_expr_context > is the next logical step. This makes a design of the DWARF expression > evaluator cleaner and allows more flexibility when supporting future > vendor and GNU extensions. > > Additional benefit here is that now all evaluators have access to all > location description types, which means that a vendor extended CFI > rules could support composite location description as well. This also > means that a new evaluator interface can be changed to return a single > struct value (that describes the result of the evaluation) instead of > a caller poking around the dwarf_expr_context internal data for answers > (like it is done currently). > > This patch starts the merging proccess by moving the frame context proccess -> process > @@ -56,6 +57,31 @@ dwarf_gdbarch_types_init (struct gdbarch *gdbarch) > return types; > } > > +/* Ensure that a FRAME is defined, throw an exception otherwise. > + > + Throwing NOT_AVAILABLE_ERROR error so that a client can chose > + to react differently if the evaluation ended because there > + was a missing context information. */ > + > +static void > +ensure_have_frame (struct frame_info *frame, const char *op_name) > +{ > + if (frame == nullptr) > + throw_error (NOT_AVAILABLE_ERROR, > + _("%s evaluation requires a frame."), op_name); > +} I don't remember if we discussed about that or not, so I'll ask: "available" in GDB terminology usually refers to the tracing functionality. While tracing, you can choose which registers and what part of the memory to collect when hitting a tracepoint. During later analysis, if you try for example to print the value of a variable that requires some information (register or memory) that you didn't collect, we'll say that this register or memory (and therefore the variable's value) is not available / unavailable. It is also used with the "record" command / concept, where some things are recorded during execution to be able to step backwards in time, that's pretty much the same as tracing. If you step backwards in time and try to access an information that wasn't recorded and an exception is thrown as a result, it will be NOT_AVAILABLE_ERROR. So I am a bit worried that by using NOT_AVAILABLE_ERROR to say "to evaluate this expression, you need a frame, but there is no frame in the current context", we are overloading this exception type / return code. I can imagine that I could be inspecting a trace, I try to print a variable whose location expression needs to be evaluated. Evaluating the location expression requires accessing some memory or register that wasn't recorded in my trace, and that results in throwing NOT_AVAILABLE_ERROR. That would be a different error than trying to evaluate an expression that requires a frame in a context that doesn't have a frame. So it seems to me like a new exception type would be desirable. Maybe INSUFFICIENT_CONTEXT_ERROR? On an unrelated topic, things like ensure_have_frame always worry me because I'm scared I'll forget to call it where necessary. An alternative could be to make sure we get the frame through a getter that throws if frame == nullptr. Basically, a tiny class (could be internal to dwarf_expr_context): struct frame_safe { frame_safe (frame_info *frame) : m_frame (frame) {} frame_info *frame (const char *op) { if (m_frame == nullptr) ... throw ... return m_frame; } private: frame_info *m_frame; }; dwarf_expr_context would have a `frame_safe m_frame` field and a `frame (const char *op)` getter that just does `return m_frame->frame (op)`. This way, it's impossible to get the frame and forgetting an ensure_have_frame call. > @@ -857,7 +928,7 @@ dwarf_expr_context::execute_stack_op (const gdb_byte *op_ptr, > if (this->location == DWARF_VALUE_MEMORY) > result = fetch_address (0); > else if (this->location == DWARF_VALUE_REGISTER) > - result = this->read_addr_from_reg (value_as_long (fetch (0))); > + result = read_addr_from_reg (this->frame, value_as_long (fetch (0))); This line is slightly too long. > @@ -259,8 +247,23 @@ struct dwarf_expr_context > void add_piece (ULONGEST size, ULONGEST offset); > void execute_stack_op (const gdb_byte *op_ptr, const gdb_byte *op_end); > void pop (); > + > + /* Return a value of type TYPE, stored in register number REGNUM > + of the frame associated to the given BATON. "the given BATON" is a bit confusing, I'm not sure which BATON this refers to. Could you try to improve this comment? > + > + REGNUM is a DWARF register number. */ > + struct value *get_reg_value (struct type *type, int regnum); > + > + /* Return the location expression for the frame base attribute, in > + START and LENGTH. The result must be live until the current > + expression evaluation is complete. */ > + void get_frame_base (const gdb_byte **start, size_t *length); > }; > > +/* Return the value of register number REG (a DWARF register number), > + read as an address in a given FRAME. */ > +CORE_ADDR read_addr_from_reg (struct frame_info *, int); Can you please add the names to the parameters? It's a bit confusing otherwise, since the comment refers to them. Simon