Re: [lmi] [PATCH] Fix build with g++5

[Greg Chicares]

On 2015-09-15 22:50, Vadim Zeitlin wrote:
> 
>  I'm not sure why I didn't see this warning before, even with g++ 4.9, but
> with 5.2.1 I now get the following warnings when building lmi:
[...reordered...]
> alert_cli.cpp:41:10: error: '{anonymous}::ensure_setup' defined but not used 
> [-Werror=unused-variable]
>      bool ensure_setup = set_alert_functions
[...]
>  The first attached patch fixes the first warning by adding "volatile"
> keyword to alert_*.cpp files, just as it was already done elsewhere.

Yes, I should have written "volatile" here too. Thanks for catching this.

There's a stylistic inconsistency that I'm not going to worry about right now.
Sometimes, I've written "volatile" before the type it modifies, to make it
prominent (as I generally do with "static") because "volatile" is kind of
exotic. Other times, I've written it after the type it modifies, which is
where I write "const"; and that's probably better, especially because
inconsistent placement of cv-qualifiers in a "const volatile" declaration
is just wrong. But in this case I'll just make the output of
  grep --no-filename ensure_setup *.?pp |less -S
consistent.

> ./.libs/liblmi.so: undefined reference to 
> `trammel_base<calendar_date>::minimum_minimorum() const'
> ./.libs/liblmi.so: undefined reference to 
> `trammel_base<int>::maximum_maximorum() const'
> collect2: error: ld returned 1 exit status
> Makefile:3068: recipe for target 'lmi_wx' failed
> make: *** [lmi_wx] Error 1
[...]
>  AFAICS this is due to the fact that g++5 just inlines these methods
> directly when instantiating tn_range<> but doesn't generate them at all. To
> be honest, I'm not 100% sure if it's allowed to do this, but I think it is
> and, in any case, it definitely does. So I also had to add explicit
> instantiations of trammel_base<> to fix this in the third patch.

At first I wondered why it didn't complain about trammel_base<double>, yet
didn't complain about default_value() at all...but grepping showed that the
linker has identified the only cases that are used outside of 'tn_*.?pp'.
I'll apply this patch, adding <double>. I'm not really sure it's required
by the language, and I don't want to spend a lot of time puzzling over that.
(Years ago, I read a lot about guiding-decls, but then they changed that, so
it was a waste of time; at least I didn't spend time on 'exported' templates.)
We can be quite confident that the changes are benign, and they're demonstrably
necessary for linking with the latest toolchain; that's good enough.

So I'll commit those two patches soon; as for the third...

> actuarial_table.cpp: In member function 'void 
> soa_actuarial_table::find_table()':
> actuarial_table.cpp:577:60: error: dereferencing type-punned pointer will 
> break strict-aliasing rules [-Werror=strict-aliasing]
>              *reinterpret_cast<boost::int32_t*>(index_record)
[...]
>  The second patch fixes the remaining ones by modifying the code to
> explicitly use std::memcpy(). Alternatively, the usual union trick could be
> used, but I thought you might frown on using it and memcpy() is arguably
> more clear. But please let me know if you disagree.

Well, the union trick is unaesthetic--it requires a union to be defined,
and that impedes readability to say the least--but it could be hidden in a
function template I suppose. There's weirder stuff hidden in libraries that
we don't balk at using (like the language runtimes). However, AIUI, (1) the
real reason we care about type-punning (at least in situations such as this
patch addresses) is that it lets the optimizer work better, and (2) the
union trick prevents such optimizations, whereas the compiler can recognize
memcpy() and replace it with an optimization-friendly intrinsic.

Let's discuss selected parts of this patch:

-        t = *reinterpret_cast<T*>(z);
+        std::memcpy(&t, z, sizeof(T));

That's quite unobjectionable. I'd say the former line expresses its
intention more clearly, but OTOH reinterpret_cast is unclean anyway.
The replacement clearly wins because it solves an actual problem.

     BOOST_STATIC_ASSERT(sizeof(boost::int32_t) <= sizeof(int));
     while(index_ifs)
         {
-        int index_table_number =
-            *reinterpret_cast<boost::int32_t*>(index_record)
-            ;
+        boost::int32_t index_table_number;
+        std::memcpy
+            (&index_table_number
+            ,index_record
+            ,sizeof(index_table_number)
+            );
         if(table_number_ == index_table_number)

Changing that local variable's datatype gives me pause. In this single
isolated statement, int32_t does make sense. But the only place this
value is used is on the last quoted line above, which compares it to an
int; in that context, it makes sense to compare two plain ints. And as
a general rule I prefer to use the more "basic" types where they work.
If you don't disagree, then does g++-5 accept this if we alter the first
added line as follows (note the zero-initialization of all bits):
-        boost::int32_t index_table_number;
+        int index_table_number = 0;
and fix up the size this way:
-            ,sizeof(index_table_number)
+            ,sizeof(boost::int32_t)
?

The datatype question aside, this begins to make the code a little harder
to understand, and causes me to think we might be better off writing an
inline (for optimizability) memcpy_cast() function. We may wish we had
done that if g++-6 or -7 gets more aggressive about these warnings and
forces us to make lots more changes elsewhere.

     for(int j = 0; j < number_of_values; ++j)
         {
         is.read(z, sizeof(double));
-        data_[j] = *reinterpret_cast<double*>(z);
+        double d;
+        std::memcpy(&d, z, sizeof(double));
+        data_[j] = d;
         }

I'm sure you don't like that any more than I do. Merely reading the code
can't be enough to make me feel comfortable accepting it, because this is
a time-critical loop, and creating a temporary variable just looks like it
might slow the system down...so let's measure it:

BEFORE PATCH:
/lmi/src/lmi[0]$for z in a b c d e; do make $coefficiency unit_tests 
unit_test_targets=actuarial_table_test.exe 2>/dev/null | grep Speed; done
  Speed test: 1.383e-03 s =    1383396 ns, mean of 100 iterations
  Speed test: 1.412e-03 s =    1412003 ns, mean of 100 iterations
  Speed test: 1.390e-03 s =    1390287 ns, mean of 100 iterations
  Speed test: 1.389e-03 s =    1389031 ns, mean of 100 iterations
  Speed test: 1.442e-03 s =    1441833 ns, mean of 100 iterations

AFTER PATCH:
/lmi/src/lmi[0]$for z in a b c d e; do make $coefficiency unit_tests 
unit_test_targets=actuarial_table_test.exe 2>/dev/null | grep Speed; done
  Speed test: 1.392e-03 s =    1391892 ns, mean of 100 iterations
  Speed test: 1.389e-03 s =    1388594 ns, mean of 100 iterations
  Speed test: 1.397e-03 s =    1396716 ns, mean of 100 iterations
  Speed test: 1.420e-03 s =    1420374 ns, mean of 100 iterations
  Speed test: 1.423e-03 s =    1422821 ns, mean of 100 iterations

Supposing the median to be the most robust statistic, I compare
1390287 ns to 1396716 ns, and conclude that there's no valid
objection on performance grounds.

But still I balk at introducing that temporary. Would something like
this example (which I even tested, with g++-3.4.5 only):

-    char z[sizeof(double)];
     for(int j = 0; j < number_of_values; ++j)
         {
-        is.read(z, sizeof(double));
-        data_[j] = *reinterpret_cast<double*>(z);
+        is.read(reinterpret_cast<char*>(&data_[j]), sizeof(double));
         }

be acceptable to g++-5?