This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH][RFC] Instrument function exit with __builtin_unreachable in C++.
- From: Jason Merrill <jason at redhat dot com>
- To: Martin Liška <mliska at suse dot cz>
- Cc: gcc-patches List <gcc-patches at gcc dot gnu dot org>, Jakub Jelinek <jakub at redhat dot com>, Jonathan Wakely <jwakely at redhat dot com>
- Date: Thu, 5 Oct 2017 11:07:57 -0400
- Subject: Re: [PATCH][RFC] Instrument function exit with __builtin_unreachable in C++.
- Authentication-results: sourceware.org; auth=none
- References: <31ddd79e-1152-9dd9-663b-acd8d1bcd4ab@suse.cz>
On Thu, Oct 5, 2017 at 6:31 AM, Martin Liška <mliska@suse.cz> wrote:
> As discussed 2 days ago on IRC with Jakub and Jonathan, C++ standard says that having a non-return
> function with missing return statement is undefined behavior. We've got -fsanitize=return check for
> that and we can in such case instrument __builtin_unreachable(). This patch does that.
Great.
> And there's still some fallout:
>
> FAIL: g++.dg/cpp0x/constexpr-diag3.C -std=c++11 (test for errors, line 7)
> FAIL: g++.dg/cpp0x/constexpr-neg3.C -std=c++11 (test for errors, line 12)
> FAIL: g++.dg/cpp1y/constexpr-return2.C -std=c++14 (test for errors, line 7)
> FAIL: g++.dg/cpp1y/constexpr-return2.C -std=c++14 (test for excess errors)
> FAIL: g++.dg/cpp1y/pr63996.C -std=c++14 (test for errors, line 9)
> FAIL: g++.dg/cpp1y/pr63996.C -std=c++14 (test for excess errors)
>
> 1) there are causing:
>
> ./xg++ -B. /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/cpp1y/pr63996.C
> /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/cpp1y/pr63996.C:9:23: in constexpr expansion of ‘foo(1)’
> /home/marxin/Programming/gcc/gcc/testsuite/g++.dg/cpp1y/pr63996.C:4:1: error: ‘__builtin_unreachable()’ is not a constant expression
> foo (int i)
> ^~~
>
> Where we probably should not emit the built-in call. Am I right?
Or constexpr.c could give a more friendly diagnostic for
__builtin_unreachable. It's correct to give a diagnostic here for
this testcase.
> FAIL: g++.dg/torture/pr34850.C -O1 (test for warnings, line 15)
> FAIL: g++.dg/torture/pr34850.C -O2 (test for warnings, line 15)
> FAIL: g++.dg/torture/pr34850.C -O2 -flto -fno-use-linker-plugin -flto-partition=none (test for warnings, line 15)
> FAIL: g++.dg/torture/pr34850.C -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects (test for warnings, line 15)
> FAIL: g++.dg/torture/pr34850.C -O3 -g (test for warnings, line 15)
> FAIL: g++.dg/torture/pr34850.C -Os (test for warnings, line 15)
>
> 2) this test is somehow hard to fix as it requires some unreachability.
Can't we add a return statement to memset?
Also, this testcase seems to indicate a danger from this patch, like
we've seen before with bounds checking: if we have a checking path
that complains about invalid arguments and then has undefined
behavior, we optimize away the diagnostic. Can we warn in such cases?
We probably want to provide a way to turn off this behavior.
If we're going to enable this by default, we probably also want
-Wreturn-type on by default.
Jason