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

[Vadim Zeitlin]

On Thu, 17 Sep 2015 22:20:03 +0000 Greg Chicares <address@hidden> wrote:

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

 Sorry, I should have mentioned that I did worry about it after discovering
that "volatile" was not always placed in the same order. As you saw, I
decided to put all "cv-qualifiers" in the same place, because this seemed
like the most globally consistent thing to do.

GC> But in this case I'll just make the output of
GC>   grep --no-filename ensure_setup *.?pp |less -S
GC> consistent.

 But I can live with local consistency as well. OTOH it would be trivial to
just exchange the order of "volatile" and "T" here and in a few other
places to achieve uniform consistency...


GC> I'm not really sure it's required by the language

 Me neither, this seems unlikely to have been done accidentally and,
thinking about the number of functions that can be safely omitted from the
binary if they don't need to be instantiated at all, I think it could be a
nice optimization.

GC> (2) the union trick prevents such optimizations, whereas the compiler
GC> can recognize memcpy() and replace it with an optimization-friendly
GC> intrinsic.

 Just to confirm: gcc5 does inline memcpy() with "-O2". For example, in
md5_finish_ctx() the first (existing) memcpy() function call remains in the
optimized version but the 2 my patch adds appear as just a single "mov"
instruction.

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

 The compiler does accept it, of course (what else can it do?), but writing
code like this looks like a very bad idea to me. From a high level point of
view, using memcpy() where the actual size of the buffer (sizeof(int)) is
not the same as the length passed to the function (sizeof(int32_t)) is an
immediate serious warning signal, so even if/when it works, it still will
give a moment pause to anybody reading the code. From a lower level point
of view, this would completely break on 64 bit big endian architectures and
even if we don't use Alphas any more, I still don't see any good reason to
sacrifice portability here as we gain strictly nothing in return.

 The real index table type is int32_t because this is what the binary
format uses. If we want to work with ints, the right thing to do would be
to do

        boost::int32_t i4;
        std::memcpy(&i4, index_record, sizeof(boost::int32_t));
        int const index_table_number = i4;

but this just seemed like an overkill to me in this particular case.

GC> The datatype question aside, this begins to make the code a little harder
GC> to understand, and causes me to think we might be better off writing an
GC> inline (for optimizability) memcpy_cast() function.

 I tried to keep things as simple as possible but yes, we surely could have
such a function and we could even have 2 template parameters for it to
optionally to return a larger type, e.g. to allow writing

        int const index_table_number = memcpy_cast<int32_t, int>(index_record);

Should I make a patch for this?

GC> We may wish we had done that if g++-6 or -7 gets more aggressive about
GC> these warnings and forces us to make lots more changes elsewhere.

 I don't think there are any other places where we would need this.

GC> 
GC>      for(int j = 0; j < number_of_values; ++j)
GC>          {
GC>          is.read(z, sizeof(double));
GC> -        data_[j] = *reinterpret_cast<double*>(z);
GC> +        double d;
GC> +        std::memcpy(&d, z, sizeof(double));
GC> +        data_[j] = d;
GC>          }
GC> 
GC> I'm sure you don't like that any more than I do. Merely reading the code
GC> can't be enough to make me feel comfortable accepting it, because this is
GC> a time-critical loop, and creating a temporary variable just looks like it
GC> might slow the system down...so let's measure it:

 Sorry, I should have mentioned that I did compare the benchmark before and
after my changes and didn't see any statistically significant differences.

 I also didn't realize this loop was time-critical. If it is, I think it
would be much faster to just read all doubles at once instead of doing it
one by one. Would you be interested in making the measurements showing it
it's worth doing this or not?

GC> But still I balk at introducing that temporary. Would something like
GC> this example (which I even tested, with g++-3.4.5 only):
GC> 
GC> -    char z[sizeof(double)];
GC>      for(int j = 0; j < number_of_values; ++j)
GC>          {
GC> -        is.read(z, sizeof(double));
GC> -        data_[j] = *reinterpret_cast<double*>(z);
GC> +        is.read(reinterpret_cast<char*>(&data_[j]), sizeof(double));
GC>          }
GC> 
GC> be acceptable to g++-5?

 Yes, it would. But if you're willing to write code like this, then IMHO we
should just directly go to

    is.read(static_cast<char*>(static_cast<void*>(&data_[0])), number_of_values 
* sizeof(double));

and remove the loop completely.

 Regards,
VZ