bugGNU Octave - Bugs: bug #63300, valgrind warning from inv.cc

 
 

bug #63300: valgrind warning from inv.cc

Submitter:  Robert Jenssen <morgawr>
Submitted:  Tue 01 Nov 2022 05:17:53 AM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Unexpected Error or Warning
Status:  Fixed Assigned to:  None
Originator Name:  Robert Jenssen 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 04 Nov 2022 02:44:09 PM UTC, comment #9: 

This looks to be resolved.  Marking report as Fixed and closing.

Rik <rik5>
Group administrator
Thu 03 Nov 2022 08:45:03 AM UTC, comment #8: 

@Robert: Thanks for the demonstration. I missed that code path while looking at the sources.
It should be fixed now with the change you proposed and that has been applied on the default branch.
I guess it is pretty unlikely that the (un-initialized) value of `info` happens to evaluate to -1 by chance. So imho, it is ok to not backport this to the stable branch in the current situation (which would mean that we might need to delay the release of Octave 7.3 again).

Markus Mützel <mmuetzel>
Group administrator
Thu 03 Nov 2022 02:48:26 AM UTC, comment #7: 

I think a BIST is useful.

Rik <rik5>
Group administrator
Thu 03 Nov 2022 01:35:47 AM UTC, comment #6: 

I could confirm this behavior after a lot of work:

1. Searching for gloptipolydemo, I found and read https://homepages.laas.fr/henrion/software/gloptipoly/ then I downloaded that software and its prerequisite sedumi, examined them and ran a demo initiated by inst.m. This took a long time.

2. I added cout statements to inv.cc and selectively switched on or off the initialization of info.

3. I used that information to find a MUCH simpler test case: passing a scalar to inv() causes info to not be set:


>> inv (1)
info == 140217805074025
m ==  1
ans = 1

>> inv (i)
info == 140217805074025
m ==  (0,1)
ans =  0 - 1i

>> inv (single(1))
info == 140217805074025
m ==  1
ans = 1


4. With the initialization patch, passing a scalar causes info to be set:

>> inv (1)
info == 0
m ==  1
ans = 1

>> inv(i)
info == 0
m ==  (0,1)
ans =  0 - 1i

>> inv(single(1))
info == 0
m ==  1
ans = 1


@OP: Thank you for reporting this bug and the fix, but seriously, next time you report a bug, please create a minimum reproducer. All you needed was one of the scalar tests above, not an appeal to valgrind or a long GDB session which took me far too long to replicate and confirm.

@rik & mmuetzel: Would a BIST be useful for this or is it redundant now?

Arun Giridhar <arungiridhar>
Group Member
Wed 02 Nov 2022 10:15:38 PM UTC, comment #5: 


$ gdb /usr/local/octave-8.0.0/bin/octave-cli
GNU gdb (GDB) Fedora 12.1-2.fc36
Copyright (C) 2022 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
Type "show copying" and "show warranty" for details.
This GDB was configured as "x86_64-redhat-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<https://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
    <http://www.gnu.org/software/gdb/documentation/>.

For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from /usr/local/octave-8.0.0/bin/octave-cli...
(gdb) b inv.cc:173
No source file named inv.cc.
Make breakpoint pending on future shared library load? (y or [n]) y
Breakpoint 1 (inv.cc:173) pending.
(gdb) r
Starting program: /usr/local/octave-8.0.0/bin/octave-cli

This GDB supports auto-downloading debuginfo from the following URLs:
https://debuginfod.fedoraproject.org/
Enable debuginfod for this session? (y or [n]) y
Debuginfod has been enabled.
To make this setting permanent, add 'set debuginfod enabled on' to .gdbinit.
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
[New Thread 0x7fffe49ff640 (LWP 3593986)]
GNU Octave, version 8.0.0
Copyright (C) 1993-2022 The Octave Project Developers.
This is free software; see the source code for copying conditions.
There is ABSOLUTELY NO WARRANTY; not even for MERCHANTABILITY or
FITNESS FOR A PARTICULAR PURPOSE.  For details, type 'warranty'.

Octave was configured for "x86_64-pc-linux-gnu".

Additional information about Octave is available at https://www.octave.org.

Please contribute if you find this software useful.
For more information, visit https://www.octave.org/get-involved.html

Read https://www.octave.org/bugs.html to learn how to submit bug reports.
For information about changes from previous versions, type 'news'.

octave:1> gloptipolydemo

+ echo on
+ % Short demo of Gloptipoly 3
+ %
+ % Consider the classical problem of minimizing globally the
+ % two-dimensional six-hump camel back function
+ %
+ % min g0(x) = 4x1^2+x1x2-4x2^2-2.1x1^4+4x2^4+x1^6/3
+ %
+ % The function has six local minima, two of them being global minima.
+ %
+ % This optimization problem can be modeled as a moment
+ % problem as follows.
+ %
+ % First we define the variables
+ % and the polynomial to be minimized
+
+ mpol x1 x2
+ g0 = 4*x1^2+x1*x2-4*x2^2-2.1*x1^4+4*x2^4+x1^6/3

Thread 1 "octave-cli" hit Breakpoint 1, octave::Finv (args=..., nargout=1) at ../octave/libinterp/corefcn/inv.cc:173
173                      result = m.inverse (mattyp, info, rcond, true, true);
(gdb) b Matrix::inverse
Breakpoint 2 at 0x7ffff5d34cf0 (9 locations)
(gdb) p info
$1 = 140737272305312
(gdb) c
Continuing.

