TakeV/dxx-rebirth
1
0
Fork 0
Code Issues Pull requests Packages Projects Releases Wiki Activity

Tighten __builtin_constant_p check to handle gcc-7

Browse source 
Compile time sanity checks are built on gcc's __builtin_constant_p.
This intrinsic is useful for optimization and for some simple forms of
compile-time checks (such as the infamous open(2) missing mode), but
upstream does not guarantee that it will work reliably for more
complicated checks.  Starting in gcc-7.1, __builtin_constant_p returns
an incorrect result of true for the vm_vec_sub expression

    `(a.x == b.x && a.y == b.y && a.z == b.z)`

even when the blamed sites clearly cannot prove that the inputs are
equal.  The useful result would be to return true if, and only if, the
inputs were provably identical; inputs which might be identical at
runtime, or might not, would return false.  Based on a bug filed with
gcc and the developer comments there, it appears many projects have
assumed this intrinsic is usable in this way, but the gcc developers do
not guarantee that it can be used this way.  Additionally, they believe
affected projects are rare and were wrong to use this intrinsic, so they
have no plans to fix this regression.

For more details, see gcc bug #72785
<https://gcc.gnu.org/bugzilla/show_bug.cgi?id=72785>.  For some more
pointed commentary on this change in gcc, see Linus' kernel commit
474c90156c8dcc2fa815e6716cc9394d7930cb9c
<https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=474c90156c8dcc2fa815e6716cc9394d7930cb9c>.

Update the SConf test to include the reproducer shown by Markus
Trippelsdorf in gcc bug #72785, comment #0.  This reproducer compiles
cleanly on <gcc-7, causing no change in semantics on older compilers.
Affected versions of gcc-7 will miscompile this reproducer into a link
error, causing SConf to record that the compiler does not optimize
__builtin_constant_p.  This is pessimistic, since even affected versions
of gcc-7 can handle some simple uses of __builtin_constant_p correctly.

This is a quick fix to get gcc-7 users working.  Upstream seems
disinclined to revert to the more useful semantics of <gcc-7 or to
introduce an alternative intrinsic with more helpful semantics.  As a
minor enhancement for Rebirth, it would be nice to probe the limits of
gcc-7's handling of __builtin_constant_p so that cases it handles
correctly could be enabled for gcc-7 users, while still blacklisting the
more complicated checks that gcc-7 miscompiles.

Thanks to Markus Trippelsdorf for providing a minimal reproducer to
detect the affected gcc versions.

Reported-by: parkerlreed <https://github.com/dxx-rebirth/dxx-rebirth/issues/337>
This commit is contained in:
Kp 2017-06-03 17:11:12 +00:00
parent 32daf8e8c3
commit 1ed7cec714
1 changed files with 23 additions and 5 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

28
SConstruct
View file

@ -1358,14 +1358,32 @@ is not a bug in the test or in the compiler.
help:assume compiler supports compile-time __builtin_constant_p
"""
f = '''
/*
* Begin `timekeeping.i` from gcc bug #72785, comment #0. This block
* provokes gcc-7 into generating an impossible code path, which then
* fails to link when ____ilog2_NaN is intentionally undefined.
*/
int a, b;
int ____ilog2_NaN();
static void by(void) {
int c = 1;
b = a ?: c;
__builtin_constant_p(b) ? b ? ____ilog2_NaN() : 0 : 0;
}
/*
* End `timekeeping.i`.
*/
int c(int);
static int a(int b){
return __builtin_constant_p(b) ? 1 : %s;
static int x(int y){
return __builtin_constant_p(y) ? 1 : %s;
}
'''
main = 'return a(1) + a(2)'
main = '''
by();
return x(1) + x(2);
'''
Define = context.sconf.Define
if self.Link(context, text=f % 'c(b)', main=main, msg='whether compiler optimizes __builtin_constant_p', calling_function='optimize_builtin_constant_p'):
if self.Link(context, text=f % 'c(y)', main=main, msg='whether compiler optimizes __builtin_constant_p', calling_function='optimize_builtin_constant_p'):
Define('DXX_HAVE_BUILTIN_CONSTANT_P')
Define('DXX_CONSTANT_TRUE(E)', '(__builtin_constant_p((E)) && (E))')
dxx_builtin_constant_p = '__builtin_constant_p(A)'
@ -3621,7 +3639,7 @@ class DXXCommon(LazyObjectConstructor):
def platform_settings(self):
# windows or *nix?
platform_name = self.user_settings.host_platform
message(self, "compiling on %s for %s into %s%s" % (sys.platform, platform_name, self.user_settings.builddir or '.',
message(self, "compiling on %r/%r for %r into %s%s" % (sys.platform, os.uname()[4], platform_name, self.user_settings.builddir or '.',
(' with prefix list %s' % str(self._argument_prefix_list)) if self._argument_prefix_list else ''))
return (
self.Win32PlatformSettings if platform_name == 'win32' else (