[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Stories from static analysis of the Octave code base
From: |
Daniel J Sebald |
Subject: |
Re: Stories from static analysis of the Octave code base |
Date: |
Sat, 5 Jan 2019 11:32:26 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 |
On 01/04/2019 02:35 PM, Rik wrote:
All,
It's been a humbling experience going through the issues detected by static
analysis of the Octave code base
(https://wiki.octave.org/PVS_static_analyzer_-_5.0_Release). Frankly, I
thought the code base was better than it turned out to be. Besides flat
out calling the wrong function (e.g., double_value instead of
xdouble_value), I just found and corrected a surprisingly simple but
pervasive performance hit for any exceptional value (-Inf, Inf, NaN).
The code in question is deep in liboctave at liboctave/util/lo-ieee.cc.
The representation of an IEEE exceptional value on the particular HW Octave
is running on is found and then stored away so that subsequent uses just
retrieve that value rather than re-deriving it each time. The
initialization code was
void
octave_ieee_init (void)
{
bool initialized = false;
if (! initialized)
{
... code to find and store exceptional values ...
initialized = true;
}
}
As you can see, initialized is always false so the longer code to derive
the exceptional values is run every single time. And the exceptional
values themselves are functions which end up calling octave_ieee_init
twice: once for the "double" value and once for the "single" value.
The compiler should have issued a "test always false" type of warning
for that...
There might be a small non-bug change in order beyond what you modified
in Rev #26425. If I understand correctly, before the change something like
x = Inf(1000);
would have a performance hit because of the reinitialization for every
instance of IEEE inf.
However, there are multiple locations where there is assurance of
initialization:
libinterp/corefcn/interpreter.cc: octave_ieee_init ();
liboctave/util/lo-ieee.cc: octave_ieee_init ();
liboctave/util/lo-ieee.cc: octave_ieee_init ();
liboctave/util/lo-ieee.cc: octave_ieee_init ();
liboctave/util/lo-ieee.cc: octave_ieee_init ();
liboctave/util/lo-ieee.cc: octave_ieee_init ();
liboctave/util/lo-ieee.cc: octave_ieee_init ();
The first check is when an instance of the interpreter is created, in
the constructor. The remaining are when there is a particular call to
actually set +/-Inf or NA. I would think that only one of those two
groups is needed, but not both.
If the IEEE initialization is checked only once upon creating the
interpreter then Octave would avoid quite a bit of checking in the
x=inf(1000) case, but that would also mean that Octave would immediately
abort if the Inf/NA aren't present when an interpreter is created for
any particular non-Inf/NA script.
Instead, if the IEEE initialization is checked only whenever an Inf/NA
assignment is done, then Octave would have a chance to run properly so
long as it didn't need Inf/NA.
Also, despite their name, Octave uses exceptional values quite a bit. For
example, graphics handles are often initialized to NaN until they can be
assigned a true handle.
Then that makes more of a case for having the octave_ieee_init() as
mandatory in interpreter.cc. [One could temporarily force the
occurrences of octave_ieee_init() in lo-ieee.cc to abort and see if that
function is used anywhere outside the interpreter prior to instantiation
of the interpreter.]
Something else I'm wondering about is what would happen in the case
where there is no octave_ieee_init() in interpreter::interpreter() and
the NA/Inf arises via CPU/ALU arithmetic operations (e.g., 1 DIV 0) as
opposed to assignment. In other words, if there were no instance of
octave_ieee_init() in interpreter::interpreter, could there be
situations where the non-IEEE float fails even when none of the
abort-plus-informative-message checks happen and one simply gets
abort-via-crash?
And what about within lo-ieee.cc itself? There are currently no
octave_ieee_init() within the routines
__lo_ieee_is_NA (double x)
__lo_ieee_is_old_NA (double x)
__lo_ieee_float_is_NA (float x)
Couldn't the user run a script, say "isinf(5)" or "isnan(20)", before
actually creating a variable with value +/-Inf or NA?
Dan