Re: Stories from static analysis of the Octave code base

[Rik]

On 01/05/2019 08:32 AM, Daniel J Sebald wrote:
> 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
>

In general, absolutely to your comments.  I saw the same chance for
optimization, but time is a bit short for the 5.0 release so I didn't
pursue it.  I think the best option would be to initialize the values just
once at the very beginning of the interpreter.  At least for the executable
Octave, this would mean keeping the instance in
libinterp/corefcn/interpreter.cc, and removing the others.

The wrinkle is that liboctave is theoretically a standalone library that
others can link in to their C++ code without also linking in the
interpreter in liboctinterp.  Is there a way to guarantee when a library is
loaded that a particular piece of code is executed?  I kind of think not. 
I don't know how many programmers have actually tried to link in liboctave,
but we probably want to preserve the flexibility to do so.

The is_XXX_NA routines are safe because they eventually call routines that
either do octave_ieee_init or do not depend on it at all such as routines
from the std library.

--Rik