Re: [PATCH] variants: avoid type punning issue.

[Théophile Ranquet]

2013/1/30 Akim Demaille <address@hidden>:
>
> Le 29 janv. 2013 à 22:59, Theophile Ranquet <address@hidden> a écrit :
>
>> This is based on what Scott Meyers recommends in 'Effective C++'.
>
> Actually it's Alexandrescu and Sutter in "C++ Coding Standards"
> that I showed to you :)

Oh, right, but both are true. Should I reference both? I do believe
"Effective C++" outdates "C++ Coding Standards".

> And there is quite a few threads about
> this on StackOverflow.
>
> http://stackoverflow.com/questions/573294/when-to-use-reinterpret-cast
> http://stackoverflow.com/questions/1863069/casting-via-void-instead-of-using-reinterpret-cast?lq=1
> http://stackoverflow.com/questions/5025843/why-do-we-have-reinterpret-cast-in-c-when-two-chained-static-cast-can-do-its
> etc.

Yes, and I have read quite a bit on this topic now.

>> Use a static_cast on void* rather than directly use a reinterpret_cast, which
>> can have nefarious effects on objects.  However, even though following this
>> guideline is good practice in general, I am not quite sure how relevant it is
>> when applied to conversions from POD to objects.  Actually, it might very 
>> well
>> be the opposite: isn't this exactly what reinterpret_cast is for? What we
>> really want *is* to transmit the memory map as a series of bytes, which, if I
>> am correct, falls into the kind of "low level" hack for which this cast is
>> meant.
>
> I agree.
>
>> In any case, this silences the warning, which will be greatly appreciated by
>> anyone using variants with a compiler supporting -fstrict-aliasing.
>
> Well, the point is not to silence the warning, but really
> to stop worrying the compiler about what we are doing here.
> The reinterpret_cast on the raw memory was fine, except
> that G++ could lose track of what we were doing with our
> object, which is bad.  I'm looking for any means to avoid this.
> I would have preferred to keep the reinterpret_cast, which is
> more faithful here (we are really reading the memory with the
> different pattern of interpretation, as you noted above) and
> with the GCC attribute "may_alias"
> (http://gcc.gnu.org/onlinedocs/gcc-4.7.0/gcc/Type-Attributes.html)
> but we failed to use it to fix the issue, right?

Well, all I can say is that I have failed to fix it with may_alias.
Anyone is welcome to step in and show me how it should have been done,
of course.

>
>> * data/variant.hh (as): Here.
>> * tests/c++.at (Exception safety): Revert commit ddb9db15, type punning
>> is no longer an issue.
>
> This is not the only place where NO_STRICT_ALIAS_CXXFLAGS is
> used: please remove:
>
> - all its uses
> - all the code that defines it
> - references to this type punning issue in the documentation

Right. I will thusly add the following changes to this patch:

diff --git a/configure.ac b/configure.ac
index f10dabc..05a8d92 100644
--- a/configure.ac
+++ b/configure.ac
@@ -144,8 +144,6 @@ if test "$enable_gcc_warnings" = yes; then
   done
   # Clang++ 3.2+ reject C code generated by Flex.
   gl_WARN_ADD([-Wno-null-conversion], [WARN_NO_NULL_CONVERSION_CXXFLAGS])
-  # Variants break strict aliasing analysis.
-  gl_WARN_ADD([-fno-strict-aliasing], [NO_STRICT_ALIAS_CXXFLAGS])
   CXXFLAGS=$save_CXXFLAGS
   AC_LANG_POP([C++])
 fi
diff --git a/data/variant.hh b/data/variant.hh
index e2a537a..78aa876 100644
--- a/data/variant.hh
+++ b/data/variant.hh
@@ -143,7 +143,7 @@ m4_define([b4_variant_define],
       YYASSERT (tname == typeid (T).name ());
       YYASSERT (sizeof (T) <= S);])[
       {
-        void *dummy = buffer.raw;
+        void* dummy = buffer.raw;
         return *static_cast<T*> (dummy);
       }
     }
@@ -156,7 +156,7 @@ m4_define([b4_variant_define],
       YYASSERT (tname == typeid (T).name ());
       YYASSERT (sizeof (T) <= S);])[
       {
-        const void *dummy = buffer.raw;
+        const void* dummy = buffer.raw;
         return *static_cast<const T*> (dummy);
       }
     }
diff --git a/doc/bison.texi b/doc/bison.texi
index d2d3da3..4046b69 100644
--- a/doc/bison.texi
+++ b/doc/bison.texi
@@ -10208,14 +10208,6 @@ type on all platforms, alignments are
enforced for @code{double} whatever
 types are actually used.  This may waste space in some cases.

 @item
-Our implementation is not conforming with strict aliasing rules.  Alias
-analysis is a technique used in optimizing compilers to detect when two
-pointers are disjoint (they cannot ``meet'').  Our implementation breaks
-some of the rules that G++ 4.4 uses in its alias analysis, so @emph{strict
-alias analysis must be disabled}.  Use the option
address@hidden to compile the generated parser.
-
address@hidden
 There might be portability issues we are not aware of.
 @end itemize

diff --git a/examples/local.mk b/examples/local.mk
index 3185976..05e28e1 100644
--- a/examples/local.mk
+++ b/examples/local.mk
@@ -17,7 +17,6 @@ dist_noinst_SCRIPTS = examples/extexi examples/test
 TEST_LOG_COMPILER = $(top_srcdir)/examples/test

 AM_CXXFLAGS =                                                  \
-  $(NO_STRICT_ALIAS_CXXFLAGS)                                  \
   $(WARN_CXXFLAGS) $(WARN_CXXFLAGS_TEST) $(WERROR_CXXFLAGS)

 ## ------------ ##
diff --git a/tests/atlocal.in b/tests/atlocal.in
index 439a261..3a9f873 100644
--- a/tests/atlocal.in
+++ b/tests/atlocal.in
@@ -47,9 +47,6 @@ NO_WERROR_CXXFLAGS='@CXXFLAGS@ @WARN_CXXFLAGS@
@WARN_CXXFLAGS_TEST@'
   CFLAGS="$NO_WERROR_CFLAGS   @WERROR_CFLAGS@"
 CXXFLAGS="$NO_WERROR_CXXFLAGS @WERROR_CXXFLAGS@"

-# C++ variants break strict aliasing analysis.
-NO_STRICT_ALIAS_CXXFLAGS='@NO_STRICT_ALIAS_CXXFLAGS@'
-
 # If 'exit 77'; skip all C++ tests; otherwise ':'.
 BISON_CXX_WORKS='@BISON_CXX_WORKS@'

diff --git a/tests/c++.at b/tests/c++.at
index 874d640..d36e322 100644
--- a/tests/c++.at
+++ b/tests/c++.at
@@ -93,7 +93,7 @@ int main()
 ]])

 AT_BISON_CHECK([-o list.cc list.yy])
-AT_COMPILE_CXX([list], [$NO_STRICT_ALIAS_CXXFLAGS list.cc])
+AT_COMPILE_CXX([list], [list.cc])
 AT_PARSER_CHECK([./list], 0, [],
 [12
 123
@@ -251,7 +251,7 @@ namespace yy
 ]])

 AT_BISON_CHECK([-o list.cc list.yy])
-AT_COMPILE_CXX([list], [$NO_STRICT_ALIAS_CXXFLAGS list.cc])
+AT_COMPILE_CXX([list], [list.cc])
 AT_PARSER_CHECK([./list], 0,
 [(0, 1, 2, 4)
 ])