bugGNU Octave - Bugs: bug #48505, build does not use any warning...

 
 

bug #48505: build does not use any warning options in libgui/qterminal

Submitter:  Mike Miller <mtmiller>
Submitted:  Wed 13 Jul 2016 10:29:04 PM UTC
   
 
Category:  Configuration and Build System Severity:  2 - Minor
Priority:  5 - Normal Item Group:  Feature Request
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 13 Jul 2018 03:32:33 PM UTC, comment #15: 

No problems.  I was solving it in the most expedient manner, not necessarily the best.  After updating, runing 'make' failed.  I eventually did a 'make maintainer-clean' followed by a full bootsrap, configure, and make.  The build system seems fine now.

Rik <rik5>
Group administrator
Fri 13 Jul 2018 07:06:08 AM UTC, comment #14: 

Rik: Thanks, that patch will work, but I thought we should use the prototype for xerbla that is in lo-blas-proto.h instead of repeating it in xerbla.c.  So I included lo-blas-proto.h there and immediately found that it will only work with C++ because of our use of references instead of pointers.  I don't want to give that up, or complicate things by using references for C++ code and pointers for C code, so I ended up converting xerbla to C++.  Then the prototypes aren't needed.  Here's my change (accidentally pushed as two changesets when I should have merged them before pushing):

http://hg.savannah.gnu.org/hgweb/octave/rev/9ea68b9e277b
http://hg.savannah.gnu.org/hgweb/octave/rev/a8325f2361c7

John W. Eaton <jwe>
Group administrator
Fri 13 Jul 2018 05:37:32 AM UTC, comment #13: 

Okay, that's good then.

I silenced the newly emitted warnings by adding prototypes to xerbla.c immediately above the function declaration.  This seems redundant, but it works.  See https://hg.savannah.gnu.org/hgweb/octave/rev/fcfedeff926c.

Rik <rik5>
Group administrator
Fri 13 Jul 2018 02:36:13 AM UTC, comment #12: 

I have carefully gone over the diffs, thanks for going in depth to compare the build log, but I don't see any problems here.

The changes in your diff are the following

  • display-available.c and xerbla.c are now being built with the same set of warnings options as the rest of Octave, they were being built with no warnings options before. This seems like a good change.


  • _fltk_uigetfile_.cc, _init_fltk_.cc, and _init_gnuplot_.cc are now being compiled with fewer redundant -I options. All of the -I options that were there before are still there. I think this is also a good change. If it breaks something for someone we can fix it by cleaning up or reordering the CPPFLAGS variables.


As an example from your log file, _init_gnuplot_.cc used to be built with the following -I options, broken out for clarity, and omitting Octave internal -I options:


...
-I/usr/include/hdf5/serial
-I/usr/include/GraphicsMagick
-I/usr/include/freetype2
-I/usr/include/freetype2
...
-I/usr/include/freetype2
-I/usr/include/freetype2


where the '...' indicates other compiler options. Now it builds with


-I/usr/include/hdf5/serial
-I/usr/include/GraphicsMagick
-I/usr/include/freetype2
-I/usr/include/freetype2


So it has dropped the number of '-I/usr/include/freetype2' options from four to two. And the reason for still having it twice is because gnuplot includes '$(FT2_CPPFLAGS) $(FONTCONFIG_CPPFLAGS)' in its options, both of which include '-I/usr/include/freetype2' (since Fontconfig depends on FreeType).

The same goes for the fltk source files, it has reduced redundant -I options but nothing has been lost.

Mike Miller <mtmiller>
Group Member
Fri 13 Jul 2018 01:31:23 AM UTC, comment #11: 

I did a sanity check on the compilations by comparing the log files of the make process.


hg update -r 053502516f38  # Before change
make clean
make -j1 V=1 |& tee oldmake.log

hg update                  # After change
make clean
make -j1 V=1 |& tee newmake.log

sort oldmake.log > sort.oldmake.log
sort newmake.log > sort.newmake.log

diff -b oldmake.log newmake.log > diffs.to_inspect


There are a few differences that I would have you look at.  The file is attached as diffs.to_inspect.

The first difference I see is that we are now including the WARN_FLAGS when compiling in liboctave.  For example,


