From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id PPK5LEQ+iWB3IgAAWB0awg (envelope-from ) for ; Wed, 28 Apr 2021 06:51:48 -0400 Received: by simark.ca (Postfix, from userid 112) id A7BDC1F11C; Wed, 28 Apr 2021 06:51:48 -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 310AA1E01F for ; Wed, 28 Apr 2021 06:51:47 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id A68BA3938C24; Wed, 28 Apr 2021 10:51:46 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org A68BA3938C24 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1619607106; bh=RsqasphvznOL7tovHebphZdlNoIdWPLzGi1FZQySqKk=; 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=GMzF2maT5aRtLw3qxWOni2BAWEe7NQhrD66C9eqedQgYcNNahJ/PlQAG5E1QFB5HK BAb3+RpvMNc39UVWdOLcGeFKZdS6CRwMg7c+BkZCvBSXykqTJ+VtUHtDtTK/pxUOXl fhYV/Y1XfuY/fvGUnuBuHG9NxWkTqJiNbkWPHBOE= Received: from NAM10-BN7-obe.outbound.protection.outlook.com (mail-bn7nam10on2059.outbound.protection.outlook.com [40.107.92.59]) by sourceware.org (Postfix) with ESMTPS id A1766382E82E for ; Wed, 28 Apr 2021 10:51:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org A1766382E82E ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=d8tIaMLLnH4FBSdL3haO7YWciUV6ZwgkRXjCqJoy28CVKir7QTKqFX0HhtY17hPmyCYIORpMjvpZorNw1+sq3UAa5rn7wzKOBCuGXs0Y2G6aqLm8/S0GGd2fcfQxuMVq6yl8ZYpC68nOKrKssi1yleoq7WB5P/S7iQSKvy227B++RU+4UScDJCDVBzyJ1JqflNSuZiotHIAglktDq0eS2KK0zUaNZOKcSLjHEBcoTWME0B0OKls6qHaOwMkJpfXSMwDEKKNmq+jYBvwHNuWlBz9dfT6avV6o9RZPgmvYvcmI1Bu5i3r9GhAihZO38rfc20TzkZw9zxsIaiVnVIOfTQ== 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=RsqasphvznOL7tovHebphZdlNoIdWPLzGi1FZQySqKk=; b=AJYYsmKLUSh4fOfTL+UGge0YsMyV4FmPS50bdC3Pv9GNfv7KHYduzelqt+cUWR1a8iTz/PbyWI+M33FvgS4MHtKtBPlM0WLMmRWDmxYuzKoi5uacw4fdijltUb/QVN2v8wQWeO+07/y8B9VYfAdi9yTykpQ+QC2bU0lVZaskQ2MQk+4tcSCIGKdchKSAR84lZOOrnt0B+WaR2u/ixdOz6hg5IJABqVM2XJRts/H0WGsy4XS6xbR6fCRP9HVbFxWnOtRrKac/JTXbJBLdlXR1bEVYmE7rFcMgQwaUU6v8TpIWEAS+Dc8jVbd9qhmdCXDQaK/N1WYuQstS1TP3GkkDJw== 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 DM6PR12MB4467.namprd12.prod.outlook.com (2603:10b6:5:2a8::14) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4042.16; Wed, 28 Apr 2021 10:51:41 +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 10:51:41 +0000 Subject: Re: [PATCH 03/43] Move frame context info to dwarf_expr_context To: Simon Marchi , gdb-patches@sourceware.org References: <20210301144620.103016-1-Zoran.Zaric@amd.com> <20210301144620.103016-4-Zoran.Zaric@amd.com> <7f00dde3-c7dc-f839-bf8d-035f18d65ce1@polymtl.ca> Message-ID: <0edee4c1-9a04-9bf5-7d44-0973c675462b@amd.com> Date: Wed, 28 Apr 2021 11:51:35 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.9.1 In-Reply-To: <7f00dde3-c7dc-f839-bf8d-035f18d65ce1@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: LO2P265CA0188.GBRP265.PROD.OUTLOOK.COM (2603:10a6:600:a::32) 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 LO2P265CA0188.GBRP265.PROD.OUTLOOK.COM (2603:10a6:600:a::32) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4087.27 via Frontend Transport; Wed, 28 Apr 2021 10:51:40 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 5b5542a2-a686-4432-b0fc-08d90a339b2b X-MS-TrafficTypeDiagnostic: DM6PR12MB4467: 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: bOjK/ijvwv9EZt3JlbHjTOfj6+lWQiD92fpVafT2QYO7l8fOqOOkvoKZyOqfxXzEEGDInzxoOC1+nZmdGuc49PM5LleyXPCS75gW/4lkyTCF2CQBlegOy1Rw5KCxd8CBFRdFDeOpjulrqvLxfmFPxu/fmVQDhdENvBsrYuihk7K9p9xFYmA44KC4K83J3H/CjHmUCf8T6Jc6WuOk8T/N0Do1D6W+1b36PJSgaJCdaiqlydWMAfVEGATLU74q5WPFo53grPRL9j5BZux5SfeY5A0UmBxfwiNcoTw2gccvo1L/p+5wFpx0X3OYBkXFFQhIHN0ZVLaBouKzO09AlAdOWk1eXt0VVRsfzEFOz2qhQr3uJB9Pse8D5q1w1q2jSo69GW6xMOx1GSM2oZU1W+JbAMTVO6V74NpZ+n5FaOssa0DVIJ6Nvq9iHnUbw/inUPfla24/aAMT6EODvot8wPnHXD8q/6eqE3oKbhSfxSKpVAWxZMW0jSNBfTutMOFRJjdzRsUJS+CV5hSklXeIpybcOYWbZ49DiPF+9COT+6HLHAeS30RWONZD/Ld5x6nfRO7q+BuR3SbW4rVCXZtCYNr4ZoqNKqnRv+j6+kwYqaQqhXC7aCb9KEeSoF3a7ejkmhvsm7394IU+5tVYmOLU7AOouBl7ZfI9zXDR2F268Wg/EIkl/zNW02ZDRk0B3Z/RDBgh 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)(39860400002)(376002)(366004)(136003)(346002)(396003)(66476007)(53546011)(36756003)(316002)(2616005)(16526019)(83380400001)(2906002)(86362001)(6486002)(31686004)(31696002)(478600001)(38100700002)(5660300002)(186003)(8676002)(66946007)(8936002)(66556008)(6666004)(52116002)(43740500002)(45980500001); DIR:OUT; SFP:1101; X-MS-Exchange-AntiSpam-MessageData: =?utf-8?B?RzhZb20rczZzakFnKzdwMWtqR3RsTW5hdFNKaTZlUDBSV3lxckk3TlY5QnNJ?= =?utf-8?B?aVA5TEVMak5OajdnaFp4d25YUVJaOGd2ODYyWWpHRTZCZ1NnOUl3SWM4REg3?= =?utf-8?B?TDNGYXVnWGpYaHZGYlBnSVNiUW5WSzdKaVFmS2ZaM05pZEVIeU1ra2Q3L1FQ?= =?utf-8?B?cDFROXpRUEZramVxRXgxb2dWM2xLY0tHMnZsU3JVMjZ2cm5EcnZxUnAzd05Z?= =?utf-8?B?TkFndmIvYm1meVE4ajdSaEc2amhxeGVvNlg0RUJETXlTUFNPSnhEQU95V2Yw?= =?utf-8?B?RHlvbm90ZU5wRmJOWHcyMHJOWVpmT001VDAyTDdLbUJIMjNuM1VGcjk5RzBz?= =?utf-8?B?QTFpL0ZTYTQwTWR1ZTA4Y043M2xSeVMvNDM1aEt1bVd5cXBQVmc3WGtBZTlC?= =?utf-8?B?VTk2c1hrRkNVQndSUER1WjREV2dBVWhSZGR5VVhtRWx4WVhtWENzMm9ZRXpl?= =?utf-8?B?TG15LytnMHFvcllWTjVWSE5rK21OOFV2ZjNNRStoNStDOEtZbURWMGxXV0ls?= =?utf-8?B?RW85VzVSNjBZajFOUVVvazBOUXlNTVUyYUhxTmhnY2lyTkdyVTRyTVVtN3lp?= =?utf-8?B?bHFXYkxnR1VrcEc2YWNxK2RPdmEzYm95SXBoV2JKUm9veGJuMjVjeTZVNktM?= =?utf-8?B?cUhpbVFnb0VPNXlFRVFhYkNYMHBQNUVZRzRwN3B1SWFXWkhMYmhPOFk3MlhB?= =?utf-8?B?Qy9UaUF2VkFBckN2VmhCTDE0dzRrQnJxRmZ6Q0tTV2RjNjdVMFJWM1pMQS80?= =?utf-8?B?aTE0Y2JuUnZ0Umlmd1F0UmVYTjQxWGdEbFRRZ3crUWQwVUVseWpQMWdSdzlR?= =?utf-8?B?bTVjWDFtZDNERlJlRmlLUXZQdE1GWXFMclFucVkzemZ2Z0ttRm9TRUFLZFdK?= =?utf-8?B?eWo4TFlHRjlQU3p3dXZyVVAxdFI5TkNtVVRub2JzV29jbHhTd0x2RXBGN0pm?= =?utf-8?B?RXBSMTlONm1lRTBWN2V4Nlo0NVc3cFNiMGxSRmIxaE12TlFENnJ6N09NTGxx?= =?utf-8?B?SG1EZVk5ZmFZYW5IS05oL0ZXTDBqTkJKOWQyNngwQWUxa2VCd1RIc3dRd00r?= =?utf-8?B?UWZFY3plNDEwakNkS3BtYXE5TS9tcmdKZEdTZzl5dmVuZEJ0aSt2ZVZwOS9P?= =?utf-8?B?bk40anNVV0hDOUxqU0ZkTTBpU3FZRzJkRENSRWpST3RnK1BENW5Uc0ZkaDVG?= =?utf-8?B?U2lFRU9WcFFEeGVzemJNQzFCZUNvdGpZTTB2TWVDa1hKV1pENEhDZWcydWhU?= =?utf-8?B?d00xMnpWWG1iM3NDMnJOTXRLSEc4Sm13d0RXQk5HSXlnSzNKdURnang4MXpC?= =?utf-8?B?MVQ2MVp1bEJqNVZHV21WalhWOHRXanMwQTlPc2JuMkJzRExvdHlOVk1iR3Bm?= =?utf-8?B?d0pDOGtPNWhIZ0ZsVXV0UW1xck10R2tjc2FJOGZGSW5pZVl3U01rY1QzRU1P?= =?utf-8?B?WC9ZT1JmRC8zU1hzamJSVXhHNFVhcnNyZGdzcS9ZakVHeGMzRWgzc0d6SVQ1?= =?utf-8?B?Z0pxbllacjVHNFRxR2JSNHNBeTFiYUpJQUlIemJhOG93YTlvQXFKT2xmMk5i?= =?utf-8?B?OGtUV28yU29GdVVnODNsbmJ2KzJFdUNwM0xoWFFzY3V6L2g4SmhlK1EwVSt0?= =?utf-8?B?UW9EVkZKQWNWS215M1hncnNhZG9aYWNCZmgrbmtxdjJkTTF6TmdWQ3NCZmJj?= =?utf-8?B?ck1YNHVBSTJaVE9kRCt2d2dzUGltK2NEMHI0SUNNNVZQeHpnMHF0ZE5oWHJl?= =?utf-8?B?cFp0WCtqNGhKU3FhM01CMTBvaFA1d1I4Z0RzRlBrWVZiaWRMU1diY0hqTjQ0?= =?utf-8?B?R2Izc1V4b3Z1MzJrSExFQ3dsTzFTdUJQZVkzenNmNmdvRUtFVHpDeUpWODNj?= =?utf-8?Q?CeASNExLENtzC?= X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-Network-Message-Id: 5b5542a2-a686-4432-b0fc-08d90a339b2b X-MS-Exchange-CrossTenant-AuthSource: DM6PR12MB2762.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Apr 2021 10:51:41.3616 (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: +6xVHKQLCdH+tp57PB4/mDBZmDH/8OTr/mK9ScPwNMW9ZXk5uSY4XdVZYzNcyRCC/BjB3EQkcW4hik+NJCUlMA== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB4467 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" On 4/27/21 3:19 AM, Simon Marchi wrote: >> >> This patch starts the merging proccess by moving the frame context > > proccess -> process Thanks, I will fix it in the next iteration. > >> @@ -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? We did discuss this and at the time the feeling was that we want to avoid adding new exception types, then I realized that the GENERIC_ERROR error doesn't work after all so I chose the error that made the most sense and was already expected by the evaluator callers for other scenarios. I wouldn't be against adding a new exception type if the feeling is that it is justifiable. Actually, I would prefer it to make the solution cleaner. > > 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. True, but then you end up with even more "switch" statements in different parts of the gdb that one needs to update every time they add a new operation and this is one of the problems with the current design as is. This would force us to add two new places (one for frame and one for compilation unit) and there will be more in the future when we add a concept of a thread to the context. At least with the current implementation the implementer of the case statement that process that operation needs to know which part of the context has to be present for that operation, which is clearly defined in the DWARF standard as part of that operations definition (or at least it will be after our extensions). Another problem with the new class approach is that it makes the information about what each operation needs convoluted and hidden which in my mind adds even bigger chance of someone forgetting to add their operation to the query calls. I don't think that there is a good way to avoid this problem. People that work on the evaluator changes need to pay close attention to what that operation requires. > >> @@ -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. Thanks, I will fix it in the next iteration. > >> @@ -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? Thanks, I will fix it in the next iteration. > >> + >> + 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. Thanks, I will fix it in the next iteration. Zoran