From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id 2lZfMwBHiWAMIwAAWB0awg (envelope-from ) for ; Wed, 28 Apr 2021 07:29:04 -0400 Received: by simark.ca (Postfix, from userid 112) id C5D0E1F11C; Wed, 28 Apr 2021 07:29:04 -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,MSGID_FROM_MTA_HEADER,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 7FEEB1E01F for ; Wed, 28 Apr 2021 07:29:03 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id E3B853939C33; Wed, 28 Apr 2021 11:29:02 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org E3B853939C33 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1619609342; bh=sOAfz0CoZ49ZO6vhxczg4lvKTN74lclX46viUY9ztHU=; 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=sP0o/7X9/yacMByosW2UuDLL14OF3S+aKVGSzH55/wRcj8KTKaALWirM6kvZtoNon PIocE8+LHVaY8Pzv0N2JwpI+fovJDIR4teQWgNS0WPd4JERPtBTYPeSbU33vpUUtKs huBqEM4N9Zd00QlgQ9ujifMaz1mPUVUJ8dksXBMw= Received: from NAM12-DM6-obe.outbound.protection.outlook.com (mail-dm6nam12on2054.outbound.protection.outlook.com [40.107.243.54]) by sourceware.org (Postfix) with ESMTPS id 39F133857005 for ; Wed, 28 Apr 2021 11:29:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 39F133857005 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Bj3/K814ny9ff+DhJaQjDFJSeGe0Nma3MPzNt/Vy1mtj/5Ish+GxG5VqrhG/14Q9wzy+jnvX1gT6/EVrTpBYA0qhDGj8KoeNum9+zLh8LKDyfJ3pS9BZPNM1sK9Wr+/VZIbfvOgpn+piRPBsmsG/C5Dw+HdtC2BorCXe0r2gFkCjXyCtAV5D2JQvtssa/U1EmH5Bl9VQYt5QHZCMtZnOOjfeQLEdsXTlJlVbY3FHAH/+ySaP/OqGFMfgEBxAiKH1yJtSsJExt44vY7jiY/0ahzSq/QNbhB5cLlyD7Jl2Vu/sq5vqcqE5sG6XtxSri48Sv6pJNZS29o7sZUNfnqlTJg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=sOAfz0CoZ49ZO6vhxczg4lvKTN74lclX46viUY9ztHU=; b=EaJZfXdyGwfJQhupC/GdId32KajZB9iGvjnGsLt1nxocmIpLJnQjqEDWx7LplNgAROPJB+9ouyVNF/B2ziHq0JeNOAJSfoMysvoUUN4rLReqbMBfVR6UqGzB5YCfrPAguyV3Oulaccf4aiD2nCvu1aTxSybGRU/yogCMWbhLD+zuDQKfBKu1LknqBJMkRJ/RrZzgEes4Px9nZWRlc0uiHHJZfSVtAMmvud31F4aXmaNkY6gk/iFy+n+qlsV1dQBAtj3Dtjzmnnzilsw7mJxGvUlSlep/3DiTpNFq5tj2C94NmaN8QWgUWCEluGvPQOHravUnIKYdt+IOyEJ0Pkucaw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=amd.com; dmarc=pass action=none header.from=amd.com; dkim=pass header.d=amd.com; arc=none Received: from DM6PR12MB2762.namprd12.prod.outlook.com (2603:10b6:5:45::15) by DM5PR12MB1370.namprd12.prod.outlook.com (2603:10b6:3:76::21) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4065.21; Wed, 28 Apr 2021 11:28:58 +0000 Received: from DM6PR12MB2762.namprd12.prod.outlook.com ([fe80::49d0:1ee5:47ef:e0e5]) by DM6PR12MB2762.namprd12.prod.outlook.com ([fe80::49d0:1ee5:47ef:e0e5%7]) with mapi id 15.20.4065.023; Wed, 28 Apr 2021 11:28:58 +0000 Subject: Re: [PATCH 05/43] Move compilation unit info to dwarf_expr_context To: Simon Marchi , gdb-patches@sourceware.org References: <20210301144620.103016-1-Zoran.Zaric@amd.com> <20210301144620.103016-6-Zoran.Zaric@amd.com> <0f9cd647-7eef-0d22-875f-dc2bacd8ebe3@polymtl.ca> Message-ID: <602ac716-b8b5-789d-85c7-75c829315bec@amd.com> Date: Wed, 28 Apr 2021 12:28:53 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.9.1 In-Reply-To: <0f9cd647-7eef-0d22-875f-dc2bacd8ebe3@polymtl.ca> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [2a00:23c7:5a85:6801:f5b0:44e8:2b3e:76c1] X-ClientProxiedBy: LO4P123CA0126.GBRP123.PROD.OUTLOOK.COM (2603:10a6:600:192::23) To DM6PR12MB2762.namprd12.prod.outlook.com (2603:10b6:5:45::15) MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 Received: from [IPv6:2a00:23c7:5a85:6801:f5b0:44e8:2b3e:76c1] (2a00:23c7:5a85:6801:f5b0:44e8:2b3e:76c1) by LO4P123CA0126.GBRP123.PROD.OUTLOOK.COM (2603:10a6:600:192::23) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4087.25 via Frontend Transport; Wed, 28 Apr 2021 11:28:58 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: d06cfabb-3233-4956-39ad-08d90a38d0ad X-MS-TrafficTypeDiagnostic: DM5PR12MB1370: X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:10000; X-MS-Exchange-SenderADCheck: 1 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: aX2lMrMF+psRELRmW+OwE+SDFprq5HFYkQvvJDuEyOmrCQ+6qFVt79NtXIpi24uH549l/nmP0spl2GpMcUwt9kRfaopfnCTF4AO2Fw1ewH9tUcGDesvOpSW4SNQqvGxRXtAFrQcSvACAz2bTLA0eCE4WScZ+PEUDx1ZdWyV88xd2aMK0dj7+HX+jDLkFK8ZuzjtUsu3/kcZQ0x0xVuHpTu7WwSeJLuOPD28CpZST9gqBkqyuQ2CyyG8Dtlj63sNvjDYTxCnFslusZbJO5dDdNzc6j0ChHNJN+KNBr2Vc/x5kY/bl0B6HomibntJvBF2Jf5HbekPBHG4LSLklMTi7i61lyN5qKzjnWDxJXYOxSzEleClmCxvVLDSOF3qcNXZqPHMDiyW0cD5Bb2qvgwzSf9xqBpv2HVDdYi5zbABJk6zXzD27MJ1T/8oyJh065etA52JY8vG9nYVqUgfykmc1Gfjxn8C0qU8m+JElNW1hMrIklw9TVOSqtkT+TTNVvHn+3OADErpbQxaZzYhts7x/DziT5oWVvyuOzO+d6KxojtEeyJzCj9XgZ/QOiJ4Me/uLTOF1iY/ZPYBz5xUw8YXM74RIb74ffXfABmn1n/OKxTdDVHyqZS2w8ia6LmM7HExMkUDzlAvS+V6Q3k7WarerZVbJbA7Spvq5istJO03rTEtIB5wJ6IGHbsLFlPTWjk5v X-Forefront-Antispam-Report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:DM6PR12MB2762.namprd12.prod.outlook.com; PTR:; CAT:NONE; SFS:(4636009)(136003)(396003)(366004)(376002)(346002)(39860400002)(2906002)(38100700002)(66476007)(478600001)(6486002)(8936002)(186003)(316002)(16526019)(83380400001)(66946007)(36756003)(66556008)(6666004)(8676002)(31696002)(2616005)(86362001)(31686004)(5660300002)(52116002)(43740500002)(45980500001); DIR:OUT; SFP:1101; X-MS-Exchange-AntiSpam-MessageData: =?utf-8?B?azR6b2p6TlkyeUNIdk9mMCtTcTZ1Mk83TjZMS0NzUFM0T3h1NlZwOVpKSVBv?= =?utf-8?B?dERQcFk0QTJPRVVvamRRYThubm4vbHg5ZGYvYnFJKzExYUxTS2F6cnBjS0J6?= =?utf-8?B?cFlHOEN1bEhqRjhIZTc2T2tWUFNWYUxzMWx1cXRCZFZ1TWwyK3dTR2w5Sy9l?= =?utf-8?B?R0RIaGZSOXRNUGphREllTzBvVFhtWTQwUE51d29uTURFSDJTbFVCUkFoZ1hS?= =?utf-8?B?emdQcmp3Qk9vRXpCK1hkeGVOQ2dGYmg3RWpZOWJzdndPYXBYa083SnlSb0xX?= =?utf-8?B?MTFBT3ZoVlA4ZXkzcUNDSmdTR0xLYlkwcm5QbDUzbllJZDJIZUJxS09MMDdN?= =?utf-8?B?Y3Jxek5GOGRialNJZG9rdXh4bmtvWXAyblVLZTJVdlpITFNKVnBJSklrbm9M?= =?utf-8?B?YmJQcVhDZ0wzSDVZS2lrdEM3NjZrZGJKMERxbFo1RGV2bWswUHNJREIzRXpn?= =?utf-8?B?QWU0RTR5aEZHMFI0ejUxY0R5UllnUUIraFFaSW02eWc1UHczdnFyam5HOTQx?= =?utf-8?B?aU5kRVRZRC9Db1pFL0dNUi9rR0UxbHYxZ202L3JTL1ZUaFp4YjJETk14Z1dM?= =?utf-8?B?NEZ1bmdIbHZ3U1NNbTJXWk1jcit5SG00NGV4KzIxVWU5M2NTaDRLV3NFWiti?= =?utf-8?B?SmFKMWJ2S0J4OWFaN0RnTUt4T1Rlc25tU04wM0xSN0RFSkhDL2ZhTzRzYk1T?= =?utf-8?B?RTBCZmNqZDJrVzNCWTdjUWRGeDEzb2F4S3ZIY0x2TjhYanBXKzdFV2ZTYWI2?= =?utf-8?B?V0xxZFVyT09xNWI1NVI0NTR5bE8rSnlvak1hU2lmbnllOVZYSHByS2xsaDFG?= =?utf-8?B?ZjZCa1FtaXFaWDRWQVFhNU5Ea29KOFpnTHkrYnNxci9kMlJydVhJeTlUcVd1?= =?utf-8?B?U3NvZTl2dWg5eHdJWklUeHJUUDBEY2JkZHNmRXJNdzBGUHZiWHVObS9hVVlE?= =?utf-8?B?d3ZSRjBsWm9PQTBSbnhnODhMR3EwRHNDeHZzeVZ4SFBYbURsQzY0eUFScHBo?= =?utf-8?B?UmhkL1NIdVBaMlY0ZktMZU9uT1FMOXRraTFlM3g0Sk9rQW9OdFJiU1ZWdzJt?= =?utf-8?B?T1RCeStYOWQ4R2ZlazFnb0pqck1Ga0kxcDFBalpiN2s1V3o5NEliRHVEYURi?= =?utf-8?B?RzRCRStBVGcvUkYvbUFxblppUFh3cHIvNmhoNjdCbXArN2dQVGFvTy9icHZv?= =?utf-8?B?aWNKNlNGNUJNcXd0Z1BnL0xIM3Q5SlIrZFQ5VHI3QXdQKytrWXg4bThhNVla?= =?utf-8?B?N2NncWhpWk9pM0I4cHNsUW5UYVE0TWN0R21sdVhVYnNQUXJQSlFsb2FOdlp4?= =?utf-8?B?MjZ2UERycFpwM1ZLYmlnREdqa1JTVUwwNU1XWm9GTnV6UDZNVHcvMmJrMm5O?= =?utf-8?B?enpEK1ZUdlU5OWl1SG1PQmphNTIvYkw4ekl2cDNaMWZWVDJIRFhaeDlDL1h5?= =?utf-8?B?VzVEWTEwUHFxZm1Penpnd1l4MGVkNk5VR1Q4aEc3VFYwYVI4K3AzeENNWFh5?= =?utf-8?B?cnFFRkpaNGZReVZiR3pCM2ZwTFpmSXF3V2hMQkt2SEJtT0dta2lnclFRUmpy?= =?utf-8?B?MFFtNGdvSEt2OUQ3d1o3MDVwRnlsdCtTUTdTUktPcWdmRW5JZFc2K2xWREls?= =?utf-8?B?Zmx1MmFzU2hwak5Qb2ZTUXc4Z2hCQkRIc0pQMWl0dnhuZFZaQXdnOTBaWDRa?= =?utf-8?B?ZkthQ3RhK2VJYktaVWxXUXdhdVYzVkxCUDJmay9SSFlha08zL0hMZDcyckc2?= =?utf-8?B?MXdOYnpmbHl1MzBSd1ZrbTBrOVgwWEx6SitYZXZMRjVmcnRPN082N2ozSTlT?= =?utf-8?B?VHg3UWdLNVZCVGxZcXcvUW8zL3gycnZ3YVFlVEFZUTFnNkUxKzViQ0srSXNh?= =?utf-8?Q?axyB9D+WPIbHG?= X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-Network-Message-Id: d06cfabb-3233-4956-39ad-08d90a38d0ad X-MS-Exchange-CrossTenant-AuthSource: DM6PR12MB2762.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Apr 2021 11:28:58.5945 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: G1G9PtHFk2eP75eRgOpZxJHc6d/InJ48PLKQEb+yXClPg9bzKCxp3YaBH2SX+eTSpTqciaDcRjB1ZmE2b4N7/A== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR12MB1370 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: Zoran Zaric via Gdb-patches Reply-To: Zoran Zaric Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" >> +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). Same comment as for the frame side. I am not convinced that this is a better approach for the reasons mentioned in my previous response. > >> + >> /* 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. As I said, this approach can make things convoluted and difficult for someone to extend in the future. How are they supposed to know if they should use has_per_cu method or the getter method? The ensure_* functions are not part of the class interface so they are only used where the standard requires them to be used. All other places just use the class member in the current design. > >> + >> + 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. Thanks, I will change it in the next iteration. > >> + >> + 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. To be honest, this is not part of my implementation. I just copied it from the loc.c file and inlined one existing functions into another during the merging process. Considering that it is mostly used by the GNU specific operations (therefore orthogonal to my implementation), and that there is no official spec that defines those operations, I am reluctant to change that part of the code. It was already there previously so it shouldn't be a problem to keep it as is after my patch series. > >> @@ -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 > Yes, this would be one benefit of the class approach. But nothing really prevent us to always use the per_cu object, considering that all the places that use the ref_addr_size information called the ensure_have_per_cu function before that. There are many places in gdb that currently use the same logic for the validity of the passed in frame information. Some don't even do that much, but that is an issue for some other discussion. I am still not convinced that this is a better way to go. Both approaches have downsides I guess... Zoran