bugGNU Octave - Bugs: bug #60316, New warnings, errors showing up...

 
 

bug #60316: New warnings, errors showing up during compilation

Submitter:  Rik <rik5>
Submitted:  Wed 31 Mar 2021 08:53:00 PM UTC
   
 
Category:  Configuration and Build System Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Regression
Status:  Fixed Assigned to:  None
Originator Name:  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

Fri 09 Apr 2021 09:46:37 AM UTC, comment #26: 

@jwe: Are there some instructions which sequence of commands to run for updating gnulib?

Markus Mützel <mmuetzel>
Group administrator
Wed 07 Apr 2021 05:46:39 PM UTC, comment #25: 

Markus: I'm afraid I'm going to forget about this change every time I update gnulib.

Maybe we could create an update-gnulib target for the Makefile that could update gnulib, edit the boostrap.conf script, and update the boostrap script from gnulib while preserving the local changes?

John W. Eaton <jwe>
Group administrator
Wed 07 Apr 2021 06:14:01 AM UTC, comment #24: 

I pushed another change that reinstates the update logic in the bootstrap script here:
https://hg.savannah.gnu.org/hgweb/octave/rev/5fad0c81f7db

Markus Mützel <mmuetzel>
Group administrator
Tue 06 Apr 2021 09:00:58 PM UTC, comment #23: 

I disabled the "-Wtype-limits" warning in this cset: http://hg.savannah.gnu.org/hgweb/octave/rev/f152b41862d9.

Closing report.

Rik <rik5>
Group administrator
Tue 06 Apr 2021 06:40:49 PM UTC, comment #22: 

I updated gnulib with this changeset:

  http://hg.savannah.gnu.org/hgweb/octave/rev/0cf53a415b7f

Then I noticed that intprops-wrappers.c did not include config.h.  Although intprops-wrappers.h does include octave-config.h, that just provides a small subset of the configuration settings that are in config.h and some definitions provided by config.h may be needed for the gnulib macros to work properly.  So I fixed that with this changeset:

  http://hg.savannah.gnu.org/hgweb/octave/rev/6f353cc08d75

Then when I tried building with GCC 6, I still saw messages like this:


/home/jwe/src/octave/libgnu/intprops.h:609:22: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
       : (tmax) / (b) < (a)))
                      ^
