diff mbox

[fixincludes] Fix <iso/math_c99.h> signbit on Solaris

Message ID yddionouizd.fsf@lokon.CeBiTec.Uni-Bielefeld.DE
State New
Headers show

Commit Message

Rainer Orth June 26, 2014, 9:18 a.m. UTC
As reported before

	https://gcc.gnu.org/ml/gcc-patches/2014-06/msg00213.html

the Solaris signbit implementation for GNU C in <iso/math_c99.h> gives
warnings with -Wstrict-aliasing, breaking Go bootstrap.

The following patch fixes this along the lines of the current
solaris_math_8 fix.  I've tried to use here documents to improve
readability by avoiding the quoting necessary in C strings, but to no
avail.  All leading tabs were converted into blanks instead, and I
couldn't get the pattern to match.  So I've decided to go for the
working, though a bit less readable, version.

Bootstrapped with no regressions on i386-pc-solaris2.11.  Passes
fixincludes make check.  Tested the testcase on x86_64-unknown-linux-gnu.

Ok for mainline?

Thanks.

	Rainer


2014-06-25  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>

	fixincludes:
	* inclhack.def (solaris_math_11): New fix.
	* fixincl.x: Regenerate.
	* tests/base/iso/math_c99.h [SOLARIS_MATH_11_CHECK]: New test.

	gcc/testsuite:
	* gcc.dg/signbit-sa.c: New test.

Comments

Bruce Korb June 28, 2014, 7:32 p.m. UTC | #1
On 06/26/14 02:18, Rainer Orth wrote:
> Ok for mainline?

Could you please reformat the c_fix_arg's and test-text to be "here strings" a la:

     c_fix_arg = <<- _EOS_
	#undef	signbit
	#define	signbit(x)	(sizeof(x) == sizeof(float) \
	\			   ? __builtin_signbitf(x) \
	\			   : sizeof(x) == sizeof(long double) \
	\			     ? __builtin_signbitl(x) \
	\			     : __builtin_signbit(x))";
	_EOS_;

I changed the "here string" thingy to eat that tab-backslash
and leave the rest of the tabs a few years ago.
That is considerably more readable than:

     c_fix_arg = "#undef\tsignbit\n"
		"#define\tsignbit(x)\t(sizeof(x) == sizeof(float) \\\n"
                 "\t\t\t   ? __builtin_signbitf(x) \\\n"
                 "\t\t\t   : sizeof(x) == sizeof(long double) \\\n"
                 "\t\t\t     ? __builtin_signbitl(x) \\\n"
                 "\t\t\t     : __builtin_signbit(x))";

and the other two are worse.  Thank you!
Rainer Orth July 1, 2014, 10:42 a.m. UTC | #2
Hi Bruce,

> On 06/26/14 02:18, Rainer Orth wrote:
>> Ok for mainline?
>
> Could you please reformat the c_fix_arg's and test-text to be "here strings" a la:
>
>     c_fix_arg = <<- _EOS_
> 	#undef	signbit
> 	#define	signbit(x)	(sizeof(x) == sizeof(float) \
> 	\			   ? __builtin_signbitf(x) \
> 	\			   : sizeof(x) == sizeof(long double) \
> 	\			     ? __builtin_signbitl(x) \
> 	\			     : __builtin_signbit(x))";
> 	_EOS_;
>
> I changed the "here string" thingy to eat that tab-backslash
> and leave the rest of the tabs a few years ago.

It's not yet in autogen 5.9: I've diffed the fixincl.x generated with my
original patch and the amended one and those backslashes after the
leading tab are still there.

> That is considerably more readable than:
>
>     c_fix_arg = "#undef\tsignbit\n"
> 		"#define\tsignbit(x)\t(sizeof(x) == sizeof(float) \\\n"
>                 "\t\t\t   ? __builtin_signbitf(x) \\\n"
>                 "\t\t\t   : sizeof(x) == sizeof(long double) \\\n"
>                 "\t\t\t     ? __builtin_signbitl(x) \\\n"
>                 "\t\t\t     : __builtin_signbit(x))";
>
> and the other two are worse.  Thank you!

Fully agreed, that's why I tried that route first.

I'm currently fighting to build autogen 5.18.3 and all its dependencies.
Trouble is, if we do require a version newer than 5.5.4 as documented in
install.texi, fixincludes make check will suddenly start to fail for
those whose autogen version isn't recent enough.

	Rainer
Rainer Orth July 1, 2014, 11:22 a.m. UTC | #3
Hi Bruce,

