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

 
 

bug #61753: Changing assert to panic_impossible

Submitted by:  Arun Giridhar <arungiridhar>
Submitted on:  Sat 01 Jan 2022 05:58:59 PM UTC  
 
Category:  None Severity:  3 - Normal
Priority:  5 - Normal Item Group:  None
Status:  In Progress Assigned to:  None
Originator Name:  Open/Closed:  Open
Release:  dev Operating System:  Any

Add a New Comment (Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

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>
Project 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>
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>
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>
Project 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>
Project 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>
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>
Project 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>
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>
Project 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>
Project 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>
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>
Project Member
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>
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>
Project 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>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
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)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    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.7