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

 
 

bug #48531: LTO warnings in liboctave

Submitted by:  Robert Jenssen <morgawr>
Submitted on:  Sat 16 Jul 2016 11:26:44 AM UTC  
 
Category: LibrariesSeverity: 1 - Wish
Priority: 3 - LowItem Group: Other
Status: FixedAssigned to: None
Originator Name: Robert JenssenOpen/Closed: Closed
Release: devOperating System: GNU/Linux

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission 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>
Project 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>
Project Administrator
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>
Project 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:

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>
Project 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>
Project 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>
Project 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:

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:

The warning messages returned.

As you say, using void* instead:

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>
Project Administrator
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:

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

(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>
Project Administrator
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:
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:

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:
gives:

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:

Robert Jenssen <morgawr>

 

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

Attach File(s):
   
   
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
  • -unavailable- added by jwe (Posted a comment)
  • -unavailable- added by mtmiller (Posted a comment)
  • -unavailable- added by morgawr (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 9 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 18 Jul 2016 03:29:40 PM UTCjweStatusNeed Info=>Fixed
      Assigned tojwe=>None
      Open/ClosedOpen=>Closed
    Mon 18 Jul 2016 04:22:03 AM UTCjweAttached File-=>Added lto-diffs.txt, #37930
    Mon 18 Jul 2016 03:36:49 AM UTCjweAssigned toNone=>jwe
    Sun 17 Jul 2016 01:00:34 AM UTCmorgawrAttached File-=>Added liboctave.link, #37916
    Sat 16 Jul 2016 09:37:10 PM UTCmtmillerSeverity3 - Normal=>1 - Wish
      Priority5 - Normal=>3 - Low
      StatusNone=>Need Info

    Back to the top


    Powered by Savane 3.1-cleanup1