warnings shut off in ov.cc?

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

warnings shut off in ov.cc?

Rik-4
jwe,

Can you try the attached patch which implements a warning when non-scalar
arguments are used with the colon operator?  Everything looks correct, and
the printf debugging that I started using when I got confused shows that
the code is executing, but no warnings are printed.  The code is exactly
the same format as the warning for complex values using in colon
expressions, however that warning fires.  Is there something that disables
warnings temporarily in the interpreter when calling operators?

Sample session:

1:[3,4]
running do_colon_op
Got here
Got here2
ans =

   1   2   3

whereas

octave:1> 1:3*i
running do_colon_op
warning: imaginary part of complex colon arguments is ignored
ans = [](1x0)

Maybe this is a red herring, but occasionally warnings seem to be turned
off for the complex code path as well.

octave:2> 1:(3i)
running do_colon_op
ans = [](1x0)

The last example should also have produced a warning.

--Rik

warncolon.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: warnings shut off in ov.cc?

John W. Eaton
Administrator
On 10/15/19 2:21 PM, Rik wrote:

> jwe,
>
> Can you try the attached patch which implements a warning when non-scalar
> arguments are used with the colon operator?  Everything looks correct, and
> the printf debugging that I started using when I got confused shows that
> the code is executing, but no warnings are printed.  The code is exactly
> the same format as the warning for complex values using in colon
> expressions, however that warning fires.  Is there something that disables
> warnings temporarily in the interpreter when calling operators?
>
> Sample session:
>
> 1:[3,4]
> running do_colon_op
> Got here
> Got here2
> ans =
>
>     1   2   3
>
> whereas
>
> octave:1> 1:3*i
> running do_colon_op
> warning: imaginary part of complex colon arguments is ignored
> ans = [](1x0)
>
> Maybe this is a red herring, but occasionally warnings seem to be turned
> off for the complex code path as well.
>
> octave:2> 1:(3i)
> running do_colon_op
> ans = [](1x0)
>
> The last example should also have produced a warning.

I think you are seeing the effect of constant folding in the parser:

 
http://hg.savannah.gnu.org/hgweb/octave/file/5a0543de1e47/libinterp/parse-tree/oct-parse.yy#l2493

Warnings are intentionally disabled when doing constant folding
operations.  The original idea may have been the same as what we should
be doing for errors, which is to be silent about them when attempting
constant folding, but if there is an error (or warning), fail to
generate a constant value so that the expression will generate an error
(or warning) later at run time.  This is clearly not happening for warnings.

Unfortunately, I don't think that skipping the message but detecting the
warning is an easy thing to do.

Maybe we should just eliminate the constant folding operations in the
parser?  I'm not sure that they really provide that much advantage.

jwe

Reply | Threaded
Open this post in threaded view
|

Re: warnings shut off in ov.cc?

Rik-4
On 10/15/2019 01:26 PM, John W. Eaton wrote:

> On 10/15/19 2:21 PM, Rik wrote:
>> jwe,
>>
>> Can you try the attached patch which implements a warning when non-scalar
>> arguments are used with the colon operator?  Everything looks correct, and
>> the printf debugging that I started using when I got confused shows that
>> the code is executing, but no warnings are printed.  The code is exactly
>> the same format as the warning for complex values using in colon
>> expressions, however that warning fires.  Is there something that disables
>> warnings temporarily in the interpreter when calling operators?
>>
>> Sample session:
>>
>> 1:[3,4]
>> running do_colon_op
>> Got here
>> Got here2
>> ans =
>>
>>     1   2   3
>>
>> whereas
>>
>> octave:1> 1:3*i
>> running do_colon_op
>> warning: imaginary part of complex colon arguments is ignored
>> ans = [](1x0)
>>
>> Maybe this is a red herring, but occasionally warnings seem to be turned
>> off for the complex code path as well.
>>
>> octave:2> 1:(3i)
>> running do_colon_op
>> ans = [](1x0)
>>
>> The last example should also have produced a warning.
>
> I think you are seeing the effect of constant folding in the parser:
>
>
> http://hg.savannah.gnu.org/hgweb/octave/file/5a0543de1e47/libinterp/parse-tree/oct-parse.yy#l2493
>
>
> Warnings are intentionally disabled when doing constant folding
> operations.  The original idea may have been the same as what we should
> be doing for errors, which is to be silent about them when attempting
> constant folding, but if there is an error (or warning), fail to generate
> a constant value so that the expression will generate an error (or
> warning) later at run time.  This is clearly not happening for warnings.
>
> Unfortunately, I don't think that skipping the message but detecting the
> warning is an easy thing to do.
>
> Maybe we should just eliminate the constant folding operations in the
> parser?  I'm not sure that they really provide that much advantage.

