From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id STKhNch9h2C5dgAAWB0awg (envelope-from ) for ; Mon, 26 Apr 2021 22:58:16 -0400 Received: by simark.ca (Postfix, from userid 112) id CE7691F11C; Mon, 26 Apr 2021 22:58:16 -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 E88EC1E01F for ; Mon, 26 Apr 2021 22:58:15 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 7EFD93857831; Tue, 27 Apr 2021 02:58:15 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 7EFD93857831 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1619492295; bh=+E+FuSne/pBLuzSk9fi0VZ+D3qPF589lEsktksfTqZA=; 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=dUxLBLASfAXa0lp9+f+MzyTbZ/f4uDE0VniJ6CfRmv1NLzwJ6wCWH+xalD8WxPtnN 8BhKhF1im7wHLTpdiRAR2C8YiH+khhWI6NEVG2NTUv6H/8A6tVVSTVB5XY8t7y7Hq3 iAoy6OXDbbJUTCCL9VLhSqeqR67B6ImjEQbzPLo8= Received: from smtp.polymtl.ca (smtp.polymtl.ca [132.207.4.11]) by sourceware.org (Postfix) with ESMTPS id 610EB3857812 for ; Tue, 27 Apr 2021 02:58:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 610EB3857812 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 13R2w401031856 (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 26 Apr 2021 22:58:09 -0400 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp.polymtl.ca 13R2w401031856 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)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id B2C451E01F; Mon, 26 Apr 2021 22:58:04 -0400 (EDT) Subject: Re: [PATCH 05/43] Move compilation unit info to dwarf_expr_context To: Zoran Zaric , gdb-patches@sourceware.org References: <20210301144620.103016-1-Zoran.Zaric@amd.com> <20210301144620.103016-6-Zoran.Zaric@amd.com> Message-ID: <0f9cd647-7eef-0d22-875f-dc2bacd8ebe3@polymtl.ca> Date: Mon, 26 Apr 2021 22:58:04 -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-6-Zoran.Zaric@amd.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Tue, 27 Apr 2021 02:58:04 +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 > > This patch moves the compilation unit context information and support > from dwarf_expr_executor and dwarf_evaluate_loc_desc to > dwarf_expr_context evaluator. The idea is to report an error when a > given operation requires a compilation unit information to be resolved, > which is not available. > > gdb/ChangeLog: > > * dwarf2/expr.c (ensure_have_per_cu): New function. > (dwarf_expr_context::dwarf_expr_context): Add compilation unit > context information. > (dwarf_expr_context::get_base_type): Move from > dwarf_evaluate_loc_desc. > (dwarf_expr_context::get_addr_index): Remove method. > (dwarf_expr_context::dwarf_variable_value): Remove method. > (dwarf_expr_context::execute_stack_op): Call compilation unit > context info check. Inline get_addr_index and > dwarf_variable_value methods. > * dwarf2/expr.h (struct dwarf_expr_context): Add compilation > context info. Remove get_addr_index and dwarf_variable_value. > * dwarf2/frame.c (dwarf_expr_executor::get_addr_index): Remove > method. > (dwarf_expr_executor::dwarf_variable_value): Remove method. > * dwarf2/loc.c (sect_variable_value): Expose function. > (dwarf_evaluate_loc_desc::get_addr_index): Remove method. > (dwarf_evaluate_loc_desc::dwarf_variable_value): Remove method. > (class dwarf_evaluate_loc_desc): Move compilation unit context > information to dwarf_expr_context class. > * dwarf2/loc.h (sect_variable_value): Expose function. > --- > gdb/dwarf2/expr.c | 55 +++++++++++++++++++++++++++++++++++++++------- > gdb/dwarf2/expr.h | 26 ++++++++-------------- > gdb/dwarf2/frame.c | 10 --------- > gdb/dwarf2/loc.c | 35 +++-------------------------- > gdb/dwarf2/loc.h | 8 +++++++ > 5 files changed, 67 insertions(+), 67 deletions(-) > > diff --git a/gdb/dwarf2/expr.c b/gdb/dwarf2/expr.c > index 6b5cba6b7b4..5a0a4a4299c 100644 > --- a/gdb/dwarf2/expr.c > +++ b/gdb/dwarf2/expr.c > @@ -72,6 +72,20 @@ ensure_have_frame (struct frame_info *frame, const char *op_name) > _("%s evaluation requires a frame."), op_name); > } > > +/* Ensure that a PER_CU is defined and 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_per_cu (struct dwarf2_per_cu_data *per_cu, const char* op_name) > +{ > + if (per_cu == nullptr) > + throw_error (NOT_AVAILABLE_ERROR, > + _("%s evaluation requires a compilation unit."), op_name); > +} Same comment as earlier about NOT_AVAILABLE_ERROR, and about wrapping per_cu to force accessing it through a getter that throws if it is nullptr (whatever we end up doing for the frame should be done here). > + > /* See expr.h. */ > > CORE_ADDR > @@ -201,6 +215,25 @@ dwarf_expr_context::get_frame_base (const gdb_byte **start, > start, length); > } > > +/* See expr.h. */ > + > +struct type * > +dwarf_expr_context::get_base_type (cu_offset die_cu_off, int size) > +{ > + if (per_cu == nullptr) > + return builtin_type (this->gdbarch)->builtin_int; Ok, so this seems to be a case where we need to check if per_cu is nullptr or not. The getter that throws won't work here, but there could be an `has_per_cu ()` method on the side. > + > + struct type *result = dwarf2_get_die_type (die_cu_off, per_cu, per_objfile); > + > + if (result == NULL) > + error (_("Could not find type for DW_OP_const_type")); Might as well change this NULL for nullptr. > + > + if (size != 0 && TYPE_LENGTH (result) != size) > + error (_("DW_OP_const_type has different sizes for type and data")); > + > + return result; > +} I find get_base_type a bit awkward. Do you think you could split it in two: - get_base_type - get_base_type_check_size ... where get_base_type_check_size uses get_base_type? The callers that don't need the size checking wouldn't need to pass 0. I also find it awkward that get_base_type hardcodes DW_OP_const_type in its error message, given that it's used in the context of many other operators. In the end we understand that DW_OP_const_type is the only case that passes size != 0, so it's the only case where that error can possibly be thrown. But if you could pass the op name as a parameter, like you did for the ensure_have_per_cu function, I think it would make things better. Or, since that size checking is only used when handling DW_OP_const_type, we could in-line that code in the caller and not bother with having get_base_type_check_size. > @@ -834,10 +873,7 @@ dwarf_expr_context::execute_stack_op (const gdb_byte *op_ptr, > case DW_OP_GNU_implicit_pointer: > { > int64_t len; > - > - if (this->ref_addr_size == -1) > - error (_("DWARF-2 expression error: DW_OP_implicit_pointer " > - "is not allowed in frame context")); > + ensure_have_per_cu (per_cu, "DW_OP_implicit_pointer"); > > /* The referred-to DIE of sect_offset kind. */ > this->len = extract_unsigned_integer (op_ptr, this->ref_addr_size, This case would be a bit weird with my suggestion of per_cu getter that throws if per_cu is nullptr, because nothing actually uses per_cu here. But, I think that in the end we could get rid of the `this->ref_addr_size` field, and use `this->per_cu ()->ref_addr_size ()`. Simon