bugGNU Octave - Bugs: bug #61753, Changing assert to panic_impossible

 
 

bug #61753: Changing assert to panic_impossible

Submitter:  Arun Giridhar <arungiridhar>
Submitted:  Sat 01 Jan 2022 05:58:59 PM UTC
   
 
Category:  Coding Style and Maintenance Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Other
Status:  In Progress Assigned to:  None
Originator Name:  Open/Closed:  * Open
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  10.1.0 (current default)
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sat 23 Mar 2024 07:01:27 PM UTC, comment #21: 

I think it's worth a review of liboctave, and 50 isn't too bad.  I'm not too bothered by the names (assert vs. panic_unless).  Rather, I would like to eliminate needless use of assert in input validation when a call to liboctave_error_handler would be more appropriate.

Rik <rik5>
Group administrator
Sat 23 Mar 2024 06:17:58 PM UTC, comment #20: 

The following changesets eliminate the last uses of assert in libinterp files:

https://hg.savannah.gnu.org/hgweb/octave/rev/432e0151f652
https://hg.savannah.gnu.org/hgweb/octave/rev/08ab46f6e241

Is there anything left to do for this bug report?  Maybe there are still some places were we could use error instead of panic?

Did you intend to also try to eliminate the uses of assert in liboctave?  I thought about moving the panic functions to liboctave, but they ultimately call error_system::vpanic.  Moving that to liboctave might not be possible.  There do seem to be some overly aggressive uses of assert in the argument validation of various liboctave functions, so maybe we should look to convert those to call error instead?  I count a total of 50 remaining uses of assert in liboctave.  I could start to look at some of those now.

John W. Eaton <jwe>
Group administrator
Wed 11 Oct 2023 09:55:28 AM UTC, comment #19: 

Rik, is this activity complete?

Arun Giridhar <arungiridhar>
Group Member
Thu 10 Feb 2022 03:59:46 PM UTC, comment #18: 

I'm waiting on the resolution of bug #62013 before continuing with this one.  I've submitted a patch there already.

Rik <rik5>
Group administrator
Tue 08 Feb 2022 06:09:19 AM UTC, comment #17: 

I pushed the changeset for libinterp here: http://hg.savannah.gnu.org/hgweb/octave/rev/08b08b7f05b2.

I think on liboctave/ we need to determine whether the statements are checking for impossible conditions, rather than input validation, that really are only relevant while debugging liboctave.  If that is the case then for final compilation of liboctave we could set the macro NDEBUG during compilation and thereby remove all of the instances without recoding.

Rik <rik5>
Group administrator
Sun 06 Feb 2022 07:25:20 PM UTC, comment #16: 

Rik, I'm good with your changes in comment #15.

I didn't touch liboctave yet because it couldn't call anything in libinterp. Should it have its own error-handling, or should the error_unless routines be moved there?

Arun Giridhar <arungiridhar>
Group Member
Sun 06 Feb 2022 05:30:14 AM UTC, comment #15: 

I reviewed version 2 of the patch.  I made a few changes and am uploading as bug61753.cset.  I think it is ready for one more review by Arun and then can be pushed to Mercurial.

There are still some issues with the use of error_if and error_unless.  Occasionally the code, for example in daspk.cc, is using these functions for what amounts to input validation.  The problem is that all of these functions are meant for tests which are present while debugging the code.  When the NDEBUG macro is set to true these functions, and their checking, are removed from the code during compilation.  For these instances, which require input validation, the error_if and error_unless instances should be changed to the normal pattern of


if (COND)
  error (...)


Lastly, no work has yet been done on the liboctave/ directory.

