bugGNU Octave - Bugs: bug #48531, LTO warnings in liboctave

 
 

bug #48531: LTO warnings in liboctave

Submitter:  Robert Jenssen <morgawr>
Submitted:  Sat 16 Jul 2016 11:26:44 AM UTC
   
 
Category:  Libraries Severity:  1 - Wish
Priority:  3 - Low Item Group:  Other
Status:  Fixed Assigned to:  None
Originator Name:  Robert Jenssen Open/Closed:  * Closed
Release:  * dev Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 18 Jul 2016 06:02:49 PM UTC, comment #11: 

Yes, the -Wterminate thing is unrelated.

I don't see a bug report about it.  Maybe it was something we discussed on the mailing list.  Hmm, I don't see it there either.  Maybe I just discussed it with myself.  Or on IRC.

I'll try to take another look at this problem and file a bug report as that is a serious issue that needs to be fixed before the release.

John W. Eaton <jwe>
Group administrator
Mon 18 Jul 2016 04:52:54 PM UTC, comment #10: 

LGTM, I get a warning free build now (except for the -Wterminate in on-cleanup.cc, but I think that's unrelated to this).

Mike Miller <mtmiller>
Group Member
Mon 18 Jul 2016 03:29:40 PM UTC, comment #9: 

I checked in a few changesets to address these issues, beginning with

  http://hg.savannah.gnu.org/hgweb/octave/rev/27b63b55bacb

Except for one warning about exception handling in onCleanup that I think already has a separate bug report, I'm get a clean build with the lto options now so I'm closing this report.  If there are other problems, please open a new bug report.

John W. Eaton <jwe>
Group administrator
Mon 18 Jul 2016 12:46:58 PM UTC, comment #8: 

I didn't succeed in applying the patch but I did notice that it doesn't change the return value of the Fortran subroutine crsf2csf from F77_RET_T (ie: int) to void. For example:


/usr/local/src/octave/octave/liboctave/cruft/lapack-xtra/zrsf2csf.f:22:2: note: return value type mismatch
        subroutine zrsf2csf(n,t,u,c,s)
  ^
/usr/local/src/octave/octave/liboctave/cruft/lapack-xtra/zrsf2csf.f:22:2: note: type 'void' should match type 'int'


Robert Jenssen <morgawr>
Mon 18 Jul 2016 04:22:03 AM UTC, comment #7: 

That should be If some system fails to allow a reinterpret_cast from "std::complex<double>*" to "double _Complex*"...

Also, actually remembering to attach the diff this time.


(file #37930)

John W. Eaton <jwe>
Group administrator
Mon 18 Jul 2016 04:21:03 AM UTC, comment #6: 

I'm attaching a preliminary version.  I think everything is fixed except the lsode common blocks.  I don't think there is actually a problem there, but we can still fix it.  I thought it would be trivial to just make all the declarations of the common blocks identical, but there is at least one instance where doing that would put the a subroutine parameter in a common block so a little more thought will be needed there about exactly what the best fix is.

I don't intend to use the "iso_c_binding" things unless forced.

If some system fails to allow a reinterpret_cast from "std::complex<double>" to "double _Complex*", then I think it should be possible to fix by using some other macro definition(s).  Or, if some different approach is needed, at least all of the locations that need to be fix may now be identified easily.

John W. Eaton <jwe>
Group administrator
Mon 18 Jul 2016 03:36:49 AM UTC, comment #5: 

I'm working on a patch for these warnings.  I should have something in a day or two.

John W. Eaton <jwe>
Group administrator
Sun 17 Jul 2016 11:58:24 AM UTC, comment #4: 


>>

Does putting a function signature in an extern "C" block somehow allow
the compiler to treat a std::complex<T> argument as a C complex argument?

>>

No, the reinterpret_cast<>() does that.


I don't claim to be a standards expert. This is my understanding of the situation and I'd be happy to be set straight. From the C++ standard Section 26.5 Complex numbers:

4 If z is an lvalue expression of type cv std::complex<T> then:
(4.1) — the expression reinterpret_cast<cv T(&)[2]>(z) shall be well-formed,
(4.2) — reinterpret_cast<cv T(&)[2]>(z)[0] shall designate the real part of z, and
(4.3) — reinterpret_cast<cv T(&)[2]>(z)[1] shall designate the imaginary part of z.


std::complex<float> is not a simple data type (POD) but it is a composition of two POD data (struct-POD). The reinterpret_cast<> ensures that the pointer passed to the Fortran side is actually a pointer to the internal data not to the C++ class instance. I'd be disappointed if the compiler designers had made std::complex<float> * not point at the underlying data but apparently this is not guaranteed.

The gfortran Fortran side expects to see a C language array of float _complex_. The iso_c_binding c_float_complex would enforce this but gfortran doesn't need it. I tried changing the function signature in schur.cc to:

F77_FUNC (crsf2csf, CRSF2CSF) (octave_idx_type *, float *,
                                 float *, float *, float *);


The warning messages returned.

As you say, using void* instead:

F77_FUNC (crsf2csf, CRSF2CSF) (octave_idx_type *, void *,
                                 void *, float *, float *);


gives no warnings but you have told the compiler not to check the types.

In summary, my understanding is that g++ does make std::complex<float>* point to the internal float data but the linker LTO warning is saying that this is implementation dependent and not guaranteed.

Robert Jenssen <morgawr>
Sun 17 Jul 2016 03:43:57 AM UTC, comment #3: 

Ok, I do see these warnings with GCC 6 but not 5.

I guess this may be due to or a side effect of some GCC 6 changes related to:

> Type merging was fixed to handle C and Fortran interoperability
> rules as defined by the Fortran 2008 language standard.


from https://gcc.gnu.org/gcc-6/changes.html.

I don't see what you're saying about the existing code assuming that a std::complex<float> can be converted to a C float complex type. Does putting a function signature in an extern "C" block somehow allow the compiler to treat a std::complex<T> argument as a C complex argument? The existing code is written to ensure that the C++ code passes a pointer to a std::complex<T> array, and the Fortran code is expecing a Fortran complex array. The C++ signature could be written with a void* parameter type instead and it would still work as well.

Mike Miller <mtmiller>
Group Member
Sun 17 Jul 2016 01:00:34 AM UTC, comment #2: 

The existing code in schur.cc and crsf2csf.f assumes that there is an implicit conversion from "std::complex<float>*" (C++ FloatComplex*) to "float _complex_ *" (in the extern "C" {} declaration) to Fortran complex. The linker LTO warnings and simple example show that g++ doesn't guarantee that this will work. The proposed diff makes these implicit type conversions explicit. I think that at the least the reinterpret_cast<> in schur.cc is required to suppress the warnings. With gfortran Fortran subroutines return void.

I am using FC24 gcc x86_64 6.1.1-2 and building with:


export CFLAGS="-O2 -march=native -flto=4 -ffat-lto-objects -fPIC"
export CXXFLAGS="-O2 -march=native -flto=4 -ffat-lto-objects -fPIC"
export FFLAGS="-O2 -march=native -flto=4 -ffat-lto-objects -fPIC"
export LDFLAGS="-fno-strict-aliasing"
export AR=gcc-ar
export NM=gcc-nm
export RANLIB=gcc-ranlib

/usr/local/src/octave/octave/configure \
--disable-java --without-fltk --without-qt \
--disable-atomic-refcount \
--with-blas="-lblas" --with-lapack="-llapack"
make V=1 -j 6


For the existing code the linker output for liboctave starts with (complete output attached):


libtool: link: g++  -fPIC -DPIC -shared -nostdlib /usr/lib/gcc/x86_64-redhat-linux/6.1.1/../../../../lib64/crti.o /usr/lib/gcc/x86_64-redhat-linux/6.1.1/crtbeginS.o  liboctave/.libs/liboctave_liboctave_la-liboctave-build-info.o  -Wl,--whole-archive liboctave/array/.libs/libarray.a liboctave/cruft/ranlib/.libs/libranlib.a liboctave/cruft/.libs/libcruft.a liboctave/numeric/.libs/libnumeric.a liboctave/operators/.libs/liboperators.a liboctave/system/.libs/libsystem.a liboctave/util/.libs/libutil.a liboctave/wrappers/.libs/libwrappers.a libgnu/.libs/libgnu.a -Wl,--no-whole-archive  -lcurl -lcholmod -lumfpack -lamd -lcamd -lcolamd -lccolamd -lcxsparse -larpack -lqrupdate -lfftw3_threads -lfftw3 -lfftw3f_threads -lfftw3f -llapack -lblas -lreadline -lncurses -lpcre -ldl -L/usr/lib/gcc/x86_64-redhat-linux/6.1.1 -L/usr/lib/gcc/x86_64-redhat-linux/6.1.1/../../../../lib64 -L/lib/../lib64 -L/usr/lib/../lib64 -L/usr/lib/gcc/x86_64-redhat-linux/6.1.1/../../.. -lgfortran -lquadmath -lstdc++ -lm -lgomp -lpthread -lc -lgcc_s /usr/lib/gcc/x86_64-redhat-linux/6.1.1/crtendS.o /usr/lib/gcc/x86_64-redhat-linux/6.1.1/../../../../lib64/crtn.o  -pthread -fopenmp -O2 -march=native -flto=4   -pthread -fopenmp -Wl,-soname -Wl,liboctave.so.3 -o liboctave/.libs/liboctave.so.3.0.0
libtool: link: g++ -pthread -fopenmp -Wall -W -Wshadow -Wold-style-cast -Wformat -Wpointer-arith -Wwrite-strings -Wcast-align -Wcast-qual -O2 -march=native -flto=4 -ffat-lto-objects -fPIC -fno-strict-aliasing -o src/octave-config src/src_octave_config-octave-config.o  libinterp/corefcn/.libs/libcorefcn.a libgnu/.libs/libgnu.a -lm -fopenmp -pthread
/usr/local/src/octave/octave/liboctave/cruft/odepack/slsode.f:1234:1: warning: type of 'sls001' does not match original declaration [-Wlto-type-mismatch]
       COMMON /SLS001/ ROWNS(209),
 ^
/usr/local/src/octave/octave/liboctave/cruft/odepack/sstode.f:109:1: warning: type of 'sls001' does not match original declaration [-Wlto-type-mismatch]
       COMMON /SLS001/ CONIT, CRATE, EL(13), ELCO(13,12),
 ^
/usr/local/src/octave/octave/liboctave/cruft/odepack/sintdy.f:51:1: note: 'sls001' was previously declared here
       COMMON /SLS001/ ROWNS(209),
 ^
/usr/local/src/octave/octave/liboctave/cruft/odepack/sintdy.f:51:1: note: code may be misoptimized unless -fno-strict-aliasing is used
/usr/local/src/octave/octave/liboctave/cruft/odepack/intdy.f:13:1: warning: type of 'ls0001' does not match original declaration [-Wlto-type-mismatch]
       COMMON /LS0001/ ROWNS(209),
 ^
/usr/local/src/octave/octave/liboctave/cruft/odepack/stode.f:17:1: warning: type of 'ls0001' does not match original declaration [-Wlto-type-mismatch]
       COMMON /LS0001/ CONIT, CRATE, EL(13), ELCO(13,12),
 ^
/usr/local/src/octave/octave/liboctave/cruft/odepack/dlsode.f:974:1: note: 'ls0001' was previously declared here
       COMMON /LS0001/ ROWNS(209),
 ^
/usr/local/src/octave/octave/liboctave/cruft/odepack/dlsode.f:974:1: note: code may be misoptimized unless -fno-strict-aliasing is used
/usr/local/src/octave/octave/liboctave/numeric/schur.cc:106:3: warning: type of 'crsf2csf_' does not match original declaration [-Wlto-type-mismatch]
   F77_FUNC (crsf2csf, CRSF2CSF) (const octave_idx_type&, FloatComplex *,
   ^
/usr/local/src/octave/octave/liboctave/cruft/lapack-xtra/crsf2csf.f:22:2: note: return value type mismatch
        subroutine crsf2csf(n,t,u,c,s)
  ^
/usr/local/src/octave/octave/liboctave/cruft/lapack-xtra/crsf2csf.f:22:2: note: type 'void' should match type 'int'
/usr/local/src/octave/octave/liboctave/cruft/lapack-xtra/crsf2csf.f:22:2: note: 'crsf2csf' was previously declared here
/usr/local/src/octave/octave/liboctave/cruft/lapack-xtra/crsf2csf.f:22:2: note: code may be misoptimized unless -fno-strict-aliasing is used
/usr/local/src/octave/octave/liboctave/numeric/schur.cc:88:3: warning: type of 'zrsf2csf_' does not match original declaration [-Wlto-type-mismatch]
   F77_FUNC (zrsf2csf, ZRSF2CSF) (const octave_idx_type&, Complex *,
   ^
/usr/local/src/octave/octave/liboctave/cruft/lapack-xtra/zrsf2csf.f:22:2: note: return value type mismatch
        subroutine zrsf2csf(n,t,u,c,s)
  ^
/usr/local/src/octave/octave/liboctave/cruft/lapack-xtra/zrsf2csf.f:22:2: note: type 'void' should match type 'int'
/usr/local/src/octave/octave/liboctave/cruft/lapack-xtra/zrsf2csf.f:22:2: note: 'zrsf2csf' was previously declared here
/usr/local/src/octave/octave/liboctave/cruft/lapack-xtra/zrsf2csf.f:22:2: note: code may be misoptimized unless -fno-strict-aliasing is used
/usr/local/src/octave/octave/liboctave/numeric/oct-convn.cc:73:181: warning: type of 'zdconv2i_' does not match original declaration [-Wlto-type-mismatch]
 FORWARD_IMPL (Complex, double, zd, ZD)
                                                                                                                                                                                     ^
/usr/local/src/octave/octave/liboctave/cruft/blas-xtra/zdconv2.f:53:231: note: return value type mismatch
       subroutine zdconv2i(ma,na,a,mb,nb,b,c)
                                                                                                                                                                                                                                       ^
/usr/local/src/octave/octave/liboctave/cruft/blas-xtra/zdconv2.f:53:231: note: type 'void' should match type 'int'
/usr/local/src/octave/octave/liboctave/cruft/blas-xtra/zdconv2.f:53:231: note: 'zdconv2i' was previously declared here
/usr/local/src/octave/octave/liboctave/cruft/blas-xtra/zdconv2.f:53:231: note: code may be misoptimized unless -fno-strict-aliasing is used

.
.
.


(file #37916)

Robert Jenssen <morgawr>
Sat 16 Jul 2016 09:37:10 PM UTC, comment #1: 

I do not see these warnings. What version of GCC and what options are you building with?

I build with GCC 5.3, with -flto in all of CFLAGS, CXXFLAGS, FFLAGS, and LDFLAGS, and with AR=gcc-ar, NM=gcc-nm, and RANLIB=gcc-ranlib. The only warnings I see are about the common sections "ls0001" and "sls001" declared in ODEPACK.

Moreover, your suggested change looks like it uses Fortran 2003 features. We try to stick to basic Fortran 77 for all sources in liboctave.

If you can propose a patch that avoids changing the Fortran in such a way, and hopefully also doesn't require doing casts between std::complex and C99 _complex_, please do. Marking as more info, if you want to try building again or work on this and provide a different patch.

Mike Miller <mtmiller>
Group Member
Sat 16 Jul 2016 11:26:44 AM UTC, original submission:  

Compiling and linking with gcc, g++ and gfortran flags including -flto gives warnings like:

../octave/liboctave/numeric/schur.cc:88:3: warning: type of 'zrsf2csf_' does not match original declaration [-Wlto-type-mismatch]
   F77_FUNC (zrsf2csf, ZRSF2CSF) (const octave_idx_type&, Complex *,
   ^
../octave/liboctave/cruft/lapack-xtra/zrsf2csf.f:22:2: note: return value type mismatch
        subroutine zrsf2csf(n,t,u,c,s)
  ^
../octave/liboctave/cruft/lapack-xtra/zrsf2csf.f:22:2: note: type 'void' should match type 'int'
../octave/liboctave/cruft/lapack-xtra/zrsf2csf.f:22:2: note: 'zrsf2csf' was previously declared here
../octave/liboctave/cruft/lapack-xtra/zrsf2csf.f:22:2: note: code may be misoptimized unless -fno-strict-aliasing is used

A possibly incomplete list of the Fortran subroutines affected is crsf2csf, zrsf2csf, zdconv2i, zdconv2o, csconv2i, csconv2o, cconv2i, cconv2o, cbiry, cairy, cbesy, cbesk, cbesj, cbesi, cbesh, xcdotu, xcdotc, xilaenv, xzdotu, xzdotc.

The following patch silences the warnings for crsf2csf:

diff -r ad1790bb8f71 liboctave/cruft/lapack-xtra/crsf2csf.f
--- a/liboctave/cruft/lapack-xtra/crsf2csf.f        Fri Jul 15 16:32:13 2016 -0700
+++ b/liboctave/cruft/lapack-xtra/crsf2csf.f        Sat Jul 16 19:12:14 2016 +1000
@@ -19,10 +19,13 @@
 c <http://www.gnu.org/licenses/>.
 c

-       subroutine crsf2csf(n,t,u,c,s)
-       integer n
-       complex t(n,n),u(n,n)
-       real c(n-1),s(n-1)
+       subroutine crsf2csf(n,t,u,c,s) bind(c, name="crsf2csf_")
+       use iso_c_binding, only: c_int, c_float, c_float_complex
+       integer(c_int), value :: n
+       complex(c_float_complex) :: t(n,n)
+       complex(c_float_complex) :: u(n,n)
+       real(c_float) :: c(n-1)
+       real(c_float) :: s(n-1)
        real x,y,z
        integer j
        do j = 1,n-1
diff -r ad1790bb8f71 liboctave/numeric/schur.cc
--- a/liboctave/numeric/schur.cc        Fri Jul 15 16:32:13 2016 -0700
+++ b/liboctave/numeric/schur.cc        Sat Jul 16 19:12:14 2016 +1000
@@ -102,9 +102,9 @@
                              F77_CHAR_ARG_LEN_DECL
                              F77_CHAR_ARG_LEN_DECL);

-  F77_RET_T
-  F77_FUNC (crsf2csf, CRSF2CSF) (const octave_idx_type&, FloatComplex *,
-                                 FloatComplex *, float *, float *);
+  void
+  F77_FUNC (crsf2csf, CRSF2CSF) (octave_idx_type, __complex__ float *,
+                                 __complex__ float *, float *, float *);
 }

 // For real types.
@@ -519,8 +519,11 @@
       OCTAVE_LOCAL_BUFFER (float, c, n-1);
       OCTAVE_LOCAL_BUFFER (float, sx, n-1);

-      F77_XFCN (crsf2csf, CRSF2CSF, (n, s.fortran_vec (),
-                                     u.fortran_vec (), c, sx));
+      F77_XFCN (crsf2csf, CRSF2CSF,
+                (n,
+                 reinterpret_cast< __complex__ float * >(s.fortran_vec()),
+                 reinterpret_cast< __complex__ float * >(u.fortran_vec()),
+                 c, sx));
     }

   return schur<FloatComplexMatrix> (s, u);


My understanding is that C++11 arranges that "__complex__ float" and "std::complex<float>" are made interchangeable by reinterpret_cast<>. See:http://en.cppreference.com/w/cpp/numeric/complex

For example:

// test_complex.cc

#include <complex>
#include <complex.h>

int main(void)
{
    std::complex<float> jf(0.0F, 1.0F);
    float __complex__ jcf={jf.real(),jf.imag()};
    jcf={0,1};
    jf=std::complex<float>(jcf);
    jcf=*(reinterpret_cast<__complex__ float*>(&jf));
    jcf=jf;
    return 0;
}

gives:

$ g++ -std=c++11 -Wall -o test_complex test_complex.cc
test_complex.cc: In function ‘int main()’:
test_complex.cc:13:9: error: cannot convert ‘std::complex<float>’ to ‘__complex__ float’ in assignment
     jcf=jf;
         ^~


I'm not sure what happens on the Fortran side when octave is configured with --enable-64.

The above patch does nothing for LTO warnings about naming differences inside Fortran COMMON blocks in the odepack code. For example:

octave/liboctave/cruft/odepack/slsode.f:1234:1: warning: type of 'sls001' does not match original declaration [-Wlto-type-mismatch]
       COMMON /SLS001/ ROWNS(209),
 ^
octave/liboctave/cruft/odepack/sstode.f:109:1: warning: type of 'sls001' does not match original declaration [-Wlto-type-mismatch]
       COMMON /SLS001/ CONIT, CRATE, EL(13), ELCO(13,12),
 ^
octave/liboctave/cruft/odepack/sintdy.f:51:1: note: 'sls001' was previously declared here
       COMMON /SLS001/ ROWNS(209),
 ^
octave/liboctave/cruft/odepack/sintdy.f:51:1: note: code may be misoptimized unless -fno-strict-aliasing is used


Robert Jenssen <morgawr>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #37930:  lto-diffs.txt added by jwe (152KiB - text/plain)
file #37916:  liboctave.link added by morgawr (24KiB - application/octet-stream - gcc-6.1.1 LTO linker command and output for liboctave.so)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by morgawr (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-07-18 jwe StatusNeed Info Fixed
        Assigned tojwe None
        Open/ClosedOpen Closed
    2016-07-18 jwe Attached File- Added lto-diffs.txt, #37930
    2016-07-18 jwe Assigned toNone jwe
    2016-07-17 morgawr Attached File- Added liboctave.link, #37916
    2016-07-16 mtmiller Severity3 - Normal 1 - Wish
        Priority5 - Normal 3 - Low
        StatusNone Need Info

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code