From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-f46.google.com (mail-wm1-f46.google.com [209.85.128.46]) by sourceware.org (Postfix) with ESMTPS id 0E2923857C57 for ; Mon, 14 Sep 2020 11:56:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 0E2923857C57 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=palves.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=alves.ped@gmail.com Received: by mail-wm1-f46.google.com with SMTP id z9so10955660wmk.1 for ; Mon, 14 Sep 2020 04:56:42 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=Tl+STR9S2bknJG79kmvB6JtQeq0DlPJdI2WJt2Vo1M0=; b=f5kViDQG+kxU6MQf7cyQ5YEavCDId3I2sgrL1lRuJ/E3a+eE5pXDj2DrQIdCEYYfc5 8y6/3G6E1/dthi+2jO0XCyf+vCRzbYVvemlN45ZBm7PZjopsCkt7via7KEUjACGl1LXX AkCzVMXa584BwllqOtKiuwNNTo1Cw4+z2YjPQrTDycLYLXU4xqk4HBp1LCE9QaKYrpbD TJ5gjG9Jh9HYRX4j93v23Es1n7/t9NgJp/FZdUqGlBLcw5zGszPcRB0izKRljarKH8tj EpymUTrvyzY6rLF7eNUEKdI5M4BGKgjF0iwlYwM1TfAA/X9JdcAPD6UNlqHm3s7Mcvb2 cCrw== X-Gm-Message-State: AOAM530gg8zvag1wROzKwnXdYC+GsivtGCSsxIcAayOMSg3LVGgyc62n 7nJHEaHC+IXWThzDZnRMIlevCNIZBmp/RA== X-Google-Smtp-Source: ABdhPJzcaX41RIBuS2L9VnCw5t7WGE42aM3WCQRzNhLgUr+uBlelfBbosCa6tV3Id9H0f0kLK8kXWw== X-Received: by 2002:a1c:99c7:: with SMTP id b190mr14297095wme.44.1600084600108; Mon, 14 Sep 2020 04:56:40 -0700 (PDT) Received: from ?IPv6:2001:8a0:f905:5600:eefd:c63:53e0:3e8a? ([2001:8a0:f905:5600:eefd:c63:53e0:3e8a]) by smtp.gmail.com with ESMTPSA id m18sm12540252wmg.32.2020.09.14.04.56.38 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 14 Sep 2020 04:56:38 -0700 (PDT) Subject: Re: [PATCH 2/3] Use type_instance_flags more throughout To: Luis Machado , gdb-patches@sourceware.org References: <20200821144523.19451-1-pedro@palves.net> <20200821144523.19451-3-pedro@palves.net> From: Pedro Alves Message-ID: <77cf5b4b-4c61-880c-c6ad-c19b089bc89a@palves.net> Date: Mon, 14 Sep 2020 12:56:36 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-4.1 required=5.0 tests=BAYES_00, FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=no autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: Mon, 14 Sep 2020 11:56:43 -0000 Hi Luis, I should have replied to this promptly. Sorry about that. On 8/25/20 12:36 PM, Luis Machado wrote: > Hi, > > On 8/21/20 11:45 AM, Pedro Alves wrote: >> The next patch in this series will rewrites enum_flags fixing some API >> holes.  That would cause build failures around code using >> type_instance_flags.  Or rather, that should be using it, but wasn't. >> >> This patch fixes it by using type_instance_flags throughout instead of >> plain integers. >> >> Note that we can't make the seemingly obvious change to struct >> type::instance_flags: >> >>   -  unsigned instance_flags : 9; >>   +  ENUM_BITFIELD (type_instance_flag_value) instance_flags : 9; >> >> Because G++ complains then that 9 bits isn't sufficient for holding >> all values of type_instance_flag_value. > > I ran into the problem above the last time I used instance flags (address classes), but mostly because of the (rather silly) 9 bit (or whatever current number is there) limitation. > > Isn't this more an artifact of trying to save space where there isn't a need to anymore? Wouldn't a 64-bit integer suffice here? That would give us 64 flags worth of data and virtually no problems until we ran out of bits. > Well, I'm not exactly sure about struct type, but symbol related structures are space sensitive, a few bytes here and there can make a significant difference when you have thousands or millions of instances. Once in a while someone tries to shrink these structures, see if we can pack fields better by reordering fields, etc. Here's what we have today, on x86-64: (top-gdb) ptype /o type /* offset | size */ type = struct type { /* 0 | 8 */ type *pointer_type; /* 8 | 8 */ type *reference_type; /* 16 | 8 */ type *rvalue_reference_type; /* 24 | 8 */ type *chain; /* 32: 0 | 4 */ unsigned int align_log2 : 8; /* 33: 0 | 4 */ unsigned int instance_flags : 9; /* XXX 7-bit hole */ /* XXX 5-byte hole */ /* 40 | 8 */ ULONGEST length; /* 48 | 8 */ main_type *main_type; /* total size (bytes): 56 */ } And if we blindly made instance_flags 64-bit: (top-gdb) ptype /o type /* offset | size */ type = struct type { /* 0 | 8 */ type *pointer_type; /* 8 | 8 */ type *reference_type; /* 16 | 8 */ type *rvalue_reference_type; /* 24 | 8 */ type *chain; /* 32: 0 | 4 */ unsigned int align_log2 : 8; /* XXX 7-byte hole */ /* 40 | 8 */ ULONGEST instance_flags; /* 48 | 8 */ ULONGEST length; /* 56 | 8 */ main_type *main_type; /* total size (bytes): 64 */ } Maybe we will need 64 bits for flags in the future anyway, I don't know. > Honestly, I don't particularly like the hackery that is going on in traits.h, just to catch something that may be easier fixed with a longer integer type. > > I understand the C++ standard is evolving, but GDB's code base is still full of old code. We don't necessarily need to convert GDB's code base to use the latest and greatest out there, right? We just need to make it good quality and well maintained? The "hackery" is basically switching to the detection idiom that's now standard in C++17. We can remove our implementation in traits.h once we start requiring C++17. But the "hackery" is used in the enum flags unit tests -- even if instance_flags was made a plain 64-bits enum, I'd still want the "hackery". It's orthogonal to instance_flags. This patch could be the first in the series, before the traits.h change. In fact, I'll do just that. > > Just an opinion from someone that has used these flags before and has been bitten by their storage-space-saving ugliness. >