(file #52813)

Rik <rik5>
Group administrator
Fri 07 Jan 2022 07:03:11 PM UTC, comment #14: 

@Arun: I won't have time (work) until this weekend to review, but will do so then.

Rik <rik5>
Group administrator
Wed 05 Jan 2022 10:09:25 PM UTC, comment #13: 

Could you also take a look at libgnu/assure.h to see if those macros can be used?

Arun Giridhar <arungiridhar>
Group Member
Wed 05 Jan 2022 09:10:09 PM UTC, comment #12: 

Updated patch attached. This one at least compiles and passes all BISTs. I created a new macro "error_unless" and I've used panic_unless for all of the parser and for stack-frame.cc. Most of the others seem to be error not panic, but it definitely needs cross-checking.

(file #52616)

Arun Giridhar <arungiridhar>
Group Member
Wed 05 Jan 2022 12:52:36 AM UTC, comment #11: 

I think this project is slightly more involved than just replacing assert with panic_unless.  The goal is actually to remove as many land mines from the code as possible which means replacing assert with calls to error() where possible and only as a last resort doing a call to panic_unless() or panic_impossible.

I believe one key to distinguishing between the two cases is whether the code maintains state.  For example, the parser maintains state about where it is in an input stream and what token it is processing.  If it encounters a condition that it believes is impossible it certainly can't go forward and continue processing, nor can it go backwards and just return to the Octave prompt because now the interpreter will be lost as to where it is in the input stream.  In this case executing an abort() seems like the only way out.

On the other hand, if Octave is just performing a calculation (which therefore doesn't have any state) and it runs into trouble it can just stop the calculation, discard any calculated results, and return to the Octave prompt.

As an example, the first change in the diff you made is


diff -r 2f1fae9dd79d libinterp/corefcn/cellfun.cc
--- a/libinterp/corefcn/cellfun.cc        Tue Jan 04 08:58:31 2022 +0100
+++ b/libinterp/corefcn/cellfun.cc        Tue Jan 04 18:10:41 2022 -0500
@@ -1990,8 +1990,8 @@
 do_mat2cell_2d (const Array2D& a, const Array<octave_idx_type> *d, int nd)
 {
   Cell retval;
-  assert (nd == 1 || nd == 2);
-  assert (a.ndims () == 2);
+  panic_unless (nd == 1 || nd == 2);
+  panic_unless (a.ndims () == 2);

   if (mat2cell_mismatch (a.dims (), d, nd))
     return retval;


do_mat2cell_2d() is an internal function that is eventually called when a user executes mat2cell().  There is no reason this sort of input validation couldn't be replaced with calls to error() which simply send the user back to the Octave prompt.

In this case, even error() is unnecessary.  A search through the code reveals do_mat2cell_2d is called from only 3 places and they all have code that guarantees 2-D matrices.  For example, the dispatch for do_mat2cell is


template <typename ArrayND>
Cell
do_mat2cell (const ArrayND& a, const Array<octave_idx_type> *d, int nd)
{
  if (a.ndims () == 2 && nd <= 2)
    return do_mat2cell_2d (a, d, nd);


which already checks the necessary conditions.

Rik <rik5>
Group administrator
Wed 05 Jan 2022 12:00:06 AM UTC, comment #10: 

It's a little tricky, but in the 3 instances that are not compiling the object in question is a symbol_record which is defined in symrec.h.  The relevant code is


    bool is_valid (void) const { return m_rep->is_valid (); }

    explicit operator bool () const { return is_valid (); }


There C++ overload for bool operators (like '!') is marked with the "explicit" keyword which means the compiler is not going to automatically compare types and deduce that it needs to call this operator.  There is probably a reason for this so I would not go changing the class definition of symbol_record in symrec.h.

Instead, change failing instances to read


panic_unless (sym.is_valid ());


Rik <rik5>
Group administrator
Tue 04 Jan 2022 11:11:58 PM UTC, comment #9: 

Here is a WIP patch for libinterp (attached). I used the panic_unless form to make the search-replace a little easier.

There are build errors with specific locations in stack-frame.cc about not being able to convert sym to boolean. Not sure how to resolve this. Thoughts?

  CXX      libinterp/corefcn/libcorefcn_la-stack-frame.lo
../libinterp/corefcn/stack-frame.cc:1697:5: error: no matching function for call to 'panic_unless'
    panic_unless (sym);
    ^~~~~~~~~~~~
../libinterp/corefcn/error.h:512:6: note: candidate function not viable: no known conversion from 'octave::symbol_record' to 'bool' for 1st argument
void panic_unless (bool cond)
     ^
../libinterp/corefcn/stack-frame.cc:2229:5: error: no matching function for call to 'panic_unless'
    panic_unless (sym);
    ^~~~~~~~~~~~
../libinterp/corefcn/error.h:512:6: note: candidate function not viable: no known conversion from 'octave::symbol_record' to 'bool' for 1st argument
void panic_unless (bool cond)
     ^
../libinterp/corefcn/stack-frame.cc:2396:5: error: no matching function for call to 'panic_unless'
    panic_unless (sym);
    ^~~~~~~~~~~~
../libinterp/corefcn/error.h:512:6: note: candidate function not viable: no known conversion from 'octave::symbol_record' to 'bool' for 1st argument
void panic_unless (bool cond)
     ^
3 errors generated.



(file #52613)

Arun Giridhar <arungiridhar>
Group Member
Tue 04 Jan 2022 12:05:40 AM UTC, comment #8: 

That's fine with me.  Of course, I used to code a lot of Perl so I'm very familiar with the two constructs


STATEMENT if (condition);
STATEMENT unless (condition);


I agree that it would make transitioning the existing instances of assert either.

Rik <rik5>
Group administrator
Mon 03 Jan 2022 11:28:07 PM UTC, comment #7: 

Suggestion that I would like your feedback on. Instead of panic_if, consider negating the logic and calling it panic_unless. That way, nearly all assert statements can be replaced by panic_unless without having to negate the logic individually for each calling location. Might be scriptable. Thoughts?

Arun Giridhar <arungiridhar>
Group Member
Mon 03 Jan 2022 08:01:55 PM UTC, comment #6: 

Attaching two lists of assert() instances to review in the liboctave/ and libinterp/ directories.

(file #52606, file #52607)

Rik <rik5>
Group administrator
Mon 03 Jan 2022 07:25:56 PM UTC, comment #5: 

Attached is a stab at writing a function "panic_if (COND)" which is just syntactic sugar for


if (COND)
  panic_impossible ();


The function is declared inline so there should never be any function calling overhead.  The body of the function is enclosed by


#ifndef NDEBUG
...
#endif


so if NDEBUG is defined during compilation the entire code construct should be optimized out by the compiler.

I put this new function in error.h since that is where panic_impossible() is.  Any code already using panic_impossible() should be including error.h.

@jwe: Could you review for correctness?

Final thought, can the macro "panic_impossible" also be replaced by a C++ inline function?


Rik <rik5>
Group administrator
Sun 02 Jan 2022 07:07:12 PM UTC, comment #4: 

Thank you for the catch! That was indeed a typo. Now the tests all work without new failures.

Updated patch attached.

(file #52601)

Arun Giridhar <arungiridhar>
Group Member
Sun 02 Jan 2022 03:05:56 PM UTC, comment #3: 

From end of patch:

   std::string
   token::superclass_class_name (void) const
   {
-    assert (m_type_tag == scls_name_token);
+    if (m_type_tag == scls_name_token)
+      panic_impossible ();
     return m_tok_info.m_superclass_info->m_class_name;
   }


That one is probably a typo. (Reversed if condition.)

Markus Mützel <mmuetzel>
Group administrator
Sat 01 Jan 2022 09:11:10 PM UTC, comment #2: 

Updated patch attached.

I see that make check is failing in bug-47680/bug-47680.tst with this error:


........................................panic: impossible state reached in file '../libinterp/parse-tree/token.cc' at line 138
fatal: caught signal Aborted -- stopping myself...


Should the parse tree code continue to use assert, or should that test be edited to accommodate panic_impossible instead of assert?



(file #52595)

Arun Giridhar <arungiridhar>
Group Member
Sat 01 Jan 2022 08:48:45 PM UTC, comment #1: 

I would not include error.h in token.h unless functions from error.h are needed in some inline functions in token.h.  Instead, include error.h in token.cc.

John W. Eaton <jwe>
Group administrator
Sat 01 Jan 2022 05:58:59 PM UTC, original submission:  

Following Rik's project description here: https://octave.discourse.group/t/project-to-review-and-replace-c-assert-with-error-calls/2043

A trial patch is attached for all the parse tree functions. Please check if this is the right thing to do, and if so I will do more.

Arun Giridhar <arungiridhar>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #52813:  bug61753.cset added by rik5 (34KiB - application/octet-stream)
file #52616:  WIP2_assert.patch added by arungiridhar (26KiB - text/x-patch)
file #52613:  WIP1_assert.patch added by arungiridhar (26KiB - text/x-patch)
file #52606:  libinterp.assert.list added by rik5 (5KiB - application/octet-stream)
file #52607:  liboctave.assert.list added by rik5 (3KiB - application/octet-stream)
file #52605:  panic_if.cset added by rik5 (1KiB - application/octet-stream)

 

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 mmuetzel (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by arungiridhar (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
    2024-03-23 jwe Planned ReleaseNone 10.1.0 (current default)
    2022-04-09 jwe CategoryNone Coding Style and Maintenance
    2022-02-06 rik5 Attached File- Added bug61753.cset, #52813
        Item GroupNone Other
    2022-01-05 arungiridhar Attached File- Added WIP2_assert.patch, #52616
    2022-01-04 arungiridhar Attached File- Added WIP1_assert.patch, #52613
    2022-01-03 rik5 Attached File- Added libinterp.assert.list, #52606
        Attached File- Added liboctave.assert.list, #52607
    2022-01-03 rik5 Attached File- Added panic_if.cset, #52605
    2022-01-03 rik5 StatusNone In Progress
    2022-01-02 arungiridhar Attached File- Added parsetree_assert_v3.patch, #52601
    2022-01-01 arungiridhar Attached File- Added parsetree_assert_v2.patch, #52595
    2022-01-01 arungiridhar Attached File- Added parsetree_assert_v1.patch, #52593

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code