From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from NAM12-MW2-obe.outbound.protection.outlook.com (mail-mw2nam12on2080.outbound.protection.outlook.com [40.107.244.80]) by sourceware.org (Postfix) with ESMTPS id 0B3953858416 for ; Fri, 22 Oct 2021 16:38:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 0B3953858416 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=f3GFaWkl4y4hFUjJccBdStF8+LlczDNAp6iBtiTL+M+UCZ0CXlrJgqXrdS7S1lVIzZ+B9DUcB8CCIs4ys/MdEUHgrHIbNGti2uTNreOprSAXBJctenU3SRQL0cy5rtDQ09ql8EquWtAgoJL1mYLdiQIqU1yDlg14yFcVxuJ0+Ptc9+jkaVkf56TEpqz6pT8JKqgm8/85EXodQvLR1HH2boFabKThrCtSoMiSxC28H11iuxBWu86jlVt7GUey820gDXkmcGjCIj6hTaRMQOqpsBNXkfRL5eKHVIO82iDvyrQqPAlkemXzIWWiUcEnav3mGKvn6HSw9bqTaJgbLUeGiw== 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-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=ffQHvBzitdmW7Ymg0wLV/i6NufWCxWRuW2y7fs7H6q0=; b=BvAlRpmbI8JNE/Mw1qsZEOGUyZHkgSiYkRko6fuY9jH1rp+5dedycdQNr2CJTGjwOHvhLIidZhlk6cnqsTWPQBAzgo07Gcw31f0ssLjMjeGHxEn49TD0syoiEyG0zKYlFcJS1/mi20AW3Ex1NL/8j6iTGjP52cOr3W5HF8jPgutxc+rd1JktvXrXWNjigzNxBZy1EfiSdLla08i7rlAEIFSGkDC+RoWxpET+gZUoMdB5TEOaZrfaTugbcvMqFOtJDxTyf4mNYOmHD5BKijluHwe9EMPuQRYh4TdnkDifiD9PbmNY/86Vr1mfXwueTukrML+vJg4WzSF+rcjIkF1xDg== 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 BN9PR12MB5065.namprd12.prod.outlook.com (2603:10b6:408:132::12) by BN9PR12MB5036.namprd12.prod.outlook.com (2603:10b6:408:135::8) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4628.18; Fri, 22 Oct 2021 16:38:16 +0000 Received: from BN9PR12MB5065.namprd12.prod.outlook.com ([fe80::f908:e014:a94:a35]) by BN9PR12MB5065.namprd12.prod.outlook.com ([fe80::f908:e014:a94:a35%6]) with mapi id 15.20.4628.018; Fri, 22 Oct 2021 16:38:16 +0000 Subject: Re: [PATCH v3 03/28] Add new classes that model DWARF stack element To: Lancelot SIX Cc: gdb-patches@sourceware.org References: <20211014093235.69756-1-zoran.zaric@amd.com> <20211014093235.69756-4-zoran.zaric@amd.com> <20211021234253.3n5proehdawuwy4l@ubuntu.lan> From: Zoran Zaric Message-ID: Date: Fri, 22 Oct 2021 17:38:13 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 In-Reply-To: <20211021234253.3n5proehdawuwy4l@ubuntu.lan> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-ClientProxiedBy: LO2P265CA0487.GBRP265.PROD.OUTLOOK.COM (2603:10a6:600:13a::12) To BN9PR12MB5065.namprd12.prod.outlook.com (2603:10b6:408:132::12) MIME-Version: 1.0 Received: from [IPv6:2a00:23c7:1093:6301:6cb9:662e:c869:8142] (2a00:23c7:1093:6301:6cb9:662e:c869:8142) by LO2P265CA0487.GBRP265.PROD.OUTLOOK.COM (2603:10a6:600:13a::12) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4628.18 via Frontend Transport; Fri, 22 Oct 2021 16:38:15 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 26eb74c1-2949-4c4f-a1a0-08d9957a5913 X-MS-TrafficTypeDiagnostic: BN9PR12MB5036: X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:10000; X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: G2LILZjAsX7cOpbUtWsnG8MiewNjhWGMWRHEm1mA7esHm/aSoeSuFo7rA4jFypEWQMOsw0I9poAU8DEq6SjX2oiC3GR2l3fbhPKaah/Dxsjm+CF81yr/pnMOzTFCLr6IkDFiZGXHZzqe+Me8CTeyta+puJC/7xBWyJQ+NFDJPzS0PgwATIpOLVEH2KgFeHN0ULhVuTay+BKLFr4oQMHNPhbyYvnI8TyQSarJPkWMCiP9blqEBHyJrV3BSyVB0Ht3DTh/xRcvEJbfb+zyQ9LjXwWyS9Kln23xZE7ciy9yriUx8SaVbJk0CS1AEvw3hhWTqRnsrD2HXfTm9lv6pGGPHGyMAZhRDrVz/WujAznNG4adlkOo2xM9b0b5Z5g+yo4Sd6HYo6j4yfUdzCYZRP1VUzlNTLtDZHwZzbn6BZzjCSYGJl22arynP61gIgDPcD5IiN5ZBrrsZwBirzaGtQOGKh+bjnQBBioCUhTJj1MuOOdh9y6h8rUovu0zkNhgB+ginsadbwn+xSZgIl51ctAjkhkiOGFemW0kklL+Gz7X2LzN+rVJKQF9FmM3DQW+4B4zkO2dOvku2liWnSL8hNJDc7zAFxzEEfErp9sASMCGyGNjqoUGv7gH1h8467EYl3GgV95Sq69fPZqGRnJadTAFt3NrOGBwREEOWeaUcEb/m291l/MOtojsdqXJnOkq+Jf7q47oudvGPothXdX0oKajZCiXCj10+Ec8WuyyEP+weJs= X-Forefront-Antispam-Report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:BN9PR12MB5065.namprd12.prod.outlook.com; PTR:; CAT:NONE; SFS:(4636009)(366004)(8936002)(2616005)(44832011)(2906002)(36756003)(8676002)(31686004)(508600001)(38100700002)(66946007)(5660300002)(6486002)(31696002)(316002)(4326008)(66556008)(186003)(66476007)(6916009)(83380400001)(86362001)(43740500002)(45980500001); DIR:OUT; SFP:1101; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?S05BK1IrOFl2N1VhMXpPaXpoWDhvQXEvWURoWUd6Wit4QVlSdTZIdFc1YUho?= =?utf-8?B?SzFNMGFSdEJWN1llZHUzb0NaeDFrZG9rV0lVazR0NDg5MFIxMkkwYnhYYmcy?= =?utf-8?B?bFFEQjhybHBaQmZvc0lHUFhVZ0h3MjJUK2NUTjl4TEhUQXRubUEyL21uS1c1?= =?utf-8?B?d1lYK1RWSCt5YlVDNHZTNEdMMWszSXFjYldocnV3d1BweWJzTXNuTXplamlm?= =?utf-8?B?L1NkYk5VVy9Xd2tMc3FoaVByZHBVMzhRWkpQMm9MVzNhNkUybHpGYUs4eXF3?= =?utf-8?B?TFFTczE1RGNQTzZwLzU1a2NXZjdET2pmRHd1SERCa0FTN2hnSDlMVlRkWklP?= =?utf-8?B?Z29zMlVGTEI5WjVOU09odGNFcnFTSVZiQ2U3OUFKY3k1WGRNdWI3WHU0QWV1?= =?utf-8?B?S25HR25xZnpWOXlwRUxVcFZGV0xzTnhmNGhvYXJPdFY3WWdBOGExcCsxa05M?= =?utf-8?B?OHpHL0VENEdZZ29JUGVLZjU3eUFUaUpwSHoxTXFaYkd4VUFQRC9oWkRPZGNW?= =?utf-8?B?ZmhNTWIwbnlkTU9IOFdXVURmNTBnaWVRM1NwUHp1RWFSRzFMdm5VdDR3UHBU?= =?utf-8?B?T3FVWXprMFpFR0VQazRiUStSaWlGZ2YvWEg5dHl6QU5FbjNIUVIwSjFNVWEr?= =?utf-8?B?RlR1ckM3a0pKSDM1RzFnM3ZxaTdLZ1VPdUcxcEdURWZxM2xlazZYSkVRendX?= =?utf-8?B?d1ZpSW11bVZ0bjh2ZGlKN29Ydms4SkgxeW9Fb21yYjcxUnZvZXA1aWtXR2Yy?= =?utf-8?B?bUtCbDY3T2s3dHdCMXQ5Tm42SXVod3BFeDV5akRTdTVhM3BPajhObzRRNUM2?= =?utf-8?B?Zm1iUjdEWWhTa2lQVWswYVg1ZUxtQmtVaUhNVEVYZ2lCMHU4bzh0VVBXdnd5?= =?utf-8?B?dHV2amp3Mi85VnJmUVVVSVZLUktnVTltRDNlOTRmWCtabkdhZHNwUzhlZzBF?= =?utf-8?B?bnc3L3NXNk5RekpWWGVxbE9wdzNxQkIvMU5kKzV3ZktFTU13K1hSMVVIakd6?= =?utf-8?B?T1VHamE3WlNBYnRxUmRnaHlnMDUxandqcmMzQkh1ZXRxYTJFN3FQS2V4S29r?= =?utf-8?B?bmdNYVV6LzRJTWtkc2I1STRJdmdIeWF2WG84M0RGcGRSTkNrMnBmSlZGd3Fi?= =?utf-8?B?eURuT0NJeHVQQ1ZsOWdaQndIM1VpbFBNNjlyZ1Iva2RxeWZodVA3bUtaL1ND?= =?utf-8?B?L2ZIb3BVa2FaU2xaejd4VHlxWE4rTjBobVAzMDVFZGlENjRsYlB0T2o1VkFE?= =?utf-8?B?cmZMZWh1cXlubXpLRlJwTS9HbC9tUXNacUlQcDZtZ0E2WjNaSHNITU5KQmFn?= =?utf-8?B?R2YxU3RyNHE5cXVjNFlKOWtScmdNeWtFSU01cVBqUEliY2xkVGEvaERlbU54?= =?utf-8?B?MzY0R3oxcXoxbjVBeVhzNDNPNEYvQndxbU9HSDdFYjllY2NVQXVMYTJVMnFR?= =?utf-8?B?TStYN1ZTajl4TkFMMzUvWlJuL1VEQm5nVnFvSkdXMGZVVXN6WFc0ak9WK1NH?= =?utf-8?B?VndkYzNVcWtvYm5MM05yQzZlLy9scjRNeTZNd25KNjVBekFpUXlndlVUaGxi?= =?utf-8?B?RjdxbmVrS3VKMi93YWd6OGhsMGFreWhPNjMzMEl1bDR1T2FBbWF0OFJ1Mkcw?= =?utf-8?B?ZGlXN1A2TUhtVTFoME4zRWlWeXpZUXpNV3FCaUJuRWJCNG5rTjJNcDFaRDli?= =?utf-8?B?MUQ1UEZ4UmhXWkc4L1prc0xRU2FVckFHaXlHeUdIYis4YjZNYi9pMjdsMmxH?= =?utf-8?B?dEN0ZkRGbTZDMzd4cm10TlF6Y2I5dk9PcGxuWGloTGRhNW14VmI4M3ZlUXAz?= =?utf-8?B?dGI5dXhPN1ZOY1lhRTRqTTVodDdQKzlSS05ObHBudGlBOThLNHM2MFFiaXFj?= =?utf-8?B?WTJEcTRsS1orZFJBSG1qSE9RcS9iQmVyWWtoVkRCUmFRUndiUUNEQ1dtUGp4?= =?utf-8?B?dlM2Q0w0VlR3akpiZGVyT0JiRHM2ZDNBYW9XWVNDTE5ZSXBMcitwSFIwM2RI?= =?utf-8?Q?Ehe0VDDVQknAX+0a8lWPKk8jYHnm0c=3D?= X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-Network-Message-Id: 26eb74c1-2949-4c4f-a1a0-08d9957a5913 X-MS-Exchange-CrossTenant-AuthSource: BN9PR12MB5065.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Oct 2021 16:38:16.4515 (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: zorzaric@amd.com X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR12MB5036 X-Spam-Status: No, score=-6.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, MSGID_FROM_MTA_HEADER, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org 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: , X-List-Received-Date: Fri, 22 Oct 2021 16:38:21 -0000 Hi Lancelot, Thank you for reviewing my patches. I know that they are a bit big. > I am far from having reached the end of your series. I am not entirely > sure if I should send comments as I progress or go through all of it and > send all my comments at once when I have reached the end (which can take > a while). Is there a way you prefer? > > Best, > Lancelot. Maybe it is better to do it as you progress because that way I can fix things in the same order on my side. I will also let some time period to pass before I send the updated series, to give people time to comment on the current one. >> +class dwarf_entry >> +{ >> +public: >> + /* Not expected to be called on it's own. */ >> + dwarf_entry () = default; > > If this constructor is not meant to be called directly, I guest it could > be possible to make it protected. This way it can no longer be used by > a user of the class while still be usable by the child of this class. > Good point. >> + >> + virtual ~dwarf_entry () = default; >> +}; >> + >> +/* Location description entry found on a DWARF expression evaluation >> + stack. >> + >> + Types of locations descirbed can be: register location, memory >> + location, implicit location, implicit pointer location, undefined >> + location and composite location (composed out of any of the location >> + types including another composite location). */ >> + >> +class dwarf_location : public dwarf_entry >> +{ >> +public: >> + /* Not expected to be called on it's own. */ >> + dwarf_location (gdbarch *arch, LONGEST offset) >> + : m_arch (arch), m_offset (offset) >> + {} > > Same remark on the visibility of the constructor here (based on the same > comment). > Agreed. >> + >> + virtual ~dwarf_location () = default; >> + >> + /* Add bit offset to the location description. */ >> + void add_bit_offset (LONGEST bit_offset) >> + { >> + LONGEST bit_total_offset = m_bit_suboffset + bit_offset; >> + >> + m_offset += bit_total_offset / HOST_CHAR_BIT; >> + m_bit_suboffset = bit_total_offset % HOST_CHAR_BIT; >> + }; >> + >> + void set_initialised (bool initialised) >> + { >> + m_initialised = initialised; >> + }; >> + >> +protected: >> + /* Architecture of the location. */ >> + gdbarch *m_arch; >> + >> + /* Byte offset into the location. */ >> + LONGEST m_offset; >> + >> + /* Bit suboffset of the last byte. */ >> + LONGEST m_bit_suboffset = 0; >> + >> + /* Whether the location is initialized. Used for non-standard >> + DW_OP_GNU_uninit operation. */ >> + bool m_initialised = true; >> +}; >> + >> +/* Value entry found on a DWARF expression evaluation stack. */ >> + >> +class dwarf_value : public dwarf_entry > > It does not look like dwarf_value has child classes. It could be > declared final as you have done for dwarf_undefined for example. Agreed. > >> +{ >> +public: >> + dwarf_value (gdb::array_view contents, struct type *type) >> + : m_contents (contents.begin (), contents.end ()), m_type (type) >> + {} >> + >> + dwarf_value (ULONGEST value, struct type *type) >> + : m_contents (TYPE_LENGTH (type)), m_type (type) >> + { >> + pack_unsigned_long (m_contents.data (), type, value); >> + } >> + >> + dwarf_value (LONGEST value, struct type *type) >> + : m_contents (TYPE_LENGTH (type)), m_type (type) >> + { >> + pack_unsigned_long (m_contents.data (), type, value); > > Shouldn't it be pack_long instead here? It should, and it was in one of the previous patches. Copy paste mistake from my last iteration I guess. > >> + } >> + >> + gdb::array_view contents () const >> + { >> + return m_contents; >> + } >> + >> + struct type* type () const > > Minor remark first so I do not forget: I think GDB coding style prefers > 'type *' over 'type* ' (space before the '*', no space after). No matter how many times I go through these patches, I will still miss something... Thanks :) > > Unless there is a reason not to, I would be tempted to return a pointer > to const (i.e. const struct type *) in order to maintain the const > correctness here. > > One way out could be to have two versions of the type method: > > struct type *type (); > const struct type *type () const; > > I have tested this on top of the entire series, it seems to be OK. > I am fine with this change as long as there is no existing API that prevents me to use the const type *, and it seems that you already checked for that. I will make sure to fix everything in the next iteration. Zoran