Thread 1 "octave-cli" hit Breakpoint 2, 0x00007ffff6ec2c60 in Matrix::inverse(MatrixType&, long&, double&, bool, bool) const@plt () from /usr/local/octave-8.0.0/lib/octave/8.0.0/liboctinterp.so.10
(gdb)
Continuing.

Thread 1 "octave-cli" hit Breakpoint 2, Matrix::inverse (this=0x7fffffffa940, mattype=..., info=@0x7fffffffaa08: 140737272305312, rcon=@0x7fffffffaa00: 0, force=true, calc_cond=true) at ../octave/liboctave/array/dMatrix.cc:637
637          int typ = mattype.type (false);
(gdb) p info
$2 = (octave_idx_type &) @0x7fffffffaa08: 140737272305312
(gdb) n
638          Matrix ret;
(gdb)
640          if (typ == MatrixType::Unknown)
(gdb)
643          if (typ == MatrixType::Diagonal)  // a scalar is classified as Diagonal.
(gdb)
645              ret = 1 / (*this);
(gdb)
646              if (calc_cond)
(gdb)
648                  double scalar = this->elem (0);
(gdb)
649                  if (octave::math::isfinite (scalar) && scalar != 0)
(gdb)
650                    rcon = 1.0;
(gdb)
684          return ret;
(gdb)
685        }
(gdb)
octave::Finv (args=..., nargout=1) at ../octave/libinterp/corefcn/inv.cc:174
174                      args(0).matrix_type (mattyp);
(gdb) p info
$3 = 140737272305312
(gdb) n
175                    }
(gdb)
202          octave_value_list retval (nargout > 1 ? 2 : 1);
(gdb)
204          retval(0) = result;
(gdb)
205          if (nargout > 1)
(gdb)
208          if (nargout < 2)
(gdb)
212              if (isfloat)
(gdb)
216                is_singular = ((rcond + 1.0 == 1.0) || octave::math::isnan (rcond))
(gdb)
217                              && ! arg.is_scalar_type ();
(gdb)
216                is_singular = ((rcond + 1.0 == 1.0) || octave::math::isnan (rcond))
(gdb)
216                is_singular = ((rcond + 1.0 == 1.0) || octave::math::isnan (rcond))
(gdb)
219              if (info == -1 || is_singular)
(gdb) p info
$4 = 140737272305312
(gdb)


Robert Jenssen <morgawr>
Wed 02 Nov 2022 04:34:13 PM UTC, comment #4: 

`valgrind` might be missing that calling `err_wrong_type_arg` always results in throwing an exception and potential jumps that appear after that will never be reached.

Anyway, like Rik already wrote there is probably not much harm in initializing that variable. So, I pushed your proposed change here:
https://hg.savannah.gnu.org/hgweb/octave/rev/7802c652081f


But please don't let that be a precedent that we'd like to silence all warnings from valgrind and similar tools.

Markus Mützel <mmuetzel>
Group administrator
Tue 01 Nov 2022 03:47:20 PM UTC, comment #3: 

This is a false positive.  The input is checked to be of numeric type at the start of the function and then there is an if/else tree which decodes the various numeric types and in every case info wil be assigned a value.

Valgrind and other software testing tools often have trouble with this construct because there is neither a default assignment nor a final else branch which the testing tool would recognize as catching all cases.

So, there is no problem with the code.  But, it might be useful to assign a default value simply to silence the testing tools.

Rik <rik5>
Group administrator
Tue 01 Nov 2022 09:24:21 AM UTC, comment #2: 

info is a local variable that, in some circumstances, is not initialised before being tested on line 214.

Robert Jenssen <morgawr>
Tue 01 Nov 2022 07:14:42 AM UTC, comment #1: 

Hu? Info is passed by reference, ye?

Anonymous
Tue 01 Nov 2022 05:17:53 AM UTC, original submission:  

I get the following valgrind warning:

 Conditional jump or move depends on uninitialised value(s)
==1651789==    at 0x59E77A5: octave::Finv(octave_value_list const&, int) (inv.cc:214)


This patch makes it go away:

--- octave-7.2.0/libinterp/corefcn/inv.cc       2022-07-28 23:08:26.000000000 +1000
+++ octave-7.2.0.new/libinterp/corefcn/inv.cc   2022-11-01 15:46:52.341223657 +1100
@@ -77,7 +77,7 @@
     err_square_matrix_required ("inverse", "A");

   octave_value result;
-  octave_idx_type info;
+  octave_idx_type info = 0;
   double rcond = 0.0;
   float frcond = 0.0;
   bool isfloat = arg.is_single_type ();


Robert Jenssen <morgawr>

 

(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

Items that depend on this one: None found

 

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

    Date Changed by Updated Field Previous Value => Replaced by
    2022-11-04 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2022-11-03 mmuetzel CategoryCoding Style and Maintenance Interpreter
        Severity2 - Minor 3 - Normal
        Priority3 - Low 5 - Normal
        Item GroupOther Unexpected Error or Warning
        Operating SystemGNU/Linux Any
    2022-11-02 mmuetzel StatusNone Ready For Test
    2022-11-02 mmuetzel Release7.2.0 dev
    2022-11-01 rik5 CategoryOctave Function Coding Style and Maintenance
        Severity3 - Normal 2 - Minor
        Priority5 - Normal 3 - Low
        Item GroupUnexpected Error or Warning Other

    Back to the top

    Powered by Savane 3.13-caa5.
    Corresponding source code