>> On 06/26/14 02:18, Rainer Orth wrote:
>>> Ok for mainline?
>>
>> Could you please reformat the c_fix_arg's and test-text to be "here strings" a la:
>>
>>     c_fix_arg = <<- _EOS_
>> 	#undef	signbit
>> 	#define	signbit(x)	(sizeof(x) == sizeof(float) \
>> 	\			   ? __builtin_signbitf(x) \
>> 	\			   : sizeof(x) == sizeof(long double) \
>> 	\			     ? __builtin_signbitl(x) \
>> 	\			     : __builtin_signbit(x))";
>> 	_EOS_;
>>
>> I changed the "here string" thingy to eat that tab-backslash
>> and leave the rest of the tabs a few years ago.
>
> It's not yet in autogen 5.9: I've diffed the fixincl.x generated with my
> original patch and the amended one and those backslashes after the
> leading tab are still there.

I've now managed to build autogen 5.18.3 on Solaris 11, but still there
is some trouble: with the following fix

/*
 * Newer Solaris 10/11 GCC signbit implementations cause strict-aliasing
 * warnings.
 */
fix = {
    hackname = solaris_math_11;
    select = '@\(#\)math_c99\.h' "[ \t]+1\\.[0-9]+[ \t]+[0-9/]+ ";
    files = iso/math_c99.h;
    c_fix = format;
    c_fix_arg = <<- _EOFix_
	#undef	signbit
	#define	signbit(x)	(sizeof(x) == sizeof(float) \
	\			   ? __builtin_signbitf(x) \
	\			   : sizeof(x) == sizeof(long double) \
	\			     ? __builtin_signbitl(x) \
	\			     : __builtin_signbit(x))
	_EOFix_;
    c_fix_arg = <<- _EOArg_
	^#undef[ 	]+signbit
	#if defined\(__sparc\)
	#define[ 	]+signbit\(x\)[ 	]+__extension__\( \\
	[ 	]+\{[ 	]*__typeof\(x\)[ 	]*__x_s[ 	]*=[ 	]*\(x\);[ 	]*\\
	[ 	]+\(int\)[ 	]*\(\*\(unsigned[ 	]*\*\)[ 	]*\&__x_s[ 	]*>>[ 	]*31\);[ 	]*\}\)
	#elif defined\(__i386\) \|\| defined\(__amd64\)
	#define[ 	]+signbit\(x\)[ 	]+__extension__\( \\
	[ 	]+\{ __typeof\(x\) __x_s = \(x\); \\
	[ 	]+\(sizeof \(__x_s\) == sizeof \(float\) \? \\
	[ 	]+\(int\) \(\*\(unsigned \*\) \&__x_s >> 31\) : \\
	[ 	]+sizeof \(__x_s\) == sizeof \(double\) \? \\
	[ 	]+\(int\) \(\(\(unsigned \*\) \&__x_s\)\[1\] >> 31\) : \\
	[ 	]+\(int\) \(\(\(unsigned short \*\) \&__x_s\)\[4\] >> 15\)\); \}\)
	#endif
	_EOArg_;
    test_text = <<- _EOText_
	/* @(#)math_c99.h	1.14	13/03/27 */
	#undef	signbit
	#if defined(__sparc)
	#define	signbit(x)	__extension__( \\
	\				{ __typeof(x) __x_s = (x); \\
	\				(int) (*(unsigned *) &__x_s >> 31); })
	#elif defined(__i386) || defined(__amd64)
	#define	signbit(x)	__extension__( \\
	\			{ __typeof(x) __x_s = (x); \\
	\			(sizeof (__x_s) == sizeof (float) ? \\
	\			(int) (*(unsigned *) &__x_s >> 31) : \\
	\			sizeof (__x_s) == sizeof (double) ? \\
	\			(int) (((unsigned *) &__x_s)[1] >> 31) : \\
	\			(int) (((unsigned short *) &__x_s)[4] >> 15)); })
	#endif
	_EOText_;
};

the test passes (not ran a bootstrap yet).  But I had to make two
unexpected changes:

* In the second c_fix_arg, all \t in charsets had to be replaced by
  literal TABs, otherwise they would occur as \\t in fixincl.x.

* In test_text, I had to backslash-escape the trailing \, otherwise they
  were eaten up.  Whether or not I do this makes no difference for the
  generated fixincl.x, but only with the escaping does make check pass.