210c212
< /bin/bash ./libtool  --tag=CC   --mode=link gcc -fPIC -pthread -fopenmp -O2 -pipe   -o liboctave/external/blas-xtra/libxerbla.la  liboctave/external/blas-xtra/liboctave_external_blas_xtra_libxerbla_la-xerbla.lo  -lutil  -lm
---
> /bin/bash ./libtool  --tag=CC   --mode=link gcc -fPIC -pthread -fopenmp -Wall -W -Wformat -Wpointer-arith -Wmissing-prototypes -Wstrict-prototypes -Wwrite-strings -Wcast-align -Wcast-qual -O2 -pipe   -o liboctave/external/blas-xtra/libxerbla.la  liboctave/external/blas-xtra/liboctave_external_blas_xtra_libxerbla_la-xerbla.lo  -lutil  -lm


This is probably okay.  However, it has produced two new warnings during compilation.


2978a2986,2987
> liboctave/external/blas-xtra/xerbla.c:48:1: warning: no previous prototype for ‘octave_set_xerbla_handler’ [-Wmissing-prototypes]
> liboctave/external/blas-xtra/xerbla.c:57:1: warning: no previous prototype for ‘xerbla_’ [-Wmissing-prototypes]


The last area that needs inspection are three files in dldfcn.  Looking just at _init_gnuplot_.cc I see during the link


< /bin/bash ./libtool  --tag=CXX   --mode=link g++ -std=gnu++11 -fPIC -pthread -fopenmp -Wall -W -Wold-style-cast -Wformat -Wpointer-arith -Wwrite-strings -Wcast-align -Wcast-qual -I/usr/include/freetype2 -I/usr/include/freetype2 -O2 -pipe -avoid-version -module -no-undefined     -o libinterp/dldfcn/__init_gnuplot__.la -rpath /home/rik/local/lib/octave/5.0.0 libinterp/dldfcn/libinterp_dldfcn___init_gnuplot___la-__init_gnuplot__.lo   -lutil  -lm
1272a1272
> /bin/bash ./libtool  --tag=CXX   --mode=link g++ -std=gnu++11 -fPIC -pthread -fopenmp -Wall -W -Wold-style-cast -Wformat -Wpointer-arith -Wwrite-strings -Wcast-align -Wcast-qual -O2 -pipe -avoid-version -module -no-undefined     -o libinterp/dldfcn/__init_gnuplot__.la -rpath /home/rik/local/lib/octave/5.0.0 libinterp/dldfcn/libinterp_dldfcn___init_gnuplot___la-__init_gnuplot__.lo   -lutil  -lm


The old compile was including "-I/usr/include/freetype2" twice.  Twice seems excessive, but once might be necessary if gnuplot really does depend on FreeType.