Thanks, that is definitely the issue.  I tried two ways of making the
expression non-constant and, sure enough, the warning appears.

x = 4
1:[x, 10]

and

1:[3+1, 10]

What about just not discarding warning messages?  I commented out this line

//    es.discard_warning_messages (true);

and then everything works as expected (no duplicate warning messages either).

Just to be sure, I added more printf test code and even though the warning
is issued, the path through the constant logic is still taken.  Maybe the
unwind_protect frame is only required to capture errors, but warnings are
fine since they return to the original code and it keeps executing?

--Rik

Reply | Threaded
Open this post in threaded view
|

Re: warnings shut off in ov.cc?

John W. Eaton
Administrator
On 10/15/19 5:13 PM, Rik wrote:

> What about just not discarding warning messages?  I commented out this line
>
> //    es.discard_warning_messages (true);
>
> and then everything works as expected (no duplicate warning messages either).

That's OK for the command line where you are parsing and executing the
code once.  But in a function, it means you'll get the warning when the
function is parsed and not when it is executed.

> Just to be sure, I added more printf test code and even though the warning
> is issued, the path through the constant logic is still taken.  Maybe the
> unwind_protect frame is only required to capture errors, but warnings are
> fine since they return to the original code and it keeps executing?

Since my recent changes to error handling, error messages are stored in
the execution_exception object so they are not printed until the
exception is caught and processed, so we have the option of ignoring
them and detecting that an error has occurred.  That happens with the
try/catch block and doesn't rely on the unwind_protect object.

Warnings are printed immediately.  We still have a way to discard
warning messages globally, but I'm not sure that's a good idea either.

See the thread

 
https://lists.gnu.org/archive/html/octave-maintainers/2019-10/msg00009.html

jwe

Reply | Threaded
Open this post in threaded view
|

Re: warnings shut off in ov.cc?

Rik-4
On 10/15/2019 02:26 PM, John W. Eaton wrote:

> On 10/15/19 5:13 PM, Rik wrote:
>
>> What about just not discarding warning messages?  I commented out this line
>>
>> //    es.discard_warning_messages (true);
>>
>> and then everything works as expected (no duplicate warning messages
>> either).
>
> That's OK for the command line where you are parsing and executing the
> code once.  But in a function, it means you'll get the warning when the
> function is parsed and not when it is executed.

True, although I don't have an easy command to parse the file, but not
execute it.

I created junk.m with

function r = junk ()
  r = [1, 2] : 5;
endfunction

and the first time the function is used the code gets parsed and the
warning is issued.

octave:2> y = junk ()
warning: colon arguments should be scalars
y =

   1   2   3   4   5

But the second time the code is run there is no warning

octave:3> z = junk ()
z =

   1   2   3   4   5

It is possible to detect that a warning has been issued and avoid constant
folding, although the mechanism is clumsy.  This diff does it

