bugGNU Octave - Bugs: bug #60531, dbstack(N) not omitting innermost...

 
 

bug #60531: dbstack(N) not omitting innermost frames as it should

Submitted by:  Sébastien Villemot <svillemot>
Submitted on:  Wed 05 May 2021 12:47:32 PM UTC  
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Regression
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  Closed
Release:  6.2.0 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)

Thu 20 May 2021 11:20:49 AM UTC, comment #10: 

Seems to be building for 32bit targets now.

Closing as fixed.

Markus Mützel <mmuetzel>
Project Member
Tue 18 May 2021 07:03:42 AM UTC, comment #9: 

Thanks. Good idea. That will simplify the code and avoid similar issues in the future.

I pushed a fix for a typo that caused the build to fail for 32bit platforms here:
https://hg.savannah.gnu.org/hgweb/octave/rev/26ad902a6749

Markus Mützel <mmuetzel>
Project Member
Mon 17 May 2021 08:50:04 PM UTC, comment #8: 

The ambiguity occurs when there is no idx_vector(int) constructor and there is no best choice when the options are char, long int, and pointer to idx_base_rep.  You can see the same behavior with the following simple example:

class foo
{
public:
  foo (char c) { }
  // foo (int i) { }
  foo (long int li) { }
  foo (void *p) { }
};

int main (void)
{
  foo x (0);
  return 0;
}

For idx_vector, this problem can happen when octave_idx_type and F77_INT are both int64_t.  It doesn't happen if F77_INT is int.

I pushed the following change to ensure that there is always an idx_vector(int) constructor defined:

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

John W. Eaton <jwe>
Project Administrator
Mon 17 May 2021 03:07:43 PM UTC, comment #7: 

It's not clear for me if this is a compiler bug. It might be:
https://en.cppreference.com/w/cpp/language/integer_literal

IIUC, an integer literal matches int, long int or long long int. I believe "octave_idx_type" is "long long int" for that target. Unfortunately, the error message is truncated. So I can't tell if there was another exact match to any of those types. Maybe "octave_f77_int_type"? It might be "int" or "long int". But I'm not sure.

Anyway. The buildbot builders seem to be fine with that change.

Closing as fixed.

Markus Mützel <mmuetzel>
Project Member
Sun 16 May 2021 09:25:31 PM UTC, comment #6: 

I think your patch is fine and should resolve the issue.  I also think it's a pretty poor compiler that can't match the constant '0' to octave_idx_type (which is either int32 or int64).

Because performance isn't an issue, I would have put the static_cast in to the for loop rather than declaring a new variable because I think that makes the code clearer.  But, it's not a big deal.

Rik <rik5>
Project Administrator
Sun 16 May 2021 07:45:39 AM UTC, comment #5: 

The windows64-64 builder is broken since today.
That might be related to this change.
From the buildbot logs:

[build]    default-octave
Failed to build package default-octave!
------------------------------------------------------------
      |     ^~~~~~~~~~
/scratch/buildbot/workers/jwe-debian-x86_64-2/w64-64-on-debian/src/tmp-default-octave/octave-7.0.0/liboctave/array/idx-vector.h:482:5: note: candidate: 'octave::idx_vector::idx_vector(char)'
  482 |     idx_vector (char c) : m_rep (new idx_colon_rep (c)) { }
      |     ^~~~~~~~~~
/scratch/buildbot/workers/jwe-debian-x86_64-2/w64-64-on-debian/src/tmp-default-octave/octave-7.0.0/liboctave/array/idx-vector.h:453:5: note: candidate: 'octave::idx_vector::idx_vector(octave_idx_type)'
  453 |     idx_vector (octave_idx_type i) : m_rep (new idx_scalar_rep (i)) { }
      |     ^~~~~~~~~~
/scratch/buildbot/workers/jwe-debian-x86_64-2/w64-64-on-debian/src/tmp-default-octave/octave-7.0.0/liboctave/array/idx-vector.h:441:5: note: candidate: 'octave::idx_vector::idx_vector(octave::idx_vector::idx_base_rep*)'
  441 |     idx_vector (idx_base_rep *r) : m_rep (r) { }
      |     ^~~~~~~~~~
make[4]: *** [Makefile:19136: libinterp/corefcn/libcorefcn_la-debug.lo] Error 1
make[4]: *** Waiting for unfinished jobs....

Hopefully fixed with this change:
https://hg.savannah.gnu.org/hgweb/octave/rev/e84f7449a0d8

Markus Mützel <mmuetzel>
Project Member
Sat 15 May 2021 01:16:45 AM UTC, comment #4: 

I changed the code to delete stackframes when dbstack (N) is used with an output argument.  I also updated the second output argument from dbstack so that it is correctly invalid (return value of 0) when the number of skipped frames exceeds the current frame index.  See changeset http://hg.savannah.gnu.org/hgweb/octave/rev/68d64190da65.

