From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf1-x434.google.com (mail-pf1-x434.google.com [IPv6:2607:f8b0:4864:20::434]) by sourceware.org (Postfix) with ESMTPS id 8D7CE3858D26 for ; Sun, 16 Jun 2024 18:21:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 8D7CE3858D26 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 8D7CE3858D26 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::434 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1718562118; cv=none; b=TRKmI+92EinlG6zz7SJJDBqHRmuDr9p3y4CQ68ruRNkhLVsHaq52wtASj+g/pDNcMvOSZqqeWHpuqNwWGANdf28ZJXEDtU9bVwADr7p2WdWw9M8a6dub5UPdC74PSz+YEzSK8pLcbg0z9Od0hVnQpG2ox/FCIXtSQlNAuk4Ap0s= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1718562118; c=relaxed/simple; bh=iI8AM5VrdVhd1WVBavB/TfXf9/HPfIo7ptGs5H2X/Ds=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=Tamjng8Em4oE4dMCbNvI7EaWs9g3NNlid6O/s4CebHHjcldkBzr4i3kxRrU41V6TQ+3nyHYVVxkb13SPTYRfQfbfa+XvC2e+XfDufIzXcNwLzTvnJYrU8oiclhJPNEZl7AE+kQaAiHj4UQSR83t1L1Bp9TWKMAKVBUPoRMlzei0= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-pf1-x434.google.com with SMTP id d2e1a72fcca58-701b0b0be38so3390269b3a.0 for ; Sun, 16 Jun 2024 11:21:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1718562114; x=1719166914; darn=sourceware.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=aIMQ945Lhcz3gn0TY8au1sRaT0eGYCFvV0z3XhH0r7w=; b=RxojnbcZS9Yop0b7UOX1z/S3Oy6vEVUiUzpyofwRriqPj3+mkRfpC1AvXIGgpGGJKg sfWqgs7AU3mfIbUZA7+2ibniezj2B9aUEZhcIpO2vEwjrbN2VyrbUNEMVvXnJQp46TIQ KpaXiCm7KA/fPv9Jw5FNxcQQLlIsemkTdRFO1RjfRc/Scs3wqFHb7BCk4Morjzmebc1p VKkwQ54fAVoDCapCm7aG/sEgD7f5t4thWrQAJs+78ZWuCfuoybuaFPUYJBQQ4J1FuTot hREm4fMCfOLbI8y7wDBHE8Iz9rcUu7QVE1pdazGbYzSF08NXW+UwjA0M/eiZF8kR/Z1j E0PA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718562114; x=1719166914; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=aIMQ945Lhcz3gn0TY8au1sRaT0eGYCFvV0z3XhH0r7w=; b=tMc99I3ZFQEZaaQzhVotog4emRSuPfjZsNTse4Uo4GrbrumuikzmG7znGi+c8wPyMI 80WhJz3NENsni682bZDMkz6SziOasYqagyqYllGmETqy/cmEMXtanSZh39NUBiUBDWDZ PJMDuSa+QUNhzqtAMJWbhC+tlGmg4jAKaFAsxFhZbyNusfZCCQabcm3GBaIxnMI7+Hjk C0Zl2PBV/UFdnzf23U3qKMkMEXliJqrh1qDalPVzyo2OlPR/lyFczC1CKy3rzHWLZ4Ar uoLyIB057VLgbzBoWUj8ZWtiuN/dkhzWGy7pufDgtZ0b8hP4e42wxSomPB0ZyPLidNGx Vqug== X-Gm-Message-State: AOJu0YxU2ZSZjA4usptjZgbP/UsC5eFt3NiTGuuKrvu2VQo9jzHy2iMH 1P71emTS8ZawYCdW49wwb45zf8BxTc58uZb8qyWUTu9j5XvydC0M7t1qFoJQNUj355S3/o7f9X6 IKSgLvb+6ZPgAl2rSB3GLr5ddu34= X-Google-Smtp-Source: AGHT+IGfkHcohJe3Kn3Ulg2Cf7bTPRZgJ61+p2sKlBSyBRuKnwnWRsm2dTXYPEBOs8K1NOGsDolccNR1gJaQJD9jpx4= X-Received: by 2002:a05:6a20:d50d:b0:1ba:e88c:c4e7 with SMTP id adf61e73a8af0-1bae88cc8e6mr9540437637.30.1718562114385; Sun, 16 Jun 2024 11:21:54 -0700 (PDT) MIME-Version: 1.0 References: <20240531181558.492045-1-carlosgalvezp@gmail.com> <53e98fad-5bca-4d70-b943-18dcdde0149b@palves.net> In-Reply-To: <53e98fad-5bca-4d70-b943-18dcdde0149b@palves.net> From: Carlos Galvez Date: Sun, 16 Jun 2024 20:21:18 +0200 Message-ID: Subject: Re: [PATCH] [gdbsupport] Fix -Wenum-constexpr-conversion in enum-flags.h To: Pedro Alves Cc: gdb-patches@sourceware.org Content-Type: multipart/alternative; boundary="0000000000005a3edd061b05ecf5" X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,HTML_MESSAGE,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: --0000000000005a3edd061b05ecf5 Content-Type: text/plain; charset="UTF-8" Hi! Thanks for the response. The way forward requires potentially quite some refactoring and since it's my first ever contribution to the project I didn't feel confident in attempting it, plus some design discussions are probably needed. So I wanted to try and leave things as unchanged as possible, hopefully better documented in case future maintainers come across it. I also don't feel super confident in the testing since I don't get deterministic results (even on trunk). The main reason for putting up the patch is to unblock the Clang team from turning the warning into a hard error (as required by the Standard). Regarding "future work", I have some ideas: *# Idea 1* (quick, more like a workaround) Do something like: flags = flags & ~static_cast(IS_PARENT_DEFERRED); Then there's no integer promotion. There's only a handful of places where this would be needed, so it should be doable. But it's extra verbose. It might also require some changes to operator~, I believe it only accepts enums (not ints) as input. Perhaps the cast can be put inside operator~ directly, to a sufficiently large type (std::size_t), that should probably work! I can put up a follow-up patch if wanted. *# Idea 2* (more work, but more proper solution) Remove the custom underlying_type altogether, and use std::underlying_type. For this, we need: 1) Make operator~ safe such that it can handle both signed and unsigned types. I haven't looked deeply because I haven't fully understood the issue nor have I found the quote in the Standard about implementation-defined/UB in this case. 2) Remove these types of requirements: CHECK_VALID (true, int, true ? EF () : EF2 ()) std::underlying_type would return unsigned int, not int. Do note: using enums of different types with ternary operator like above will be ill-formed in C++26: https://eel.is/c++draft/expr.arith.conv#1. 3 GCC is already throwing a warning: https://godbolt.org/z/eMv4551aM So eventually this code pattern will have to be removed from the codebase. It's hard for me to find out where in the codebase it's used and the impact of this, but it's good to know. Best regards, Carlos On Fri, 14 Jun 2024 at 20:28, Pedro Alves wrote: > Hi! > > Thank you very much for working on this. > > On 2024-05-31 19:15, Carlos Galvez wrote: > > This fixes PR 31331: > > https://sourceware.org/bugzilla/show_bug.cgi?id=31331 > > > > Currently, enum-flags.h is suppressing the warning > -Wenum-constexpr-conversion > > coming from recent versions of Clang. This warning is intended to be > made a > > compiler error (non-downgradeable) in future versions of Clang: > > > > https://github.com/llvm/llvm-project/issues/59036 > > > > The rationale is that casting a value of an integral type into an > enumeration > > is Undefined Behavior if the value does not fit in the range of values > of the > > enum: > > https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1766 > > > > Undefined Behavior is not allowed in constant expressions, leading to an > > ill-formed program. > > > > In this case, in enum-flags.h, we are casting the value -1 to an enum of > a > > positive range only, which is UB as per the Standard and thus not allowed > > in a constexpr context. > > > > The purpose of doing this instead of using std::underlying_type is to > detect > > cases where a C-style enum (unscoped, without underlying type) would > decay > > (be promoted) to a signed integer. > > > > The same can be achieved by explicitly forcing an integer promotion on > the > > enum, and evaluating its sign. > > > > This approach is more generic and it actually uncovers existing issues, > where > > enums with unsigned small integer underlying types (e.g. unsigned char) > are > > also promoted to signed integers, for example: > > > > flags = flags & ~IS_PARENT_DEFERRED; > > > > Where IS_PARENT_DEFERRED belongs to the following enum: > > > > enum enum cooked_index_flag_enum : unsigned char > > { > > ... > > IS_PARENT_DEFERRED = 16, > > }; > > > > > Here, the enum is first promoted to signed int before applying operator~ > on it, > > which is what we want to avoid. These should be fixed in a follow-up > patch, to > > keep this one small and focused. > > I think I would want to see or at least understand what you are proposing > to fix that. I'd say that it would be even better if this was a two-patch > series with a second patch to fix that issue. > > Thanks for all the standard references. I wish I had added something like > this originally, as I have to keep rediscovering why the original code > is the way it is whenever this comes up... > > > > + > > + Note: currently, we need to add an extra condition: > > + "!enum_has_underlying_small_integer". > > + > > + This is because we have the following code on trunk: > > + > > + flags = flags & ~IS_PARENT_DEFERRED; > > + > > + Where IS_PARENT_DEFERRED belongs to a enum like: > > + > > + enum enum cooked_index_flag_enum : unsigned char > > + { > > + ... > > + IS_PARENT_DEFERRED = 16, > > + }; > > + > > + Here, IS_PARENT_DEFERRED will _also_ be promoted to signed integer, > > + due to integral promotions on small integers: > > + > > + https://timsong-cpp.github.io/cppwp/n4140/conv.prom#1 > > + > > + This is undesirable, because operator~ will be applied on a signed > > + integer, which is what we are trying to avoid. > > + Future work should remove this condition and fix the relevant code. > > This is what I'd like to understand better, and if possible avoid adding > this "future work" commentary. What are you imagining would be the fixes > to the relevant code like? > > Pedro Alves > > > +*/ > > +template > > +struct enum_sign > > +{ > > + static_assert(std::is_enum::value, "T must be an enum"); > > + static constexpr bool value = > > + std::is_signed::type>::value || > > + (enum_will_be_promoted_to_signed_integer::value && > > + /* TODO: remove the condition below and fix affected code */ > > + !enum_has_underlying_small_integer::value); > > +}; > > + > > --0000000000005a3edd061b05ecf5--