>> That is considerably more readable than:
>>
>>     c_fix_arg = "#undef\tsignbit\n"
>> 		"#define\tsignbit(x)\t(sizeof(x) == sizeof(float) \\\n"
>>                 "\t\t\t   ? __builtin_signbitf(x) \\\n"
>>                 "\t\t\t   : sizeof(x) == sizeof(long double) \\\n"
>>                 "\t\t\t     ? __builtin_signbitl(x) \\\n"
>>                 "\t\t\t     : __builtin_signbit(x))";
>>
>> and the other two are worse.  Thank you!
>
> Fully agreed, that's why I tried that route first.
>
> I'm currently fighting to build autogen 5.18.3 and all its dependencies.
> Trouble is, if we do require a version newer than 5.5.4 as documented in
> install.texi, fixincludes make check will suddenly start to fail for
> those whose autogen version isn't recent enough.

This is certainly something that needs to be decided: if we go this
route, we should bump the autogen version requirement in install.texi
(to whatever is necessary to support the TAB\ magic).

Thanks.
        Rainer
diff mbox

Patch

# HG changeset patch
# Parent 4bb6a086dc232e27851ff33b22610d45dd18be57
Fix <iso/math_c99.h> signbit on Solaris

diff --git a/fixincludes/fixincl.x b/fixincludes/fixincl.x
--- a/fixincludes/fixincl.x
+++ b/fixincludes/fixincl.x
@@ -2,11 +2,11 @@ 
  * 
  * DO NOT EDIT THIS FILE   (fixincl.x)
  * 
- * It has been AutoGen-ed  Tuesday January  7, 2014 at 12:02:54 PM MET
+ * It has been AutoGen-ed  Wednesday June 25, 2014 at 05:24:42 PM MEST
  * From the definitions    inclhack.def
  * and the template file   fixincl
  */
-/* DO NOT SVN-MERGE THIS FILE, EITHER Tue Jan  7 12:02:54 MET 2014
+/* DO NOT SVN-MERGE THIS FILE, EITHER Wed Jun 25 17:24:42 MEST 2014
  *
  * You must regenerate it.  Use the ./genfixes script.
  *
@@ -15,7 +15,7 @@ 
  * certain ANSI-incompatible system header files which are fixed to work
  * correctly with ANSI C and placed in a directory that GNU C will search.
  *
- * This file contains 224 fixup descriptions.
+ * This file contains 225 fixup descriptions.
  *
  * See README for more information.
  *
@@ -6893,6 +6893,60 @@  static const char* apzSolaris_Math_9Patc
 
 /* * * * * * * * * * * * * * * * * * * * * * * * * *
  *
+ *  Description of Solaris_Math_11 fix
+ */
+tSCC zSolaris_Math_11Name[] =
+     "solaris_math_11";
+
+/*
+ *  File name selection pattern
+ */
+tSCC zSolaris_Math_11List[] =
+  "iso/math_c99.h\0";
+/*
+ *  Machine/OS name selection pattern
+ */
+#define apzSolaris_Math_11Machs (const char**)NULL
+
+/*
+ *  content selection pattern - do fix if pattern found
+ */
+tSCC zSolaris_Math_11Select0[] =
+       "@\\(#\\)math_c99\\.h[ \t]+1\\.[0-9]+[ \t]+[0-9/]+ ";
+
+#define    SOLARIS_MATH_11_TEST_CT  1
+static tTestDesc aSolaris_Math_11Tests[] = {
+  { TT_EGREP,    zSolaris_Math_11Select0, (regex_t*)NULL }, };
+
+/*
+ *  Fix Command Arguments for Solaris_Math_11
+ */
+static const char* apzSolaris_Math_11Patch[] = {
+    "format",
+    "#undef\tsignbit\n\
+#define\tsignbit(x)\t(sizeof(x) == sizeof(float) \\\n\
+\t\t\t   ? __builtin_signbitf(x) \\\n\
+\t\t\t   : sizeof(x) == sizeof(long double) \\\n\
+\t\t\t     ? __builtin_signbitl(x) \\\n\
+\t\t\t     : __builtin_signbit(x))",
+    "^#undef[ \t]+signbit\n\
+#if defined\\(__sparc\\)\n\
+#define[ \t]+signbit\\(x\\)[ \t]+__extension__\\( \\\\\n\
+[ \t]+\\{[ \t]*__typeof\\(x\\)[ \t]*__x_s[ \t]*=[ \t]*\\(x\\);[ \t]*\\\\\n\
+[ \t]+\\(int\\)[ \t]*\\(\\*\\(unsigned[ \t]*\\*\\)[ \t]*\\&__x_s[ \t]*>>[ \t]*31\\);[ \t]*\\}\\)\n\
+#elif defined\\(__i386\\) \\|\\| defined\\(__amd64\\)\n\
+#define[ \t]+signbit\\(x\\)[ \t]+__extension__\\( \\\\\n\
+[ \t]+\\{ __typeof\\(x\\) __x_s = \\(x\\); \\\\\n\
+[ \t]+\\(sizeof \\(__x_s\\) == sizeof \\(float\\) \\? \\\\\n\
+[ \t]+\\(int\\) \\(\\*\\(unsigned \\*\\) \\&__x_s >> 31\\) : \\\\\n\
+[ \t]+sizeof \\(__x_s\\) == sizeof \\(double\\) \\? \\\\\n\
+[ \t]+\\(int\\) \\(\\(\\(unsigned \\*\\) \\&__x_s\\)\\[1\\] >> 31\\) : \\\\\n\
+[ \t]+\\(int\\) \\(\\(\\(unsigned short \\*\\) \\&__x_s\\)\\[4\\] >> 15\\)\\); \\}\\)\n\
+#endif",
+    (char*)NULL };
+
+/* * * * * * * * * * * * * * * * * * * * * * * * * *
+ *
  *  Description of Solaris_Once_Init_1 fix
  */
 tSCC zSolaris_Once_Init_1Name[] =
