Re: [lmi] Working with and around strict aliasing (was: Fix build with g++5)

[Vadim Zeitlin]

On Fri, 18 Sep 2015 21:34:34 +0000 Greg Chicares <address@hidden> wrote:

GC> > GC> [...] this begins to make the code a little harder
GC> > GC> to understand, and causes me to think we might be better off writing 
an
GC> > GC> inline (for optimizability) memcpy_cast() function.
GC> > 
GC> >  I tried to keep things as simple as possible but yes, we surely could 
have
GC> > such a function and we could even have 2 template parameters for it to
GC> > optionally to return a larger type, e.g. to allow writing
GC> > 
GC> >   int const index_table_number = memcpy_cast<int32_t, int>(index_record);
GC> > 
GC> > Should I make a patch for this?
GC> 
GC> Yes, please. I'm in a quandary. The original reinterpret_cast lines seem
GC> much clearer to me than the memcpy() replacements; but if their behavior
GC> is undefined, then they have to change. I'm hoping that memcpy_cast<>()
GC> might preserve code clarity while slaying the UB demon.

 Here is how the code would look like:
---------------------------------- >8 --------------------------------------
--- a/actuarial_table.cpp
+++ b/actuarial_table.cpp
@@ -84,7 +85,7 @@
         t = invalid;
         char z[sizeof(T)];
         is.read(z, sizeof(T));
-        t = *reinterpret_cast<T*>(z);
+        t = from_buffer_cast<T>(z);
         LMI_ASSERT(invalid != t);
         return t;
     }
@@ -574,13 +575,13 @@ void soa_actuarial_table::find_table()
     while(index_ifs)
         {
         int index_table_number =
-            *reinterpret_cast<boost::int32_t*>(index_record)
+            from_buffer_cast_as<boost::int32_t, int>(index_record)
             ;
         if(table_number_ == index_table_number)
             {
             char* p = 54 + index_record;
-            boost::int32_t z = *reinterpret_cast<boost::int32_t*>(p);
-            table_offset_ = std::streampos(static_cast<int>(z));
+            int z = from_buffer_cast_as<boost::int32_t, int>(p);
+            table_offset_ = std::streampos(z);
             break;
             }
         index_ifs.read(index_record, index_record_length);
@@ -810,7 +811,7 @@ void soa_actuarial_table::read_values(std::istream& is, int 
nominal_length)
     for(int j = 0; j < number_of_values; ++j)
         {
         is.read(z, sizeof(double));
-        data_[j] = *reinterpret_cast<double*>(z);
+        data_[j] = from_buffer_cast<double>(z);
         }
 }

---------------------------------- >8 --------------------------------------

I've called the function from_buffer_cast() instead of memcpy_cast()
because I think the use of memcpy is just an implementation detail and the
important thing is that this function is meant to be used to reconstruct
the values from their raw representation in the buffer. And this name also
allows to have the symmetric to_buffer_cast() which could be used in
md5.cpp code if you think it's worth it (rather than "raw" memcpy).

 I've also had to add a separate from_buffer_cast_as() because I stupidly
forgot that default values for function template arguments were only valid
in C++11 and so couldn't be used in lmi code.

 This being said, I think the code above is not too bad and it definitely
looks better than my initial version, but I'll wait for your approval
before finalising the patch. I'd also appreciate if you could tell me where
would you prefer these new functions to go. It seems a bit of an overkill
to add a separate header just for them, but I don't see where else to put
them.

GC> Perhaps you could try -Wstrict-aliasing=1 or -Wstrict-aliasing=2 and
GC> let me know what you see.

 Building lmi_wx with the first of these options doesn't give any warnings
at all while the second one resulted in tons of warnings in wx/any.h, but
after fixing those, it gave exactly the same warning as the default (as
included in -Wall) -Wstrict-aliasing=3.

GC> AIUI, LLVM doesn't make any attempt to diagnose
GC> aliasing problems.

 No, it doesn't and I can confirm that clang 3.7 compiles the current lmi
code without any warnings (and seems to generate correct code, at least at
first glance...).

 Regards,
VZ