diff -r 5a0543de1e47 libinterp/parse-tree/oct-parse.yy
--- a/libinterp/parse-tree/oct-parse.yy Wed Oct 09 14:35:03 2019 +0200
+++ b/libinterp/parse-tree/oct-parse.yy Tue Oct 15 17:56:41 2019 -0700
@@ -2531,7 +2531,23 @@ namespace octave
           {
             tree_evaluator& tw = interp.get_evaluator ();
 
+            std::string oldmsg = es.last_warning_message ();
+            if (oldmsg.empty ())
+              {
+                oldmsg = "INTERPRETER";
+                es.last_warning_message (oldmsg);
+              }
+            else
+              {
+                oldmsg = "";
+                es.last_warning_message (oldmsg);
+              }
             octave_value tmp = e->evaluate (tw);
+            if (es.last_warning_message () != oldmsg)
+              {
+                retval = e;
+                return retval;
+              }
 
             tree_constant *tc_retval
               = new tree_constant (tmp, e->line (), e->column ());

There may be a more clever way to use frame.protect_var to restore the
member variable m_last_warning_message.

To benchmark things, I created this function

function [a,b,c,d] = tst_rangefold ()
  a = 1:100;
  b = 1:3:99;
  c = -1:.01:+1;
  d = 0:.001:1;
endfunction

Then I benchmarked things using

N = 1e4;
bm = zeros (N,1);
for i = 1:N
  tic;
  [a,b,c,d] = tst_rangefold ();
  bm(i) = toc;
  clear a b c d
endfor

The mean runtime (mean (bm)) for various configurations is

Baseline: 1.3343e-05
Warn Detection: 1.3592e-05
No Constant Folding: 1.5367e-05

So a 2% slowdown to add warning detection, but a 15% slowdown if constant
folding is removed altogether.  Although, let's be real, even if constant
folding is removed entirely the absolute difference is 2 microseconds which
isn't very meaningful.

Given that a future JIT might benefit from knowing when a range was
constant at compile time, I think we should leave the code in.  If you have
a more clever way of testing the lastwarn variable that would be good. 
Otherwise, I can implement what I have.

--Rik




Reply | Threaded
Open this post in threaded view
|

Re: warnings shut off in ov.cc?

John W. Eaton
Administrator
On 10/15/19 9:15 PM, Rik wrote:

> It is possible to detect that a warning has been issued and avoid constant
> folding, although the mechanism is clumsy.  This diff does it

Thanks for the patch.  I made a similar change but used an
unwind_protect object and did the same for array objects.  I checked it
in here:

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

> To benchmark things, I created this function
>
> [...]
>
> So a 2% slowdown to add warning detection, but a 15% slowdown if constant
> folding is removed altogether.  Although, let's be real, even if constant
> folding is removed entirely the absolute difference is 2 microseconds which
> isn't very meaningful.

A large array object might show a much larger difference, and I think it
is worth doing the constant folding if possible.

jwe



Reply | Threaded
Open this post in threaded view
|

Re: warnings shut off in ov.cc?

Rik-4
On 10/16/2019 12:47 PM, John W. Eaton wrote:
> On 10/15/19 9:15 PM, Rik wrote:
>
>> It is possible to detect that a warning has been issued and avoid constant
>> folding, although the mechanism is clumsy.  This diff does it
>
> Thanks for the patch.  I made a similar change but used an unwind_protect
> object and did the same for array objects.  I checked it in here:
>
>   http://hg.savannah.gnu.org/hgweb/octave/rev/c3e24c82157f

I thought there must be an easier way.  With your change in place I easily
completed the rest of the changeset which adds two new warning IDs when the
colon operator is called in questionable ways.  The first ID is
Octave:colon-complex-arg and is triggered by an expression such as 1:5i. 
The second ID is Octave:colon-nonscalar-arg and is triggered by an
expression such as 1:[3,5].  This ID is disabled when --traditional mode is
used since Matlab doesn't validate their inputs as tightly as Octave.  See
https://hg.savannah.gnu.org/hgweb/octave/rev/85ad4689aa05.

Final note, I had written such a complex warning detection scheme to handle
this oddball case I found where the interpreter produces different results
for the same input (because of global state held in the lastwarn() variable).

octave:1> 1:warning('xyz'):5
warning: xyz
ans = [](1x0)
octave:2> 1:warning('xyz'):5
warning: xyz
warning: colon arguments should be scalars
ans = 1
octave:3> lastwarn ("")
octave:4> 1:warning('xyz'):5
warning: xyz
ans = [](1x0)

Up to you whether this is really worth fixing.  I wouldn't bother.

--Rik