From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-x631.google.com (mail-pl1-x631.google.com [IPv6:2607:f8b0:4864:20::631]) by sourceware.org (Postfix) with ESMTPS id 5CF203858C53 for ; Fri, 21 Jul 2023 05:39:37 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 5CF203858C53 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-pl1-x631.google.com with SMTP id d9443c01a7336-1b8a8154f9cso11091815ad.1 for ; Thu, 20 Jul 2023 22:39:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1689917976; x=1690522776; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=aAdc3MwBXfxUFtinCWLg+bJqL/pQdUtDnkw4T+tzpzc=; b=PCo4xMIaE+kPEqyvJq9YqSWPDLspQha+B12ujYojHZHArakx+D7CgTo8S+QSM7ZNEU dcfA/xCiPvbgmINrgh9jBe852DMqpZu8hD7QsGFb03N2VDkkvz8wfEFlEIO5x/irRaT4 L4JCz42TtQajpvwCaPnTLZaXSxrkEpy2ZUrBNZqLrlIUJdsqaHM3yuwYaFcNy+l1hbft RvnjN02pjOyTkaX4GiJnNR5N9y5Z9mx6a72mNzZnVBMiWxJusEIKXuZ13yYwUeFbJQG9 rWh3giVkVVf9rVQmhhSmkHFuoCLqfKbCgnPaX8QS/4bOV69Q9oH3rALApZPiHyQrcsiO /6yw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689917976; x=1690522776; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=aAdc3MwBXfxUFtinCWLg+bJqL/pQdUtDnkw4T+tzpzc=; b=VKEi7rbt1J7nCHUjj2GKZ6BvAdiOuND1AU8SFgoFNCPPC89ysy7msuHIcFalitpI59 xd+IL89zL7EgXSAfVR5YT7w7qRwJiZzn6G1PWrVX2UVq+tkWDB12RGERuCniMgejGERW iaP6yU3sVMZIhbIWFBkFsGqeVJY4+TTtxneGJfdrpcipeqt+bJcgPgdBc/Yh/5gqfE7u JXWTK/XqZ2DPQeLSuUvQE9eYo6FPEix3BsSa2ovVWpoHs7hVOoSMBcaUsU5Hb9BfD6GQ kYALQvOH4hXRo9s3qH/BxJCe0cnKGtozHQLBm77xlxkGUdUFRMp2BuSTxpI5rpglAR0B TsOA== X-Gm-Message-State: ABy/qLakfoLUy+C6RnHwrFt24AEYjPpUQwcEWQ9R5FVng4LAgAhkMtKJ qMtwYJvXoH7URRsGX3ezblE= X-Google-Smtp-Source: APBJJlHzDkCwRsJvwmkIYUDn2g/cVgJsUoPpOWsOeygbajFZ70Ro/HIgLAE4OS1l44fmnE4QhGcW2A== X-Received: by 2002:a17:902:d481:b0:1b9:e23b:bb6a with SMTP id c1-20020a170902d48100b001b9e23bbb6amr2290117plg.11.1689917975779; Thu, 20 Jul 2023 22:39:35 -0700 (PDT) Received: from Thaum.localdomain (59-102-120-25.tpgi.com.au. [59.102.120.25]) by smtp.gmail.com with ESMTPSA id s9-20020a170902b18900b001b9de39905asm2435122plr.59.2023.07.20.22.39.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 20 Jul 2023 22:39:35 -0700 (PDT) Date: Fri, 21 Jul 2023 15:39:28 +1000 From: Nathaniel Shead To: Jason Merrill Cc: gcc-patches@gcc.gnu.org, Patrick Palka Subject: Re: [PATCH v4 2/3] c++: Improve constexpr error for dangling local variables [PR110619] Message-ID: References: <033dbf6e-6585-f5fc-75de-5ac7a47c8250@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <033dbf6e-6585-f5fc-75de-5ac7a47c8250@redhat.com> X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,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: On Thu, Jul 20, 2023 at 11:46:47AM -0400, Jason Merrill wrote: > On 7/20/23 05:36, Nathaniel Shead wrote: > > Currently, when typeck discovers that a return statement will refer to a > > local variable it rewrites to return a null pointer. This causes the > > error messages for using the return value in a constant expression to be > > unhelpful, especially for reference return values. > > > > This patch removes this "optimisation". > > This isn't an optimization, it's for safety, removing a way for an attacker > to get a handle on other data on the stack (CWE-562). > > But I agree that we need to preserve some element of UB for constexpr > evaluation to see. > > Perhaps we want to move this transformation to cp_maybe_instrument_return, > so it happens after maybe_save_constexpr_fundef? Hm, OK. I can try giving this a go. I guess I should move the entire maybe_warn_about_returning_address_of_local function to cp-gimplify.cc to be able to detect this? Or is there a better way of marking that a return expression will return a reference to a local for this transformation? (I guess I can't use whether the warning has been surpressed or not because the warning might not be enabled at all.) It looks like this warning is raised also by diag_return_locals in gimple-ssa-isolate-paths, should the transformation also be made here? I note that the otherwise very similar -Wdangling-pointer warning doesn't do this transformation either, should that also be something I look into fixing here? > > Relying on this raises a warning > > by default and causes UB anyway, so there should be no issue in doing > > so. We also suppress additional warnings from later passes that detect > > this as a dangling pointer, since we've already indicated this anyway. > > > > PR c++/110619 > > > > gcc/cp/ChangeLog: > > > > * semantics.cc (finish_return_stmt): Suppress dangling pointer > > reporting on return statement if already reported. > > * typeck.cc (check_return_expr): Don't set return expression to > > zero for dangling addresses. > > > > gcc/testsuite/ChangeLog: > > > > * g++.dg/cpp1y/constexpr-lifetime5.C: Test reported message is > > correct. > > * g++.dg/cpp1y/constexpr-lifetime6.C: Likewise. > > * g++.dg/cpp1y/constexpr-110619.C: New test. > > * g++.dg/warn/Wreturn-local-addr-6.C: Remove check for return > > value optimisation. > > > > Signed-off-by: Nathaniel Shead > > --- > > gcc/cp/semantics.cc | 5 ++++- > > gcc/cp/typeck.cc | 5 +++-- > > gcc/testsuite/g++.dg/cpp1y/constexpr-110619.C | 10 ++++++++++ > > gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime5.C | 4 ++-- > > gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime6.C | 8 ++++---- > > gcc/testsuite/g++.dg/warn/Wreturn-local-addr-6.C | 3 --- > > 6 files changed, 23 insertions(+), 12 deletions(-) > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-110619.C > > > > diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc > > index 8fb47fd179e..107407de513 100644 > > --- a/gcc/cp/semantics.cc > > +++ b/gcc/cp/semantics.cc > > @@ -1260,7 +1260,10 @@ finish_return_stmt (tree expr) > > r = build_stmt (input_location, RETURN_EXPR, expr); > > if (no_warning) > > - suppress_warning (r, OPT_Wreturn_type); > > + { > > + suppress_warning (r, OPT_Wreturn_type); > > + suppress_warning (r, OPT_Wdangling_pointer_); > > + } > > r = maybe_cleanup_point_expr_void (r); > > r = add_stmt (r); > > diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc > > index 859b133a18d..47233b3b717 100644 > > --- a/gcc/cp/typeck.cc > > +++ b/gcc/cp/typeck.cc > > @@ -11273,8 +11273,9 @@ check_return_expr (tree retval, bool *no_warning) > > else if (!processing_template_decl > > && maybe_warn_about_returning_address_of_local (retval, loc) > > && INDIRECT_TYPE_P (valtype)) > > - retval = build2 (COMPOUND_EXPR, TREE_TYPE (retval), retval, > > - build_zero_cst (TREE_TYPE (retval))); > > + /* Suppress the Wdangling-pointer warning in the return statement > > + that would otherwise occur. */ > > + *no_warning = true; > > } > > /* A naive attempt to reduce the number of -Wdangling-reference false > > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-110619.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-110619.C > > new file mode 100644 > > index 00000000000..cca13302238 > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-110619.C > > @@ -0,0 +1,10 @@ > > +// { dg-do compile { target c++14 } } > > +// { dg-options "-Wno-return-local-addr" } > > +// PR c++/110619 > > + > > +constexpr auto f() { > > + int i = 0; > > + return &i; > > +}; > > + > > +static_assert( f() != nullptr ); > > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime5.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime5.C > > index a4bc71d890a..ad3ef579f63 100644 > > --- a/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime5.C > > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime5.C > > @@ -1,11 +1,11 @@ > > // { dg-do compile { target c++14 } } > > // { dg-options "-Wno-return-local-addr" } > > -constexpr const int& id(int x) { return x; } > > +constexpr const int& id(int x) { return x; } // { dg-message "note: declared here" } > > constexpr bool test() { > > const int& y = id(3); > > return y == 3; > > } > > -constexpr bool x = test(); // { dg-error "" } > > +constexpr bool x = test(); // { dg-error "accessing object outside its lifetime" } > > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime6.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime6.C > > index f358aff4490..b81e89af79c 100644 > > --- a/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime6.C > > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-lifetime6.C > > @@ -4,12 +4,12 @@ > > struct Empty {}; > > constexpr const Empty& empty() { > > - return Empty{}; > > + return Empty{}; // { dg-message "note: declared here" } > > } > > -constexpr const Empty& empty_parm(Empty e) { > > +constexpr const Empty& empty_parm(Empty e) { // { dg-message "note: declared here" } > > return e; > > } > > -constexpr Empty a = empty(); // { dg-error "" } > > -constexpr Empty b = empty_parm({}); // { dg-error "" } > > +constexpr Empty a = empty(); // { dg-error "accessing object outside its lifetime" } > > +constexpr Empty b = empty_parm({}); // { dg-error "accessing object outside its lifetime" } > > diff --git a/gcc/testsuite/g++.dg/warn/Wreturn-local-addr-6.C b/gcc/testsuite/g++.dg/warn/Wreturn-local-addr-6.C > > index fae8b7e766f..ec8e241d83e 100644 > > --- a/gcc/testsuite/g++.dg/warn/Wreturn-local-addr-6.C > > +++ b/gcc/testsuite/g++.dg/warn/Wreturn-local-addr-6.C > > @@ -24,6 +24,3 @@ return_addr_local_as_intref (void) > > return (const intptr_t&)a; // { dg-warning "\\\[-Wreturn-local-addr]" } */ > > } > > - > > -/* Verify that the return value has been replaced with zero: > > - { dg-final { scan-tree-dump-times "return 0;" 2 "optimized" } } */ >