bugGNU Octave - Bugs: bug #55672, [octave forge] (control) undefined...

 
 

bug #55672: [octave forge] (control) undefined symbol: _Z11warning_msgPKcllPS0_l (development branch)

Submitter:  Mike Miller <mtmiller>
Submitted:  Fri 08 Feb 2019 01:08:27 AM UTC
   
 
Category:  Octave Package Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Unexpected Error or Warning
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

Wed 13 Feb 2019 09:52:10 PM UTC, comment #9: 

Thanks, you're right, still need to use the "%s" conversion to make sure the output is rendered as a literal string.

I made this change in the control package repo

  http://hg.code.sf.net/p/octave/control/rev/d9577257b76d

closing this as fixed again.

Mike Miller <mtmiller>
Group Member
Mon 11 Feb 2019 10:33:22 PM UTC, comment #8: 

I was thinking about doing something similar in the packages:

#if defined (OCTAVE_ENABLE_64)
#  define PKG_OCTAVE_IDX_TYPE_FORMAT PRId64
#else
#  define PKG_OCTAVE_IDX_TYPE_FORMAT PRId32
#endif


and using that macro.

But I now prefer your approach with C++ string streams. Only change maybe in the last line to avoid possible issues if somehow a format specifier would find its way into os:

  std::ostringstream os;
  os << name << ": the " << index << "-th argument had an invalid value";
  error ("%s", os.str ());


Also, if I remember correctly, gcc complained if a printf-style function was called with a format string that it couldn't analyse at compile time.

Markus Mützel <mmuetzel>
Group administrator
Mon 11 Feb 2019 10:15:22 PM UTC, comment #7: 

The long long type is also not guaranteed to exist on all platforms either, so that's not completely portable. Octave tests for long long in its configure, and also sets the preprocessor symbol OCTAVE_HAVE_LONG_LONG_INT, but only in Octave 4.2 and newer.

OCTAVE_IDX_TYPE_FORMAT only exists in Octave 5 and later, so that's not useful for packages that want to be backwards compatible with Octave 4.

I would also prefer to avoid preprocessor conditionals on '__WIN32__' or on 'OCTAVE_HAVE_LONG_LONG_INT' anyway, makes the code more unwieldy for such a small thing.

Maybe it would be safest and clearest to avoid printf-style conversions completely and use C++ stream conversions instead


  std::ostringstream os;
  os << name << ": the " << index << "-th argument had an invalid value";
  error (os.str ());


?

Mike Miller <mtmiller>
Group Member
Mon 11 Feb 2019 10:09:24 PM UTC, comment #6: 

"long long" is 64-bit on all platforms. Unfortunately, the format specifier for "long long" is platform specific. d'oh!

In core Octave we solved this in hg id 7f6a50f73625 by defining this macro:

#if defined (OCTAVE_ENABLE_64)
#  define OCTAVE_IDX_TYPE_FORMAT PRId64
#else
#  define OCTAVE_IDX_TYPE_FORMAT PRId32
#endif


Using it is a little wordy, e.g.:

error ("element number %" OCTAVE_IDX_TYPE_FORMAT
       " undefined in return list", k+1);


Markus Mützel <mmuetzel>
Group administrator
Mon 11 Feb 2019 09:55:00 PM UTC, comment #5: 

Thanks Markus, that is a problem. Do you have a recommendation on how to solve this portably and safely if we do want to avoid inadvertently casting to a smaller integer? It would be good to establish a pattern that we can use for all packages, I use the same conversion in the signal package.

The intent is to safely cast to an integer that is at least as large as octave_idx_type and has a predicable corresponding printf format specifier.

Mike Miller <mtmiller>
Group Member
Mon 11 Feb 2019 09:31:46 PM UTC, comment #4: 

Just a minor remark. On Windows, "long" is 32-bit. "octave_idx_type" might be 64-bit. So casting to long might cause an integer overflow.
For the warning messages, there is probably no harm in casting to the potentially smaller type.

@Doug: Do you plan to release a new version of control in time for the Octave 5? I am asking because we should probably do something about the patch in MXE Octave if it will be packaged with the current of-control.

Markus Mützel <mmuetzel>
Group administrator
Mon 11 Feb 2019 09:02:44 PM UTC, comment #3: 

Fixed on the control package development branch

http://hg.code.sf.net/p/octave/control/rev/3b79c7206376

Mike Miller <mtmiller>
Group Member
Fri 08 Feb 2019 01:43:10 AM UTC, comment #2: 

Here's a more complete change that fixes this bug and eliminates the -Wformat warning messages in what I think is a safer approach.

(file #46210)

Mike Miller <mtmiller>
Group Member
Fri 08 Feb 2019 01:22:05 AM UTC, comment #1: 

I can confirm that the attached change completely fixes this bug for me. But this change does introduce a printf format warning that you were probably trying to fix originally.

I think the most appropriate fix is to revert the argument types to octave_idx_type and then cast the value inside the function when assigning it to a format conversion, either "%d" or "%ld".

(file #46209)

Mike Miller <mtmiller>
Group Member
Fri 08 Feb 2019 01:08:27 AM UTC, original submission:  

If a snapshot of the current development branch of the control package is installed in Octave (any version), the following error comes out when calling certain functions


>> norm (tf (1, [1, 1]))
error: __proper_tf2ss__: /path/to/__control_slicot_functions__.oct: failed to load: /path/to/__control_slicot_functions__.oct: undefined symbol: _Z11warning_msgPKcllPS0_l
error: called from
    __sys2ss__>__proper_tf2ss__ at line 121 column 16
    __sys2ss__ at line 69 column 18
    ss at line 181 column 18
    ssdata at line 53 column 9
    norm>h2norm at line 68 column 18
    norm at line 55 column 12


This works in control 3.1.0, the error happens because some parameter types have been changed in the control repository. I suspect these should be changed back to octave_idx_type, but I'm not sure that these changes were correct in the first place.

Doug - did you intend to change these parameter types from octave_idx_type to int? This change doesn't seem safe if Octave is built with 64-bit Fortran integers. If you agree that the parameters types should be reverted from int to octave_idx_type, do you need help fixing whatever warnings you were trying to fix with these changes?

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 #46210:  bug55672.patch added by mtmiller (3KiB - text/x-patch)
file #46209:  bug55672.patch added by mtmiller (1KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by mtmiller
  •  

    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 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-02-13 mtmiller StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2019-02-11 mtmiller StatusFixed In Progress
        Open/ClosedClosed Open
    2019-02-11 mtmiller StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2019-02-08 mtmiller Attached File- Added bug55672.patch, #46210
        StatusNone Patch Submitted
    2019-02-08 mtmiller Attached File- Added bug55672.patch, #46209
    2019-02-08 mtmiller Carbon-CopyRemoved 80942 -
    2019-02-08 mtmiller Carbon-Copy- Added dastew

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code