From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 40DA83858D1E for ; Wed, 20 Sep 2023 19:23:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 40DA83858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1695237831; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=arA8g1y1H8Hr5Efqx/IR/LwTbQ0gdsBpDy0xh5NgWuI=; b=EOCWL2lCYkzn1HDpAClhNQDBk3yNg3KdMYIsG/kjr6LyrnSPPSTOXNcVRBBZGVWuGdg3do vYph7h1FL+2qcxWqtEQAnRcibCctcXJS5XtlALHNLx2pi57oQrZkMjxJXv+OCs0i5yKT9s yiP14Dah0MISpVpLiwtAUapad3F2ZlA= Received: from mail-qt1-f200.google.com (mail-qt1-f200.google.com [209.85.160.200]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-542-QMXcheEiOHSP2aJFZ9TvUg-1; Wed, 20 Sep 2023 15:23:50 -0400 X-MC-Unique: QMXcheEiOHSP2aJFZ9TvUg-1 Received: by mail-qt1-f200.google.com with SMTP id d75a77b69052e-41517126c99so1121651cf.2 for ; Wed, 20 Sep 2023 12:23:50 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695237830; x=1695842630; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=arA8g1y1H8Hr5Efqx/IR/LwTbQ0gdsBpDy0xh5NgWuI=; b=Uspz/RbWUij+o8cVOG4TtqlNrqQmp5c9SYJGDMgZeT/8azD8DOSBnA39bpO532ypiz PJ688rdY1IaeYxbjV209XxnmHSLNFjSTaHKXvloE+lAoe2om/0j02A3BPPGmKu6z8oaa ddMmUbgPzNWJEMSRUXEsANS8yvU2hWWL8Wcx5WqFldPaqVdSfSWBjdU48LMdfbeuW/rO zzRXL1foYxTwzO1fxwvJAyq+ZIAeZnGvbHbvlaDGQXnfASBEYh9OrZcCoHNbCmEDYrAP V6rq+AE8W7snmWSD146pU/JQQXqwl1lNx+mKwgXH0HR54GtQgyqz7GVjSbzbxX5XC/9y 73xQ== X-Gm-Message-State: AOJu0YzrALtHqPPDneoQOOsgor0mDSz2YKgDbQC1EBQLj/cOR5f2EVwK AFK9LSnopUoPZoEQ4jy6qn5YPf99LE0/3BG3MmNpD6wagrWezXrKrCIPuw8ZuemwKUDQxTp+42O 1pF0ADB7C8wc9SPHFrVY4JsDrKg== X-Received: by 2002:ac8:7c45:0:b0:412:4c9:93be with SMTP id o5-20020ac87c45000000b0041204c993bemr4051825qtv.64.1695237829800; Wed, 20 Sep 2023 12:23:49 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFcjIFgOGAlLuP2rkrr3UQ+4Gu4gQC0fd3I3blv96PIwJZQQ7bDJzMvEWrIRc8Xdsrwxlffpg== X-Received: by 2002:ac8:7c45:0:b0:412:4c9:93be with SMTP id o5-20020ac87c45000000b0041204c993bemr4051814qtv.64.1695237829527; Wed, 20 Sep 2023 12:23:49 -0700 (PDT) Received: from [192.168.1.108] (130-44-146-16.s12558.c3-0.arl-cbr1.sbo-arl.ma.cable.rcncustomer.com. [130.44.146.16]) by smtp.gmail.com with ESMTPSA id c11-20020ac84e0b000000b00405d7c1a4b0sm4756270qtw.15.2023.09.20.12.23.48 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 20 Sep 2023 12:23:48 -0700 (PDT) Message-ID: Date: Wed, 20 Sep 2023 15:23:47 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.15.1 Subject: Re: [PATCH v2] c++: Catch indirect change of active union member in constexpr [PR101631] To: Nathaniel Shead Cc: gcc-patches@gcc.gnu.org References: <053faf76-f918-7527-4a41-755a18d0018a@redhat.com> From: Jason Merrill In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-7.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP 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: On 9/19/23 20:55, Nathaniel Shead wrote: > On Tue, Sep 19, 2023 at 05:25:20PM -0400, Jason Merrill wrote: >> On 9/1/23 08:22, Nathaniel Shead wrote: >>> On Wed, Aug 30, 2023 at 04:28:18PM -0400, Jason Merrill wrote: >>>> On 8/29/23 09:35, Nathaniel Shead wrote: >>>>> This is an attempt to improve the constexpr machinery's handling of >>>>> union lifetime by catching more cases that cause UB. Is this approach >>>>> OK? >>>>> >>>>> I'd also like some feedback on a couple of pain points with this >>>>> implementation; in particular, is there a good way to detect if a type >>>>> has a non-deleted trivial constructor? I've used 'is_trivially_xible' in >>>>> this patch, but that also checks for a trivial destructor which by my >>>>> reading of [class.union.general]p5 is possibly incorrect. Checking for a >>>>> trivial default constructor doesn't seem too hard but I couldn't find a >>>>> good way of checking if that constructor is deleted. >>>> >>>> I guess the simplest would be >>>> >>>> (TYPE_HAS_TRIVIAL_DFLT (t) && locate_ctor (t)) >>>> >>>> because locate_ctor returns null for a deleted default ctor. It would be >>>> good to make this a separate predicate. >>>> >>>>> I'm also generally unsatisfied with the additional complexity with the >>>>> third 'refs' argument in 'cxx_eval_store_expression' being pushed and >>>>> popped; would it be better to replace this with a vector of some >>>>> specific structure type for the data that needs to be passed on? >>>> >>>> Perhaps, but what you have here is fine. Another possibility would be to >>>> just have a vec of the refs and extract the index from the ref later as >>>> needed. >>>> >>>> Jason >>>> >>> >>> Thanks for the feedback. I've kept the refs as-is for now. I've also >>> cleaned up a couple of other typos I'd had with comments and diagnostics. >>> >>> Bootstrapped and regtested on x86_64-pc-linux-gnu. >>> >>> @@ -6192,10 +6197,16 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t, >>> type = reftype; >>> - if (code == UNION_TYPE && CONSTRUCTOR_NELTS (*valp) >>> - && CONSTRUCTOR_ELT (*valp, 0)->index != index) >>> + if (code == UNION_TYPE >>> + && TREE_CODE (t) == MODIFY_EXPR >>> + && (CONSTRUCTOR_NELTS (*valp) == 0 >>> + || CONSTRUCTOR_ELT (*valp, 0)->index != index)) >>> { >>> - if (cxx_dialect < cxx20) >>> + /* We changed the active member of a union. Ensure that this is >>> + valid. */ >>> + bool has_active_member = CONSTRUCTOR_NELTS (*valp) != 0; >>> + tree inner = strip_array_types (reftype); >>> + if (has_active_member && cxx_dialect < cxx20) >>> { >>> if (!ctx->quiet) >>> error_at (cp_expr_loc_or_input_loc (t), >> >> While we're looking at this area, this error message should really mention >> that it's allowed in C++20. >> >>> @@ -6205,8 +6216,36 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t, >>> index); >>> *non_constant_p = true; >>> } >>> - else if (TREE_CODE (t) == MODIFY_EXPR >>> - && CONSTRUCTOR_NO_CLEARING (*valp)) >>> + else if (!is_access_expr >>> + || (CLASS_TYPE_P (inner) >>> + && !type_has_non_deleted_trivial_default_ctor (inner))) >>> + { >>> + /* Diagnose changing active union member after initialisation >>> + without a valid member access expression, as described in >>> + [class.union.general] p5. */ >>> + if (!ctx->quiet) >>> + { >>> + if (has_active_member) >>> + error_at (cp_expr_loc_or_input_loc (t), >>> + "accessing %qD member instead of initialized " >>> + "%qD member in constant expression", >>> + index, CONSTRUCTOR_ELT (*valp, 0)->index); >>> + else >>> + error_at (cp_expr_loc_or_input_loc (t), >>> + "accessing uninitialized member %qD", >>> + index); >>> + if (is_access_expr) >>> + { >>> + inform (DECL_SOURCE_LOCATION (index), >>> + "%qD does not implicitly begin its lifetime " >>> + "because %qT does not have a non-deleted " >>> + "trivial default constructor", >>> + index, inner); >>> + } >> >> The !is_access_expr case could also use an explanatory message. > > Thanks for the review, I've updated these messages and will send through > an updated patch once bootstrap/regtest is complete. > >> Also, I notice that this testcase crashes with the patch: >> >> union U { int i; float f; }; >> constexpr auto g (U u) { return (u.i = 42); } >> static_assert (g({.f = 3.14}) == 42); > > This appears to segfault even without the patch since GCC 13.1. > https://godbolt.org/z/45sPh8WaK > > I haven't done a bisect yet to work out what commit exactly caused this. > Should I aim to fix this first before coming back with this patch? Ah, I was just assuming it was related, never mind. I'll fix it. Jason