bugGNU Octave - Bugs: bug #65029, VM - deal() makes Octave crash

 
 

bug #65029: VM - deal() makes Octave crash

Submitter:  Philip Nienhuis <philipnienhuis>
Submitted:  Wed 13 Dec 2023 08:07:45 PM UTC
   
 
Category:  None Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Segfault, Bus Error, etc.
Status:  Fixed Assigned to:  None
Originator Name:  Philip Nienhuis Open/Closed:  * Closed
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

Wed 10 Jan 2024 10:06:06 PM UTC, comment #15: 

Tested and pushed to https://hg.savannah.gnu.org/hgweb/octave/rev/90d3b8ad2a98

Leaving as Fixed, but if anyone gets errors, pls post here and it can be reopened if necessary.

Arun Giridhar <arungiridhar>
Group Member
Wed 10 Jan 2024 07:57:27 PM UTC, comment #14: 

The attached patch implements inputname for the bytecode frame class.


Implement inputname for bytecode interpreter (bug #65029)

The way inputname is resolved was changed in the main branch.
Non-bytecode user function frames called from a bytecode frame
need to lookup inputname in the caller bytecode frame.

* stack-frame.cc: Implement inputname() for bytecode frames


(file #55552)

Petter <petter>
Mon 08 Jan 2024 06:19:26 PM UTC, comment #13: 

I pushed the following change to avoid the crash.  The bytecode_fcn_stack_frame::inputname function still needs to be implemented to make the inputname function work properly again when using the bytecode interpreter but I don't know how to do that.

http://hg.savannah.gnu.org/hgweb/octave/rev/b951807bfde3

John W. Eaton <jwe>
Group administrator
Mon 08 Jan 2024 05:20:41 PM UTC, comment #12: 

I'd be surprised if this cset provoked that regression, although obviously I cannot rule it out.

FWIW, on Linux and Windows all 4 __run_test_suite__ (with and without __vm_enable__ ) invocations ran fine.
In my work (where I consistently use bleeding edge dev Octave unless proven unstable) I see no regressions or issues compared to before applying the cset. (Of course during the Xmas holidays I didn't use Octave daily, but often enough to be confident).

Can I help digging further?

Philip Nienhuis <philipnienhuis>
Group Member
Mon 08 Jan 2024 10:46:23 AM UTC, comment #11: 

The function at that line:

std::string
stack_frame::inputname (int, bool) const
{
  // This function should only be called for user_fcn_stack_frame.
  // Anything else indicates an error in the implementation.

  panic_impossible ();
}


So, it's probably not this exact commit that caused the abort. But one of the accompanying merge commits.

Markus Mützel <mmuetzel>
Group administrator
Mon 08 Jan 2024 10:43:24 AM UTC, comment #10: 

This (or another change?) might have caused the following regression in CI:
https://github.com/gnu-octave/octave/actions/runs/7442369941/job/20245702191#step:14:49

panic: impossible state reached in file '../libinterp/corefcn/stack-frame.cc' at line 3321
fatal: caught signal Aborted -- stopping myself...
Aborted (core dumped)
  libinterp/corefcn/conv2.cc-tst .................................make[1]: Leaving directory '/home/runner/work/octave/octave/.build/test'
make[1]: *** [Makefile:3278: check-tree-evaluator] Error 134
make: *** [Makefile:32595: check-tree-evaluator] Error 2
make: Leaving directory '/home/runner/work/octave/octave/.build'


Markus Mützel <mmuetzel>
Group administrator
Mon 08 Jan 2024 02:09:42 AM UTC, comment #9: 

Sorry for the delay. Just pushed this already-reviewed patch. Closing as fixed.  https://hg.savannah.gnu.org/hgweb/octave/rev/2d3074d82ab3

Arun Giridhar <arungiridhar>
Group Member
Thu 21 Dec 2023 09:11:44 PM UTC, comment #8: 

Works fine on Windows as well.
Thanks very much for fixing this, Petter!

Status => Patch reviewed

Could 0ne of the core devs please push, & close this report as "Fixed"? Thx.

Philip Nienhuis <philipnienhuis>
Group Member
Thu 21 Dec 2023 07:31:03 PM UTC, comment #7: 

Thanks again :-)

OK, patch works on Linux.
Next, Windows ...

Philip Nienhuis <philipnienhuis>
Group Member
Thu 21 Dec 2023 05:13:45 PM UTC, comment #6: 

Oops. Wrong patch sorry.

I hopefully attached the correct one this time. '32995.patch'


Fix bytecode interpreter stack overflow with varargout (bug #65029)

Don't push varargout elements to the stack when returning from
bytecode functions to non-bytecode functions. Don't push more elements
to the stack then needed when returning from bytecode to bytecode.

* pt-bytecode-vm.cc: Avoid overflow
* bytecode_varargout.m: Update tests


(file #55475)

Petter <petter>
Thu 21 Dec 2023 10:56:40 AM UTC, comment #5: 

Thanks Petter
I tried the patch but on Linux (Mageia-8) I still get the crash.
Is it the patch for this bug? in the commit message you refer to bug #64977

Philip Nienhuis <philipnienhuis>
Group Member
Wed 20 Dec 2023 08:35:20 PM UTC, comment #4: 

The attached patch should do it.

Instead of pushing varargout's elements to the stack, they are put in an octave_value_list, when the bytecode interpreter is returning to non-bytecode functions.

There is some cheating in the bytecode interpreter and '[bshp(1:nlay).Geometry] = deal (tmp{:})' is executed by an eval() due to dynamic nargout.

When support to do that in the bytecode interpreter is added, some way to handle the same problem need to be considered for bytecode to bytecode function returns.


Fix bytecode interpreter stack overflow with varargout (bug #65029)

Don't push varargout elements to the stack when returning from
bytecode functions to non-bytecode functions. Don't push more elements
to the stack then needed when returning from bytecode to bytecode.

pt-bytecode-vm.cc: Avoid overflow
bytecode_varargout.m: Update tests


(file #55474)

Petter <petter>
Thu 14 Dec 2023 09:54:41 PM UTC, comment #3: 

Thanks for the report. I was able to replicate the issue.

deal() makes the bytecode interpreter stack overflow when it pushed varargout's elements to the stack.

I got a fix for it, but I'll might need to figure out a more general solution so that varargout can be big.

Petter <petter>
Thu 14 Dec 2023 10:14:50 AM UTC, comment #2: 

cc'ing petter

Philip Nienhuis <philipnienhuis>
Group Member
Wed 13 Dec 2023 08:50:52 PM UTC, comment #1: 

Crashes on Linux as well.

Philip Nienhuis <philipnienhuis>
Group Member
Wed 13 Dec 2023 08:07:45 PM UTC, original submission:  

Steps to reliably reproduce (on Win10):

load vm_deal.mat  # attached
nlay = 20000
tmp = repmat ({'Line'}, 1, nlay);
[bshp(1:nlay).Geometry] = deal (tmp{:})

nlay = 16000 still goes fine on my box.
nlay = 17000 leads to a silent crash.

After '__vm_enable__ (0)' there's no crash.

Has been working fine for years on Matlab and Octave.
Don't know if it's a regression as it only crashes using the bytecode interpreter :-)

Philip Nienhuis <philipnienhuis>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #55552:  octave_33151.patch added by petter (2KiB - text/x-patch)
file #55475:  32995.patch added by petter (8KiB - text/x-patch)
file #55474:  32841.patch added by petter (2KiB - text/x-patch)
file #55448:  vm_deal.mat added by philipnienhuis (28KiB - application/octet-stream)

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

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

    Date Changed by Updated Field Previous Value => Replaced by
    2024-01-10 petter Attached File- Added octave_33151.patch, #55552
    2024-01-08 arungiridhar StatusPatch Reviewed Fixed
        Open/ClosedOpen Closed
    2023-12-28 philipnienhuis Dependencies- bugs #65057 is dependent
    2023-12-21 philipnienhuis StatusIn Progress Patch Reviewed
    2023-12-21 petter Attached File- Added 32995.patch, #55475
    2023-12-20 petter Attached File- Added 32841.patch, #55474
    2023-12-15 philipnienhuis StatusNone In Progress
    2023-12-14 philipnienhuis Carbon-Copy- Added petter
    2023-12-13 philipnienhuis Operating SystemMicrosoft Windows Any
    2023-12-13 philipnienhuis Attached File- Added vm_deal.mat, #55448

    Back to the top

    Powered by Savane 3.13-04b1.
    Corresponding source code