@@ -9187,9 +9241,9 @@  static const char* apzX11_SprintfPatch[]
  *
  *  List of all fixes
  */
-#define REGEX_COUNT          261
+#define REGEX_COUNT          262
 #define MACH_LIST_SIZE_LIMIT 187
-#define FIX_COUNT            224
+#define FIX_COUNT            225
 
 /*
  *  Enumerate the fixes
@@ -9361,6 +9415,7 @@  typedef enum {
     SOLARIS_MATH_4_FIXIDX,
     SOLARIS_MATH_8_FIXIDX,
     SOLARIS_MATH_9_FIXIDX,
+    SOLARIS_MATH_11_FIXIDX,
     SOLARIS_ONCE_INIT_1_FIXIDX,
     SOLARIS_POSIX_SPAWN_RESTRICT_FIXIDX,
     SOLARIS_POW_INT_OVERLOAD_FIXIDX,
@@ -10252,6 +10307,11 @@  tFixDesc fixDescList[ FIX_COUNT ] = {
      SOLARIS_MATH_9_TEST_CT, FD_MACH_ONLY | FD_SUBROUTINE,
      aSolaris_Math_9Tests,   apzSolaris_Math_9Patch, 0 },
 
+  {  zSolaris_Math_11Name,    zSolaris_Math_11List,
+     apzSolaris_Math_11Machs,
+     SOLARIS_MATH_11_TEST_CT, FD_MACH_ONLY | FD_SUBROUTINE,
+     aSolaris_Math_11Tests,   apzSolaris_Math_11Patch, 0 },
+
   {  zSolaris_Once_Init_1Name,    zSolaris_Once_Init_1List,
      apzSolaris_Once_Init_1Machs,
      SOLARIS_ONCE_INIT_1_TEST_CT, FD_MACH_ONLY | FD_SUBROUTINE,
diff --git a/fixincludes/inclhack.def b/fixincludes/inclhack.def
--- a/fixincludes/inclhack.def
+++ b/fixincludes/inclhack.def
@@ -3562,6 +3562,53 @@  fix = {
 };
 
 /*
+ * Newer Solaris 10/11 GCC signbit implementations cause strict-aliasing
+ * warnings.
+ */
+fix = {
+    hackname = solaris_math_11;
+    select = '@\(#\)math_c99\.h' "[ \t]+1\\.[0-9]+[ \t]+[0-9/]+ ";
+    files = iso/math_c99.h;
+    c_fix = format;
+    c_fix_arg = "#undef\tsignbit\n"
+		"#define\tsignbit(x)\t(sizeof(x) == sizeof(float) \\\n"
+                "\t\t\t   ? __builtin_signbitf(x) \\\n"
+                "\t\t\t   : sizeof(x) == sizeof(long double) \\\n"
+                "\t\t\t     ? __builtin_signbitl(x) \\\n"
+                "\t\t\t     : __builtin_signbit(x))";
+    c_fix_arg = "^#undef[ \t]+signbit\n"
+		"#if defined\\(__sparc\\)\n"
+		"#define[ \t]+signbit\\(x\\)[ \t]+__extension__\\( \\\\\n"
+		"[ \t]+\\{[ \t]*__typeof\\(x\\)[ \t]*__x_s[ \t]*=[ \t]*\\(x\\);[ \t]*\\\\\n"
+		"[ \t]+\\(int\\)[ \t]*\\(\\*\\(unsigned[ \t]*\\*\\)[ \t]*\\&__x_s[ \t]*>>[ \t]*31\\);[ \t]*\\}\\)\n"
+		"#elif defined\\(__i386\\) \\|\\| defined\\(__amd64\\)\n"
+		"#define[ \t]+signbit\\(x\\)[ \t]+__extension__\\( \\\\\n"
+		"[ \t]+\\{ __typeof\\(x\\) __x_s = \\(x\\); \\\\\n"
+		"[ \t]+\\(sizeof \\(__x_s\\) == sizeof \\(float\\) \\? \\\\\n"
+		"[ \t]+\\(int\\) \\(\\*\\(unsigned \\*\\) \\&__x_s >> 31\\) : \\\\\n"
+		"[ \t]+sizeof \\(__x_s\\) == sizeof \\(double\\) \\? \\\\\n"
+		"[ \t]+\\(int\\) \\(\\(\\(unsigned \\*\\) \\&__x_s\\)\\[1\\] >> 31\\) : \\\\\n"
+		"[ \t]+\\(int\\) \\(\\(\\(unsigned short \\*\\) \\&__x_s\\)\\[4\\] >> 15\\)\\); \\}\\)\n"
+		"#endif";
+    test_text =
+    "/* @(#)math_c99.h	1.14	13/03/27 */\n"
+    "#undef	signbit\n"
+    "#if defined(__sparc)\n"
+    "#define	signbit(x)	__extension__( \\\\\n"
+    "				{ __typeof(x) __x_s = (x); \\\\\n"
+    "				(int) (*(unsigned *) &__x_s >> 31); })\n"
+    "#elif defined(__i386) || defined(__amd64)\n"
+    "#define	signbit(x)	__extension__( \\\\\n"
+    "			{ __typeof(x) __x_s = (x); \\\\\n"
+    "			(sizeof (__x_s) == sizeof (float) ? \\\\\n"
+    "			(int) (*(unsigned *) &__x_s >> 31) : \\\\\n"
+    "			sizeof (__x_s) == sizeof (double) ? \\\\\n"
+    "			(int) (((unsigned *) &__x_s)[1] >> 31) : \\\\\n"
+    "			(int) (((unsigned short *) &__x_s)[4] >> 15)); })\n"
+    "#endif";
+};
+
+/*
  * Sun Solaris defines PTHREAD_ONCE_INIT as an array containing a
  * structure.  As such, it need two levels of brackets, but only
  * contains one.  Wrap the macro definition in an extra layer.
diff --git a/fixincludes/tests/base/iso/math_c99.h b/fixincludes/tests/base/iso/math_c99.h
--- a/fixincludes/tests/base/iso/math_c99.h
+++ b/fixincludes/tests/base/iso/math_c99.h
@@ -75,3 +75,14 @@ 
 #undef	isunordered
 #define	isunordered(x, y)	__builtin_isunordered(x, y)
 #endif  /* SOLARIS_MATH_9_CHECK */
+
+
+#if defined( SOLARIS_MATH_11_CHECK )
+/* @(#)math_c99.h	1.14	13/03/27 */
+#undef	signbit
+#define	signbit(x)	(sizeof(x) == sizeof(float) \
+			   ? __builtin_signbitf(x) \
+			   : sizeof(x) == sizeof(long double) \
+			     ? __builtin_signbitl(x) \
+			     : __builtin_signbit(x))
+#endif  /* SOLARIS_MATH_11_CHECK */
diff --git a/gcc/testsuite/gcc.dg/signbit-sa.c b/gcc/testsuite/gcc.dg/signbit-sa.c
new file mode 100644
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/signbit-sa.c
@@ -0,0 +1,11 @@ 
+/* Some versions of Solaris <math.h> give strict-aliasing warnings for
+   signbit.  */
+/* { dg-options "-std=c99 -O2 -Wstrict-aliasing" } */
+
+#include <math.h>
+
+int
+main (void)
+{
+  return signbit (1.0f) | signbit (1.0) | signbit (1.0l);;
+}