Marking as Ready for Test.

Rik <rik5>
Project Administrator
Thu 06 May 2021 01:09:50 AM UTC, comment #3: 

I think it was just a mistake to not handle nskip there.  Some care will also be needed to adjust the current frame index correctly.

We could also add an nskip parameter to the backtrace functions in the call_stack class but there are a bunch of different variants of that function.  Should they all get the nskip paramter?  Probably it is simpler to just limit nskip to the do_dbstack function for now.

John W. Eaton <jwe>
Project Administrator
Wed 05 May 2021 11:52:30 PM UTC, comment #2: 

Perhaps jwe knows why this changed.  The code is in do_dbstack in the file debug.cc.  The interesting bit is

  octave::tree_evaluator& tw = interp.get_evaluator ();

  if (nargout == 0)
    {
      octave_map stk = tw.backtrace (curr_frame);
      octave_idx_type nframes = stk.numel ();

      if (nframes > 0)
        {
          octave::preserve_stream_state stream_state (os);

          os << "stopped in:\n\n";

          Cell names = stk.contents ("name");
          Cell files = stk.contents ("file");
          Cell lines = stk.contents ("line");

          bool show_top_level = true;

          std::size_t max_name_len = 0;

          for (octave_idx_type i = nskip; i < nframes; i++)
            {
              std::string name = names(i).string_value ();

              max_name_len = std::max (name.length (), max_name_len);
            }

          for (octave_idx_type i = nskip; i < nframes; i++)
            {
              std::string name = names(i).string_value ();
              std::string file = files(i).string_value ();
              int line = lines(i).int_value ();

              if (show_top_level && i == curr_frame)
                show_top_level = false;

              os << (i == curr_frame ? "  --> " : "      ")
                 << std::setw (max_name_len) << name
                 << " at line " << line
                 << " [" << file << ']'
                 << std::endl;
            }

          if (tw.at_top_level () && show_top_level)
            os << "  --> top level" << std::endl;
        }
    }
  else
    {
      octave_map stk = tw.backtrace (curr_frame, false);

      // If current stack frame is not in the list curr_frame will be
      // -1 and either nskip caused us to skip it or we are at the top
      // level, which is not included in the list of frames.  So in the
      // interpreter, 0 will be our invalid frame index value.

      retval = ovl (stk, curr_frame + 1);
    }

When the true branch of the if statement is taken, nskip is used and only the frames requested are printed.  This matches the behavior I mentioned in comment #1.  When the else branch is taken, Octave just gets the entire stack frame and returns it.

Maybe there is a way to call in to the interpreter and get just the number of stack_frames desired.  I looked at pt-eval.h and call-stack.h, but I wasn't sure.

Alternatively, all of the stack frames are returned in an octave_map (i.e., a struct array).  One could check for nskip > 0 and if necessary, create a new appropriately sized octave_map and copy over the desired stack frames in a for loop.  Alternatively, one could use an index() call.  If this were in an m-file, one would write

new_map = old_map(nskip:end);

It should be possible to do something similar in C++.

Or one could keep the old map and use delete_elements() to get rid of the frames one didn't need.

I've added jwe because I think he might instantly have an intuition about the best approach.

Rik <rik5>
Project Administrator
Wed 05 May 2021 09:37:41 PM UTC, comment #1: 

Confirmed.  Interestingly, this seems to be an issue only when there is an output argument.  If I set a breakpoint in the second file with "dbstop bar", then execute foo, then inspect things with "dbstack" and "dbstack(1)", I find that the output printed to screen is correct.

Rik <rik5>
Project Administrator
Wed 05 May 2021 12:47:32 PM UTC, original submission:  

Hi,

I noticed a regression in Octave 6 with respect to dbstack(N).

Suppose I have the following two files:

foo.m:

function foo
  bar
end

bar.m:

function bar
  size(dbstack(1), 1)
end

Under Octave 6, calling foo will display:

ans = 2

It turns out that the innermost frame has not been omitted.

While under Octave 5 (and MATLAB), one gets the correct answer:

ans = 1

Sébastien Villemot <svillemot>

 

(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 mmuetzel (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by rik5
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by svillemot (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
    2021-05-20 mmuetzel StatusReady For Test => Fixed
        Open/ClosedOpen => Closed
    2021-05-18 mmuetzel StatusFixed => Ready For Test
        Open/ClosedClosed => Open
    2021-05-17 mmuetzel StatusReady For Test => Fixed
        Open/ClosedOpen => Closed
    2021-05-15 rik5 StatusConfirmed => Ready For Test
    2021-05-05 rik5 Carbon-Copy- => Added jwe
    2021-05-05 rik5 StatusNone => Confirmed

    Back to the top


    Powered by Savane 3.7