Re: [lmi] gcc -fprofile-generate and -fprofile-use [Was: gcc -flto]

[Greg Chicares]

On 2016-12-27 15:35, Vadim Zeitlin wrote:
> On Tue, 27 Dec 2016 14:59:36 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> With inadequate care, while battling insomnia, I repeated those steps
> GC> except that instead of manually removing everything except '*.gcda'
> GC> I did 'make clean' (removing even '*.gcda'), and then something odd
> GC> happened:
> GC> 
> GC> i686-w64-mingw32-g++ -MMD -MP -MT operations_posix_windows.o -MF 
> operations_posix_windows.d  -c -I /opt/lmi/src/lmi -I 
> /opt/lmi/src/lmi/tools/pete-2.1.1 -I 
> /opt/lmi/local/lib/wx/include/i686-w64-mingw32-msw-unicode-3.1 -I 
> /opt/lmi/local/include/wx-3.1 -I /opt/lmi/third_party/include -I 
> /opt/lmi/third_party/src -I /opt/lmi/local/include -I 
> /opt/lmi/local/include/libxml2 -DLMI_WX_NEW_USE_SO  -DLIBXML_USE_DLL -DSTRICT 
>    -D_FILE_OFFSET_BITS=64 -DWXUSINGDLL -D__WXMSW__ -DBOOST_STRICT_CONFIG   
> -std=c++11 -pedantic-errors -Werror -Wall -Wcast-align -Wconversion 
> -Wdeprecated-declarations -Wdisabled-optimization -Wextra -Wimport 
> -Wmultichar -Wpacked -Wpointer-arith -Wredundant-decls -Wsign-compare -Wundef 
> -Wwrite-strings  -Wno-long-long -Wctor-dtor-privacy -Wdeprecated 
> -Wnon-template-friend -Woverloaded-virtual -Wpmf-conversions -Wsynth  
> -Wcast-qual  -Wno-unused-parameter -Wno-conversion 
> -Wno-deprecated-declarations -Wno-parentheses -Wno-unused-local-typedefs 
> -Wno-unused-variable     -ggdb -O2 -fno-omit-frame-pointer -fprofile-use   
> /opt/lmi/third_party/src/boost/libs/filesystem/src/operations_posix_windows.cpp
>  -ooperations_posix_windows.o
> GC> 
> GC> 
> /opt/lmi/third_party/src/boost/libs/filesystem/src/operations_posix_windows.cpp:
>  In function 'bool boost::filesystem::equivalent(const 
> boost::filesystem::path&, const boost::filesystem::path&)':
> GC> 
> /opt/lmi/third_party/src/boost/libs/filesystem/src/operations_posix_windows.cpp:524:25:
>  error: 'error1' may be used uninitialized in this function 
> [-Werror=maybe-uninitialized]
> GC>            ph1, error1 ) );
> GC>                          ^
> GC> cc1plus: all warnings being treated as errors
> GC> /opt/lmi/src/lmi/workhorse.make:779: recipe for target 
> 'operations_posix_windows.o' failed
> 
>  This looks like a bug in compiler to me, the warning is given only when
> -fprofile-use is specified (even if there is no profile data) and only by
> MinGW-w64 4.9.1 compiler, i.e. it's not given neither by the native Linux
> amd64 4.9.2 nor by MinGW-w64 6.2.1.

https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
| -Wmaybe-uninitialized
| ...the compiler emits a warning if it cannot prove the uninitialized paths
| are not executed at run time

It's possible to consider that a QoI issue rather than a frank defect in
the compiler (if we take "cannot" to mean that it failed to find a proof
even though it wasn't impossible to find one). And the warning does flag a
frank weakness in the code: a reasonable initial value could be specified
when declaring 'error1'.

> GC> I think that warning is correct: if
> GC>        p1.handle != BOOST_INVALID_HANDLE_VALUE
> GC>     && p2.handle == BOOST_INVALID_HANDLE_VALUE
> GC> then 'error1' is used without ever having been initialized.
> 
>  No, the warning is definitely not correct, there is an earlier return if
> either of the handles is valid and there even is an assert checking that
> both of them are invalid before the line throwing the exception.

Wow, you're right. Rewriting it to clarify the logic:

      int error1; // save error code in case we have to throw
      bool p1_bad = p1.handle == BOOST_INVALID_HANDLE_VALUE;
      if      (  p1_bad )
        error1 = fs::detail::system_error_code();  // set error1 only if first 
is bad
      // ...
      bool p2_bad = p2.handle == BOOST_INVALID_HANDLE_VALUE;

      if      (  p1_bad ||  p2_bad )               // if either is bad...
      {
        if    ( !p1_bad || !p2_bad ) return false; // return 'false' if either 
is good
        assert(  p1_bad &&  p2_bad );              // now both must be bad...
        boost::throw_exception( filesystem_error(  // ...so report errno on 
first only
          "boost::filesystem::equivalent",         //   since we know both are 
bad, the
          ph1, error1 ) );                         //   first is bad, so error1 
is valid
      }

The code could obviously be improved thus:
-      int error1; // save error code in case we have to throw
+      int error1 = NO_ERROR; // save error code in case we have to throw
if we knew what value to initialize it with. But that's only a small
incremental improvement, and we really don't want to rewrite any of
that old boost::filesystem code.

> GC> I won't try to figure that out, or to fix the defect. Only the text of
> GC> an exception is affected, so I'll just suppress that warning for this
> GC> file.
> 
>  If you've decided to not use -fprofile-xxx finally, then it's not worth
> doing this neither as the warning doesn't happen without this option.

I agree with your statement; however, I would like to consider using
-fprofile-xxx someday. If that day comes before we upgrade from
gcc-4.9.1, then the patch I was ready to commit may be useful, so
for now I'll just paste it here rather than committing it...

...except now I see that I had already tested and committed it (in
order to get it out of the way so I could focus on 'round_to_test.cpp');
it's benign and even has some arguably (if slight) value, so it's not
worth the trouble to undo the commit even locally.