/home/jwe/src/octave/libgnu/intprops.h:534:4: note: in expansion of macro ‘_GL_INT_MULTIPLY_RANGE_OVERFLOW’
   (overflow (a, b, tmin, tmax) \
    ^~~~~~~~
/home/jwe/src/octave/libgnu/intprops.h:512:11: note: in expansion of macro ‘_GL_INT_OP_CALC’
         : _GL_INT_OP_CALC (a, b, r, op, overflow, unsigned long int, \
           ^~~~~~~~~~~~~~~
/home/jwe/src/octave/libgnu/intprops.h:505:7: note: in expansion of macro ‘_GL_INT_OP_WRAPV_LONGISH’
     : _GL_INT_OP_WRAPV_LONGISH(a, b, r, op, overflow))
       ^~~~~~~~~~~~~~~~~~~~~~~~
/home/jwe/src/octave/libgnu/intprops.h:416:4: note: in expansion of macro ‘_GL_INT_OP_WRAPV’
    _GL_INT_OP_WRAPV (a, b, r, *, _GL_INT_MULTIPLY_RANGE_OVERFLOW)
    ^~~~~~~~~~~~~~~~
/home/jwe/src/octave/liboctave/wrappers/intprops-wrappers.c:56:10: note: in expansion of macro ‘INT_MULTIPLY_WRAPV’
   return INT_MULTIPLY_WRAPV (a, b, r);
          ^~~~~~~~~~~~~~~~~~


so I added a #pragma to silence the warning in this changeset:

  http://hg.savannah.gnu.org/hgweb/octave/rev/8f1cf32ada13

If other compiler versions produce other warnings, then go ahead and include additional #pragma lines for them in the intprops-wrappers.c file.

Note that we have our own macro to decide whether to use the "#pragma GCC diagnostic" lines.  I don't know why the gnulib sources are using "4 < _GNUC_ + (3 <= _GNUC_MINOR_)" because the GCC release notes say that "#pragma GCC diagnostic" was added in GCC 4.6.0.

John W. Eaton <jwe>
Group administrator
Tue 06 Apr 2021 09:14:27 AM UTC, comment #21: 

As Octave requires C++11, any gcc older than 4.8.1 is per definition unsupported.

https://gcc.gnu.org/gcc-4.8/cxx0x_status.html

Kai Torben Ohlhus <siko1056>
Group Member
Tue 06 Apr 2021 08:30:51 AM UTC, comment #20: 

I should have asked:
Do we still support gcc older than version 4.3?

Markus Mützel <mmuetzel>
Group administrator
Tue 06 Apr 2021 06:28:46 AM UTC, comment #19: 

In their test function, gnulib has the following:
https://git.savannah.gnu.org/cgit/gnulib.git/tree/tests/test-intprops.c?id=5ccd7e60ad475b481144437c8e4037a56f32e4a0#n19

/* Tell gcc not to warn about the long expressions that the overflow
   macros expand to, or about the (X < 0) expressions.  */
#if 4 < __GNUC__ + (3 <= __GNUC_MINOR__)
# pragma GCC diagnostic ignored "-Woverlength-strings"
# pragma GCC diagnostic ignored "-Wtype-limits"

/* Work around a bug in GCC 6.1 and earlier; see:
   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68971  */
# pragma GCC diagnostic ignored "-Woverflow"

#endif


I'm not exactly sure why they guard those pragmas with that particular condition. Maybe older gcc version (or other compilers) fail to understand them? Do we still support gcc 4.3 or older?
If we would like to make the change even more specific, we could also limit to gcc until version 6.1.

Markus Mützel <mmuetzel>
Group administrator
Tue 06 Apr 2021 02:58:17 AM UTC, comment #18: 

So jwe verified that gcc 8 does not issue a warning.  Should we disable the warnings in the wrapper for all versions of gcc <= 7?

Rik <rik5>
Group administrator
Mon 05 Apr 2021 04:06:03 PM UTC, comment #17: 

IIUC, they only disabled the warning in their test suite (for affected compilers). They didn't (and I'd guess they won't) disable it generally in the "actual" code.

Markus Mützel <mmuetzel>
Group administrator
Mon 05 Apr 2021 03:42:13 PM UTC, comment #16: 

@Markus: Should Octave be disabling these warnings, or should this info be forwarded to gnulib and they disable it on their side?

From the gnulib mailing list post you sent, gnulib is taking the hack of disabling -Wtype-limits via a #pragma just for certain versions of gcc.

Rik <rik5>
Group administrator
Sat 03 Apr 2021 06:57:17 AM UTC, comment #15: 

See also this old thread from the gnulib mailing list:
https://lists.gnu.org/archive/html/bug-gnulib/2011-05/msg00554.html

Maybe we could also disable the warning for these older compilers...

Markus Mützel <mmuetzel>
Group administrator
Fri 02 Apr 2021 08:19:55 PM UTC, comment #14: 

That's great that gnulib takes care of the optimizations.

I did manage to update and use a new version of gnulib (hint for future readers, you need to comment out GNULIB_REVISION in bootstrap.conf).  However, I still get the warning messages.  I took a look at 'git log' and there have been several check-ins that touch intprops.h.  For example, gnulib has disabled the HW-based __builtin commands for the clang compiler, and also for the gcc compiler versions 5 & 6.

Rik <rik5>
Group administrator
Fri 02 Apr 2021 05:07:03 PM UTC, comment #13: 

The gnulib macros do use GCC __builtin* functions if possible.  We do introduce some overhead by creating wrapper functions.  We do that because we want to avoid including gnulib headers directly in C++ code because their heavy use of macros has caused trouble in the past.  If necessary, we might consider making some exceptions to that rule but some care is needed.

John W. Eaton <jwe>
Group administrator
Fri 02 Apr 2021 05:03:11 PM UTC, comment #12: 

I thought that instead of reinventing the wheel we could just use what gnulib already has.
It seems to be using that function if it can:
https://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/intprops.h?id=38d0749a3077b03fda46567510b1217fb5e4e170#n405

Markus Mützel <mmuetzel>
Group administrator
Fri 02 Apr 2021 04:45:34 PM UTC, comment #11: 

I admit I don't know what the driving motivation here is.

If it is to detect overflow, then generally the fastest way is going to be via hardware.  Do the calculation and then check the overflow flag from the CPU.  This is generally just one instruction since there is always a BRANCH-ON-FLAG assembly instruction.

If the hardware does not support this (e.g., MIPS) then you need to resort to software workarounds with gnulib bit-inspection functions being an example.

This thread is useful on the general topic:
https://stackoverflow.com/questions/199333/how-do-i-detect-unsigned-integer-multiply-overflow

Specifically, there are gcc/clang extensions such as __builtin_umull_overflow(b, c, &c_test) which will perform the multiply and overflow detection in the most expedient way possible (HW if available, SW if not supported by target CPU).

It might be worth having #ifdef's in the code and having this HW fast path if this is going to become a frequent need in Octave (for example, implementing saturation semantics).

Rik <rik5>
Group administrator
Fri 02 Apr 2021 03:27:24 PM UTC, comment #10: 

We might also consider using these functions to implement saturation semantics for Matlab-compatible integer operations instead of using our own home grown code.

John W. Eaton <jwe>
Group administrator
Fri 02 Apr 2021 03:24:36 PM UTC, comment #9: 

Rik: I think we are using these functions when we specifically don't want multiplication to wrap, like when computing the number of array elements to allocate given a set of dimensions.  In that case, if the multiplication overflows we need an error, not the wrapped result.

John W. Eaton <jwe>
Group administrator
Fri 02 Apr 2021 03:04:23 PM UTC, comment #8: 

@Markus: I didn't look too closely at what the gnulib wrapper function does.  A true replacement for unsigned int would probably be

int
octave_ui_multiply_overflow_wrapper (unsigned int a, unsigned int b,
                                     unsigned int *r)
{
  r = a b;
  return 0;
}

This computes the multiplication and always indicates that no overflow occurred.

As far as I can tell, the C standard requires that unsigned integers implement wraparound semantics.  Therefore, these multiplications are performed the same everywhere and there are no cross-platform issues.  See, as just one random reference, https://www.learncpp.com/cpp-tutorial/unsigned-integers-and-why-to-avoid-them/.

However, the C standard is silent about signed integer multiplications.  Hence, this becomes compiler and architecture dependendent.  The gnulib wrappers provide a way of guaranteeing consistency, but that is only necessary for signed integers.

I will try updating gnulib and see if it helps.  If it doesn't, I suggest we avoid the macros where they are not required.

Rik <rik5>
Group administrator
Fri 02 Apr 2021 08:05:43 AM UTC, comment #7: 

I'm not sure how the code in comment #5 would effectively detect an integer overflow. I also cannot see where it sets the value of `r`.

If I understand gnulib's stance on compiler warnings correctly, they usually don't try to pacify all of them. Unless, this warning really indicates an issue with the code. (I'm not sure if the issue could lead to a wrong result of the function.)

Does the compiler still issue the warning if you update to a current gnulib?

Markus Mützel <mmuetzel>
Group administrator
Fri 02 Apr 2021 03:36:05 AM UTC, comment #6: 

I see the warnings if I compile with GCC 6 but not with GCC 8.

With GCC 6, I can suppress the messages with -Wno-type-limits.

Has this problem been reported to the gnulib maintainers?

John W. Eaton <jwe>
Group administrator
Thu 01 Apr 2021 09:44:50 PM UTC, comment #5: 

I just checked and re-built the libgnu directory from my copy of gnulib and still get these warnings.  My C compiler is


gcc --version
gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0
Copyright (C) 2017 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.


The warning looks conceptually right to me.  One full example is


In file included from liboctave/wrappers/intprops-wrappers.c:26:0:
liboctave/wrappers/intprops-wrappers.c: In function ‘octave_ui_multiply_overflow_wrapper’:
libgnu/intprops.h:45:55: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
 #define EXPR_SIGNED(e) (_GL_INT_NEGATE_CONVERT (e, 1) < 0)
                                                       ^
libgnu/intprops.h:392:42: note: in expansion of macro ‘EXPR_SIGNED’
     ((!_GL_SIGNED_TYPE_OR_EXPR (*(r)) && EXPR_SIGNED (a) && EXPR_SIGNED (b) \
                                          ^~~~~~~~~~~
liboctave/wrappers/intprops-wrappers.c:60:10: note: in expansion of macro ‘INT_MULTIPLY_WRAPV’
   return INT_MULTIPLY_WRAPV (a, b, r);
          ^~~~~~~~~~~~~~~~~~


The code in intprops-wrapper.c is


int
octave_ui_multiply_overflow_wrapper (unsigned int a, unsigned int b,
                                     unsigned int *r)
{
  return INT_MULTIPLY_WRAPV (a, b, r);
}


In this case, the inputs are "unsigned int" so they never can be less than 0 which matches the compiler warning.

Maybe we shouldn't be calling these wrapper functions at all for "unsigned int" objects.  Or if we must, then we can define them inline here as

+verbatim
int
octave_ui_multiply_overflow_wrapper (unsigned int a, unsigned int b,
                                     unsigned int *r)
{
  return (int) (a*b);
}

Rik <rik5>
Group administrator
Thu 01 Apr 2021 09:13:45 PM UTC, comment #4: 

On Fedora buildbot (that does not show this warning):



git log
commit 6160ee8e4d2b88d934c3c4c8c5930a75b835723f (HEAD)
Author: Paul Eggert <eggert@cs.ucla.edu>
Date:   Mon Sep 28 01:18:49 2020 -0700

    version-etc: pacify Oracle Studio 12.6

    Without this patch, it complains: "version-etc.h", line 64:
    warning: token-less macro argument (E_TOKENLESS_MACRO)" when in
    pedantic mode.
    * lib/version-etc.h (version_etc): Port to C89 macro rules.


Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Thu 01 Apr 2021 08:58:13 PM UTC, comment #3: 

I can build again after adding Qt5svg-dev package.

The error with the latex_interpreter has been reported in bug #60320.

That leaves the issue with intprops-wrappers.c.  Markus, can you tell me what version of gnulib you are building from?

I used 'git log' to find


commit 6160ee8e4d2b88d934c3c4c8c5930a75b835723f (HEAD)
Author: Paul Eggert <eggert@cs.ucla.edu>
Date:   Mon Sep 28 01:18:49 2020 -0700


We don't update gnulib very often, but maybe I need to.

Rik <rik5>
Group administrator
Thu 01 Apr 2021 06:37:28 AM UTC, comment #2: 

The gnulib warning might be due to this change:
https://hg.savannah.gnu.org/hgweb/octave/rev/b5f3ae7610cf

This might be a warning that we can just ignore. To be sure, we could ask on the gnulib mailing list.

Like Dmitri already wrote: The fact that you don't build with Qt even when you configured to build the GUI is due to the change in bug #59546. The same reason causes off-screen printing to fail.
I had to install `libqt5svg5-dev` on my Ubuntu to get the Qt5Svg module that is needed now. (That dependency might become obsolete again if I correctly understood Pantxo.)

The "latex-renderer" warning is also due to the change for bug #59546. We should probably delay the check for a working LaTeX toolchain until it is really needed. It looks like atm it is checked when the first figure opens.

Markus Mützel <mmuetzel>
Group administrator
Wed 31 Mar 2021 09:02:32 PM UTC, comment #1: 

I think you need Qt5Svg dev now.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Wed 31 Mar 2021 08:53:00 PM UTC, original submission:  

I did 'make maintainer-clean' and then re-built Octave from scratch starting with bootstrap, configure, and then make.

I'm seeing some warnings that certainly weren't there before.

Sample warning #1


In file included from liboctave/wrappers/intprops-wrappers.c:26:0:
liboctave/wrappers/intprops-wrappers.c: In function ‘octave_ui_multiply_overflow_wrapper’:
libgnu/intprops.h:45:55: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
 #define EXPR_SIGNED(e) (_GL_INT_NEGATE_CONVERT (e, 1) < 0)


There are more than 10 of these coming from intprops.h

Sample warning #2


  GEN      doc/interpreter/plot-patchproperties.texi
warning: latex_renderer: unable to compile "?"
warning: called from
    axes at line 107 column 8
    gca at line 62 column 7
    hggroup at line 56 column 9
    genpropdoc>getstructure at line 2069 column 7
    genpropdoc at line 49 column 8


Many of these coming from genpropdoc or the geometryimages.m script.

Error warning #1


error: print: figure must be visible or qt toolkit must be used with __gl_window__ property 'on' or QT_OFFSCREEN feature available
error: called from
    __opengl_print__ at line 209 column 7
    print at line 771 column 16
    geometryimages at line 83 column 5


Incidentally, something seems to have gone wrong with configure because QT, and the GUI, are disabled despite my not having configured things that way.

Rik <rik5>
Group administrator

 

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

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by siko1056 (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by dasergatskov (Posted a comment)
  • -email is unavailable- added by rik5 (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 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-04-06 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2021-04-06 jwe StatusConfirmed Ready For Test
    2021-04-01 mmuetzel Dependencies- bugs #60320 is dependent
    2021-04-01 mmuetzel StatusNone Confirmed

    Back to the top

    Powered by Savane 3.13-caa5.
    Corresponding source code