(file #44546)

Rik <rik5>
Group administrator
Thu 12 Jul 2018 11:47:28 PM UTC, comment #10: 

And a couple more simplifications

https://hg.savannah.gnu.org/hgweb/octave/rev/e65dc7a2de42
https://hg.savannah.gnu.org/hgweb/octave/rev/6e6b5e85e08a

We now just have project-wide AM_CFLAGS, AM_CXXFLAGS, and AM_FFLAGS defined once, with the few exceptions defined in their own module.mk files (libgui/qterminal, liboctave/external, and liboctave/util).

Thanks for the help!

Mike Miller <mtmiller>
Group Member
Thu 12 Jul 2018 11:13:26 PM UTC, comment #9: 

Yes, thanks, I pushed a change to fix that oversight

https://hg.savannah.gnu.org/hgweb/octave/rev/12bb7bd389fc

Mike Miller <mtmiller>
Group Member
Thu 12 Jul 2018 10:31:34 PM UTC, comment #8: 

I think there might need to be an adjustment for libinterp/dldfcn/module.mk.  As an example,


%canon_reldir%___delaunayn___la_CXXFLAGS = $(libinterp_liboctinterp_la_CXXFLAGS) $(QHULL_CPPFLAGS)


It used to be that libinterp/module.mk defined libinterp_liboctinterp_la_CXXFLAGS:


-%canon_reldir%_liboctinterp_la_CFLAGS = $(AM_CFLAGS) $(WARN_CFLAGS)
-
-%canon_reldir%_liboctinterp_la_CXXFLAGS = $(AM_CXXFLAGS) $(WARN_CXXFLAGS)
-


but it no longer does.

Can the variable just be deleted entirely or should it now contain $(AM_CXXFLAGS)?

Rik <rik5>
Group administrator
Thu 12 Jul 2018 10:08:23 PM UTC, comment #7: 

The diff looks like the essence of what I wanted to accomplish, so I'm closing this as fixed and leaving it to the buildbots to test it.

Mike Miller <mtmiller>
Group Member
Thu 12 Jul 2018 09:34:58 PM UTC, comment #6: 

I pushed the following change:

http://hg.savannah.gnu.org/hgweb/octave/rev/90d43011e8e7

Mike or Rik: If this looks OK to you, please close this report.

John W. Eaton <jwe>
Group administrator
Thu 12 Jul 2018 09:14:15 PM UTC, comment #5: 

The patches don't apply now, but I'm looking at it and will check something in soon.

John W. Eaton <jwe>
Group administrator
Thu 12 Jul 2018 08:19:37 PM UTC, comment #4: 

Yeah I think we should apply both file #37865 and file #37877 as one a single change. I haven't tested these changes in a long while, not sure if they still apply or are complete.

Mike Miller <mtmiller>
Group Member
Thu 12 Jul 2018 04:26:41 PM UTC, comment #3: 

Just noting, a patch was submitted by jwe based on Mike's work back in 2016, but never seems to have made it to the "Patch Reviewed" phase.

I agree with jwe that we really don't want to enable warnings on the qterminal directory.  First, it is someone else's code and debugging and understanding someone else's code enough to decide whether a warning is real or not is a PITA.  Secondly, jwe has talked about removing this stuff entirely, so there's no reason to spend a lot of time on it.

Rik <rik5>
Group administrator
Thu 14 Jul 2016 09:20:42 PM UTC, comment #2: 

Simplifying the way we handle the compiler flags would be good.  I'd prefer to not enable warnings in the qterminal code though.  So how about applying your changes except that in the /libgui/qterminal-module.mk file, use the attached change?


(file #37877)

John W. Eaton <jwe>
Group administrator
Wed 13 Jul 2016 10:31:23 PM UTC, comment #1: 

And here's what I have changed so far, which simplifies all the compiler flags defined throughout the project but causes all these warnings to be emitted.

(file #37865)

Mike Miller <mtmiller>
Group Member
Wed 13 Jul 2016 10:29:04 PM UTC, original submission:  

Octave's configure script sets some default compiler options in the variables $(CXXFLAGS), $(AM_CXXFLAGS), and $(XTRA_CXXFLAGS), as well as some gcc warning options in the variable $(WARN_CXXFLAGS).

The top-level Makefile.am does


AM_CXXFLAGS = $(XTRA_CXXFLAGS)


and then every subdirectory does its own


foo_CXXFLAGS = $(AM_CXXFLAGS) $(WARN_CXXFLAGS)


This seems pretty redundant and wasteful, so I tried to simplify this by just adding the WARN_ flags to the AM_ flags at the top and eliminating all of the unnecessary foo_CXXFLAGS. The end result is that all the flags end up being the same, but we are intentionally omitting warning flags from the libgui/qterminal subdirectory.

The qterminal code (that we imported from somewhere else) contains many many warnings that our default flags would catch, including

  • 836 counts of -Wold-style-cast
  • 15 counts of -Wunused-variable
  • 7 counts of -Wcast-qual
  • 6 counts of -Wshadow
  • 1 count of -Wunused-parameter


This is not a huge priority, and maybe we leave it as it is now, but good to remember that there is potentially buggy code in the qterminal directory, and maybe we can fix it or maybe we replace the entire command window implementation before that happens.

Mike Miller <mtmiller>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #44546:  diffs.to_inspect added by rik5 (22KiB - application/octet-stream)
file #37877:  diffs.txt added by jwe (530B - text/plain)
file #37865:  cflags-simplify.diff added by mtmiller (10KiB - text/x-diff)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by jwe (Updated the item)
  • -email is unavailable- added by mtmiller (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 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-07-13 rik5 Attached File- Added diffs.to_inspect, #44546
    2018-07-12 mtmiller StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2018-07-12 jwe StatusPatch Submitted Ready For Test
    2018-07-12 rik5 StatusConfirmed Patch Submitted
    2016-07-14 jwe Attached File- Added diffs.txt, #37877
    2016-07-13 mtmiller Attached File- Added cflags-simplify.diff, #37865

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code