Undefined behavior sanitizing with Clang

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

Undefined behavior sanitizing with Clang

Philipp Kutin
Hi,


I built Octave using Clang from SVN and
"-fsanitize=undefined,address", both dynamic checkers for C and C++.
The first one, UBSan, instruments the generated code to catch
"miscellaneous" undefined behavior such as casts of overlarge FP
numbers to integers. A starting point for reading is [1]. Some
background info can be found in [2] and [3]. The AddressSanitizer [4]
is a bit different -- it catches stuff like out-of-bounds accesses
(like the one fixed with patch #8152) and traps immediately. In
contrast, UBSan only writes diagnostics to standard error, presumably
since the kind of UB it catches is "less evil" (but still UB!). I'm
enabling the latter simply because I don't want to have
combinatorially many Octave builds lying around; this post is about
UBSan results.

After builing the classdef branch (actually classdef-pk), I ran the
test suite using "make check" and directed its standard output and
error streams to a file. That file, annotated with comments (in square
brackets) and symbolized where UBSan gave only addresses, is attached
to this mail. The file is best viewed in Emacs, some regexps to
highlight are provided at the beginning.


As expected with any large software project, the sanitizer exposed a
couple of issues, both of the interesting and the rather boring
variety. Let's start with the not-so-interesting ones.

* scripts/io/textscan.m:
liboctinterp.so.1:0x1772cb99: runtime error: load of value 4294963199,
which is not a valid value for type 'std::_Ios_Fmtflags'
[
std::operator&=(std::_Ios_Fmtflags&, std::_Ios_Fmtflags)
/usr/lib/gcc/x86_64-linux-gnu/4.7/../../../../include/c++/4.7/bits/ios_base.h:98
4294963199 is 0xffffefff, std::_Ios_Fmtflags is an enum.
]
This one is in the category "mixing signed and unsigned integers when
the highest bit is set". I think it should be harmless, but I haven't
yet read C++'s conversion rules for basic types in depth.

Another one in randmtzig.c is similar:
* randmtzig.c:271:71: runtime error: left shift of 147 by 24 places
cannot be represented in type 'int'
[
            entropy[n++] = word[0]+(word[1]<<8)+(word[2]<<16)+(word[3]<<24);
]
In C99, left-shifts invoke UB if a set bit gets shifted into or past
the highest bit position. However, I recall a GCC bug report [5] where
one of the devs asserted that GCC considers this defined behavior (as
long as the shift value is legal, of course), yielding the "expected"
result on the assumption that signed integers are represented using
two's complement. Fixing this should be easy -- just cast word[3] to
uint32_t before the shift.


Then there are prevalent messages about divisions by zero -- note that
this is undefined behavior in C99 (6.5.5#5) and C++11 (5.6#4)!
[At least in the drafts I'm using -- N843 and N3242, respectively.]
It's already obtained with something as simple as "1/0" from the
prompt. This one is actually a bit tricky. As far as I can see, C99
(haven't looked at C++11) doesn't specify a method of dividing in the
expected "silent" way (1/0 -> inf, 0/0 -> nan, etc.) So it might seem
that implementing divison necessiates resorting to assembly, but
curiously Intel's x86 instructions manual has a table for the FDIV
instruction that actually lists some combinations as producing traps!
I dunno...


Now about the more interesting ones.

Running "norm (single ([1e200, 1]))" gives
* liboctinterp.so.1:0xfc52e49: runtime error: value 1e+200 is outside
the range of representable values of type 'float'
and makes the test (expecting the norm to be 1e200) fail for me,
giving Inf instead.

Then there's
liboctave.so.1:0x1000c320: runtime error: value -1 is outside the
range of representable values of type 'unsigned long'
[
regexp::replace(std::string const&, std::string const&)
/home/pk/dl/octave-classdef-clang/build/liboctave/../../liboctave/util/regexp.cc:611
          from = static_cast<size_t> (p->end () - 1) + 1;
]
Here, an apparently floating-point -1 is cast to size_t, which is is
unsigned long on my system. (If the -1 was integral, it wouldn't be UB
per C++11 4.7#2.) I don't know which test is responsible for that one,
though.

In the same category is the following:
liboctave.so.1:0xe244b00: runtime error: value -8 is outside the range
of representable values of type 'unsigned int'
[
octave_rand::set_internal_state(ColumnVector const&)
/home/pk/dl/octave-classdef-clang/build/liboctave/../../liboctave/numeric/oct-rand.cc:675
    tmp[i] = static_cast<uint32_t> (s.elem (i));
% Can be had with
> rand("state", -8)
]


Finally, and this one looks really rather dangerous,
* dim-vector.cc:101:9: runtime error: signed integer overflow: 3 *
-1094795586 cannot be represented in type 'int'

Unfortunately, I don't know where it comes from. At one point, "make
check" appears to hang and has to be Ctrl-C'd.


If this mail reads a bit like a dump without context, sorry for that
:). It's sort of a compound bug report I wanted to initiate some
discussion on. Because, if I elaborated, the mail would have been even
more TLDR. Feel free to ask for details.


Greetings,
Philipp


[1] http://blog.regehr.org/archives/963
[2] http://blog.regehr.org/archives/213
[3] http://blog.llvm.org/2011/05/what-every-c-programmer-should-know.html
[4] http://clang.llvm.org/docs/AddressSanitizer.html
[5] http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54027

MAKECHECK_CLANG_SAN.log (12K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Undefined behavior sanitizing with Clang

Philipp Kutin
Updating a bit. For the divides-by-zero, I guess I should be reading
IEEE-754, which seems to specify the "silent" behavior in corner
cases. (Suggested on Stackoverflow [1]). Still, it's curious that the
C and C++ Standards just take the easy route of UB. So probably that
sub-discussion should be taken to the Clang UBSan people instead.


The one with the overlarge double->float cast comes from this test in data.cc:
  assert (norm (single ([1e200, 1])), single (1e200))

However, this test doesn't make terribly much sense IMO. 1e200 is not
representable as a finite single-precision FP number, so (assuming
that 1e200 casts to Inf) it really collapses to
  assert (norm (single ([Inf, 1])), single (Inf))
which is true (and passes for me), but probably not what the intent of
the test was.

The failure was actually from another test, here's the output:
  ***** assert (log2 (complex (0,Inf)), Inf + log2 (i))
!!!!! test failed
assert (log2 (complex (0, Inf)),Inf + log2 (i)) expected
Inf + 2.266i
but got
Inf - NaNi
NaNs don't match

The test passes with an Octave built with GCC, so I don't want to
exclude that my Clang build is dodgy (meaning, either the Clang which
was compiled with GCC, or the Octave that was compiled with that
Clang).


However, there are still some cases left that deserve attention IMO.
First, when running the test
  assert (normest (A), norm (A), 1e-6),
the PRNG is initialized to a negative state in normest.m:
  rand ("state", trace (A));
  % (probably)-->
  rand("state", -8)

In oct-rand.cc, that value is cast to a uint32_t. The problem with
this is that you'll likely to get an arbitrary but fixed value for
that cast. Here's a small C program:

----------
#include <stdio.h>
int main() {
    printf("%u\n", (unsigned)(-1.0));
    printf("%u\n", (unsigned)(-2.0));
}
----------

Compiling it with {GCC, G++, Clang} x {-O0, -O2} always yields 0 as
the result for both casts. So in effect, the comment
  ## Set random number generator to depend on target matrix
  v = rand ("state");
  rand ("state", trace (A));
is only partially true (whenever A has negative trace, the state will
be identical).

Personally, I'd prefer Octave's rand() to error whenever values not in
the range of uint32_t are passed as state initialization, it would be
the most honest approach. But other ways of "fixing" are thinkable,
too.


It would be most interesing to get to the source of this one
dim-vector.cc:101:9: runtime error: signed integer overflow: 3 *
-1094795586 cannot be represented in type 'int'

Could anyone give a hint on why "make check" appears to hang for me,
or maybe how to run the test suite with a bit more control, excluding
some tests? It seems that from this point on, some graphics
functionality is tested (copyobj.m)...

[1] http://stackoverflow.com/questions/7267838/division-by-zero-does-not-throw-sigfpe

--Philipp
Reply | Threaded
Open this post in threaded view
|

Re: Undefined behavior sanitizing with Clang

John W. Eaton
Administrator
In reply to this post by Philipp Kutin
On 08/05/2013 12:11 PM, Philipp Kutin wrote:
> Finally, and this one looks really rather dangerous,
> * dim-vector.cc:101:9: runtime error: signed integer overflow: 3 *
> -1094795586 cannot be represented in type 'int'
>
> Unfortunately, I don't know where it comes from. At one point, "make
> check" appears to hang and has to be Ctrl-C'd.

Which test is running when this message appears?

jwe

Reply | Threaded
Open this post in threaded view
|

Re: Undefined behavior sanitizing with Clang

Mike Miller
In reply to this post by Philipp Kutin
On Wed, Aug 7, 2013 at 13:41:48 +0200, Philipp Kutin wrote:
> The one with the overlarge double->float cast comes from this test in data.cc:
>   assert (norm (single ([1e200, 1])), single (1e200))
>
> However, this test doesn't make terribly much sense IMO. 1e200 is not
> representable as a finite single-precision FP number, so (assuming
> that 1e200 casts to Inf) it really collapses to
>   assert (norm (single ([Inf, 1])), single (Inf))
> which is true (and passes for me), but probably not what the intent of
> the test was.

Probably. You'll note there's also a couple of lines not far from there:

  %! flo = single(1e-300);
  %! fhi = single(1e+300);

This seems to me like a copy/paste from the double-precision section.

> The failure was actually from another test, here's the output:
>   ***** assert (log2 (complex (0,Inf)), Inf + log2 (i))
> !!!!! test failed
> assert (log2 (complex (0, Inf)),Inf + log2 (i)) expected
> Inf + 2.266i
> but got
> Inf - NaNi
> NaNs don't match

I get the same result with Debian clang 3.3-4.

> However, there are still some cases left that deserve attention IMO.
> First, when running the test
>   assert (normest (A), norm (A), 1e-6),
> the PRNG is initialized to a negative state in normest.m:
>   rand ("state", trace (A));
>   % (probably)-->
>   rand("state", -8)
>
> In oct-rand.cc, that value is cast to a uint32_t. The problem with
> this is that you'll likely to get an arbitrary but fixed value for
> that cast. Here's a small C program:
>
> ----------
> #include <stdio.h>
> int main() {
>     printf("%u\n", (unsigned)(-1.0));
>     printf("%u\n", (unsigned)(-2.0));
> }
> ----------
>
> Compiling it with {GCC, G++, Clang} x {-O0, -O2} always yields 0 as
> the result for both casts. So in effect, the comment
>   ## Set random number generator to depend on target matrix
>   v = rand ("state");
>   rand ("state", trace (A));
> is only partially true (whenever A has negative trace, the state will
> be identical).

Not exactly. Try the following amended test program instead. Literals
and variables are treated differently in the case of casting to an
unsigned.

---
#include <stdio.h>
double num() { return -2.0; }
int main() {
  printf("%u\n", (unsigned)(-1.0));
  printf("%u\n", (unsigned)(-2.0));
  printf("%u\n", (unsigned)(num()));
}
---

> Personally, I'd prefer Octave's rand() to error whenever values not in
> the range of uint32_t are passed as state initialization, it would be
> the most honest approach. But other ways of "fixing" are thinkable,
> too.

If Matlab's rand('state',s) accepts negative numbers, then ours does
as well. The best we can do is map any possible input values to the
uint32_t array in a deterministic way.

> It would be most interesing to get to the source of this one
> dim-vector.cc:101:9: runtime error: signed integer overflow: 3 *
> -1094795586 cannot be represented in type 'int'
>
> Could anyone give a hint on why "make check" appears to hang for me,
> or maybe how to run the test suite with a bit more control, excluding
> some tests? It seems that from this point on, some graphics
> functionality is tested (copyobj.m)...

I don't see either of these problems with my build (Debian clang
3.3-4, no sanitize options used).

--
mike
Reply | Threaded
Open this post in threaded view
|

Re: Undefined behavior sanitizing with Clang

Philipp Kutin
On Wed, Aug 7, 2013 at 3:12 PM, Mike Miller <[hidden email]> wrote:

>
> Not exactly. Try the following amended test program instead. Literals
> and variables are treated differently in the case of casting to an
> unsigned.
>
> ---
> #include <stdio.h>
> double num() { return -2.0; }
> int main() {
>   printf("%u\n", (unsigned)(-1.0));
>   printf("%u\n", (unsigned)(-2.0));
>   printf("%u\n", (unsigned)(num()));
> }
> ---

No. It's not that literals and variables are treated any differently,
it's that converting a double whose truncated value can't be
represented in the target integer type is *undefined behavior*, and as
a consequence, our sample programs are completely devoid of meaning
from the start. A compiler is not obliged to produce any particular
code for them at all.

What I wanted to show with my program is that the behavior I observed
is one possibility, of infinitely many as far as the Standard is
concerned. Usually, with UB code the behavior differs with factors
such as using a different compiler, or the same compiler with
different options, etc. etc. It was a bit unfortunate (for the point I
was trying to make) that in my case, all combinations gave the same
result, but in fact, you succeeded there: your line prints 4294967294
when compiled with -O0, but 0 with -O2.

I really recommend reading the acticles by John Regehr [1] and Chris
Lattner [2]. They shed light on why one cannot "rely" on UB code to
produce any particular result, why deducing from a given compiled
example to the general case is not allowed, and why undefined behavior
will be more of something to keep an eye on as compilers get better at
aggressively optimizing code. It's a bit surprising on the first read,
but makes perfect sense when seen from the compiler's (or its
authors') point of view: after all, its main job is to produce the
fastest possible code, as fast as possible.

> If Matlab's rand('state',s) accepts negative numbers, then ours does
> as well. The best we can do is map any possible input values to the
> uint32_t array in a deterministic way.

I see. Then I guess the most logical way to procees is to reduce each
element modulo 2^32, somewhat like this pseudo-code ("elem" is a
double):

if (isnan (elem))
    elem = <arbitrary-but-fixed>;
else
{
    elem = fmod(elem, 4294967296.0)  // UINT32_MAX+1.0
    // fmod() is the remainder on division that rounds toward 0, i.e.
its sign follows the numerator.
    // we want a number in [0 .. UINT32_MAX+1.0) though.
    if (elem < 0)
        elem = elem += UINT32_MAX+1.0;
}

Because currently, the behavior may only be called "deterministic" (in
big quotes) for a particular build, if the compiler was well-meaning
on that day.

[1] http://blog.regehr.org/archives/213
[2] http://blog.llvm.org/2011/05/what-every-c-programmer-should-know.html

--Philipp
Reply | Threaded
Open this post in threaded view
|

Re: Undefined behavior sanitizing with Clang

Philipp Kutin
In reply to this post by John W. Eaton
On Wed, Aug 7, 2013 at 2:41 PM, John W. Eaton <[hidden email]> wrote:
> Which test is running when this message appears?

Unfortunately, I didn't have any luck in ultimately closing in on it.
The log suggests that it's from copyobj.m, and there appears to be
only one test there, the one conditional on HAVE_MAGICK. (Are demos
considered tests too?) I initially tried setting a breakpoint on
__ubsan_handle_mul_overflow(), but that one didn't get caught even
though the message appeared. So it looks like it's invoked from a
spawned child Octave... presumably the one that executes some external
program. This is as deep as I have gotten, though.

--Philipp
Reply | Threaded
Open this post in threaded view
|

Re: Undefined behavior sanitizing with Clang

Mike Miller
In reply to this post by Philipp Kutin
On Sat, Aug 10, 2013 at 13:53:39 +0200, Philipp Kutin wrote:
> No. It's not that literals and variables are treated any differently,
> it's that converting a double whose truncated value can't be
> represented in the target integer type is *undefined behavior*, and as
> a consequence, our sample programs are completely devoid of meaning
> from the start. A compiler is not obliged to produce any particular
> code for them at all.

True, I wasn't trying to assert that the behavior is defined, although
it probably came off that way. Just that the sample program you came up
with doesn't appear to be handled by the particular compilers we are
looking at in the same way as the suspect code in Octave :)

I agree with your digging into and helping to eliminate UB code in
Octave and thanks for your efforts.

> I see. Then I guess the most logical way to procees is to reduce each
> element modulo 2^32, somewhat like this pseudo-code ("elem" is a
> double):
>
> if (isnan (elem))
>     elem = <arbitrary-but-fixed>;
> else
> {
>     elem = fmod(elem, 4294967296.0)  // UINT32_MAX+1.0
>     // fmod() is the remainder on division that rounds toward 0, i.e.
> its sign follows the numerator.
>     // we want a number in [0 .. UINT32_MAX+1.0) though.
>     if (elem < 0)
>         elem = elem += UINT32_MAX+1.0;
> }
>
> Because currently, the behavior may only be called "deterministic" (in
> big quotes) for a particular build, if the compiler was well-meaning
> on that day.

Right, understand.

Also, to be clear, I wasn't suggesting that Matlab's rand does accept
negative numbers, but *if* it does, then Octave should also. I'm not
sure whether it does or not, someone else will have to confirm.

--
mike
Reply | Threaded
Open this post in threaded view
|

Re: Undefined behavior sanitizing with Clang

Philipp Kutin
Hi,

just build a Clang-SVN -fsanitize=address,undefined build of Octave HG
default again and ran its test suite. Trimmed results are attached.
The summary is that besides a couple of div-by-zero and the known
1e200/1e300-to-float conversion issues, there's only a single other
one, like this:

/home/pk/dl/octave-default/build/libinterp/../../libinterp/corefcn/sparse-xpow.cc:375:
runtime error: value 1.79769e+308 is outside the range of
representable values of type 'int'

This is caused by these kinds of tests in sparse.tst:

%!test bf=realmin;
%% Make sure newly introduced zeros get eaten
%!assert (nnz (sparse ([bf,bf,1]).^realmax), 1)

The C++ code in question,

      if (static_cast<int> (b) != b && a.any_element_is_negative ())

does a cast-to-int on a double that's not necessarily known to be in
the safe range for this conversion, the half-open interval (INT_MIN-1,
INT_MAX+1).

Looking at the top of sparse-xpow.cc, there already seems to exist a
function tailored to that range check, xisint(). However, I'm
wondering why it does the slightly too-strict check INT_MIN < x && x <
INT_MAX instead of the more liberal (and presumably still safe for the
uses that follow) INT_MIN <= x && x <= INT_MAX. (Well, or for
symmetry, INT_MIN < x && x <= INT_MAX.)

So, I'm definitely pretty happy to see such a clean output from the
Clang sanitizers. But then again I'm not sure whether the results now
and a couple of months ago are comparable. Clang seems to optimize
away some expresssions even at -O0 these days (experieced because it
seems harder to get a build useful for debugging), so hopefully it
takes care to insert the checks even then.

Greetings,
Philipp

MAKECHECK_CLANG_CHECK_3.log (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Undefined behavior sanitizing with Clang

Philipp Kutin
On Thu, Nov 14, 2013 at 12:28 PM, Philipp Kutin <[hidden email]> wrote:
> (...) the half-open interval (INT_MIN-1,
> INT_MAX+1).

Oops, I meant "the open interval" here, none of endpoints are valid to
a conversion.

--Philipp
Reply | Threaded
Open this post in threaded view
|

Re: Undefined behavior sanitizing with Clang

Rik-4
In reply to this post by Philipp Kutin
On 11/14/2013 07:29 AM, [hidden email] wrote:
Message: 2
Date: Thu, 14 Nov 2013 12:28:36 +0100
From: Philipp Kutin [hidden email]
To: [hidden email]
Subject: Re: Undefined behavior sanitizing with Clang
Message-ID:
	[hidden email]
Content-Type: text/plain; charset="iso-8859-1"

Hi,

just build a Clang-SVN -fsanitize=address,undefined build of Octave HG
default again and ran its test suite. Trimmed results are attached.
The summary is that besides a couple of div-by-zero and the known
1e200/1e300-to-float conversion issues, there's only a single other
one, like this:

/home/pk/dl/octave-default/build/libinterp/../../libinterp/corefcn/sparse-xpow.cc:375:
runtime error: value 1.79769e+308 is outside the range of
representable values of type 'int'

This is caused by these kinds of tests in sparse.tst:

%!test bf=realmin;
%% Make sure newly introduced zeros get eaten
%!assert (nnz (sparse ([bf,bf,1]).^realmax), 1)

The C++ code in question,

      if (static_cast<int> (b) != b && a.any_element_is_negative ())

does a cast-to-int on a double that's not necessarily known to be in
the safe range for this conversion, the half-open interval (INT_MIN-1,
INT_MAX+1).

Looking at the top of sparse-xpow.cc, there already seems to exist a
function tailored to that range check, xisint(). However, I'm
wondering why it does the slightly too-strict check INT_MIN < x && x <
INT_MAX instead of the more liberal (and presumably still safe for the
uses that follow) INT_MIN <= x && x <= INT_MAX. (Well, or for
symmetry, INT_MIN < x && x <= INT_MAX.)

So, I'm definitely pretty happy to see such a clean output from the
Clang sanitizers. But then again I'm not sure whether the results now
and a couple of months ago are comparable. Clang seems to optimize
away some expresssions even at -O0 these days (experieced because it
seems harder to get a build useful for debugging), so hopefully it
takes care to insert the checks even then.

Greetings,
Philipp
11/14/13

Philipp,

How does this differ from the sanitize log that PrasannaKumar sent to the list 3 days ago?  He says that he used static analysis and it seems that you actually compiled and ran code, but you also said that compiling with -O0 sometimes doesn't report issues.  Should we trust one report more than the other?  Can you also upload the log a bug report so it doesn't get lost?

--Rik

Reply | Threaded
Open this post in threaded view
|

Re: Undefined behavior sanitizing with Clang

PrasannaKumar Muralidharan
> How does this differ from the sanitize log that PrasannaKumar sent to the
> list 3 days ago?

Hi Rik,

I used clang static analyser not clang sanitise - both are different
tools. I should have clarified it before, sorry. Clang static analyser
works at compile time and lists possible issue (from the compiler's
AST). Clang sanitiser on the other hand works at run time, checks for
issues as they occur. Certain issues can be found only when it is
running, for those clang sanitiser will help. Certain issues that can
be found without running code, for those clang static analyser can be
used.

Using both the tool may be necessary for large projects like octave. I
think both the reports should be used.

Thanks,
PrasannaKumar
Reply | Threaded
Open this post in threaded view
|

Re: Undefined behavior sanitizing with Clang

Philipp Kutin
In reply to this post by Rik-4
On Thu, Nov 14, 2013 at 6:58 PM, Rik <[hidden email]> wrote:
> How does this differ from the sanitize log that PrasannaKumar sent to the
> list 3 days ago?

I looked at PrasannaKumar's log too. While it may expose interesting
issues, with static analysis you always have the possibility of false
positives. So you always have to look at the individual reports and
try to reconstruct what the analyzer is trying to complain at. IMO
Clang's static analysis has nicely annotated output, but the false
positive rate is too high [*]. The dynamic "sanitizers" on the other
hand always expose actual issues when they complain, namely constructs
that are undefined behavior according to the respective language
standards.

> He says that he used static analysis and it seems that you
> actually compiled and ran code, but you also said that compiling with -O0
> sometimes doesn't report issues.  Should we trust one report more than the
> other?

What I feared was false negatives, i.e. missed defects with the
sanitizers relative to earlier Clang versions. But that was rather
random speculation on my part. Also, I can now confirm that the output
is the same for an Octave build compiled with -O0 or -O1.

> Can you also upload the log a bug report so it doesn't get lost?

I don't want to say "I'll try" because I still have two of those
pending [**], but I'll see what I can do. Personally, I'd prefer to
resolve this quickly rather than opening a bug report because the
necessary changes and possible effects seem relatively local.

-----

[*] For example, sorting the list by path length and starting with the
shorter ones, even those ones contain false positives because the
analyzer isn't able to "see the whole picture". Such as in the report

API, Argument with 'nonnull' attribute passed null,
libinterp/corefcn/mex.cc, 1376, 5,

the analyzer doesn't see that "pi!=NULL iff val.pi!=NULL" and
concludes that memcpy may be called with a NULL source.

[**] One of those is my "promised" detailed analysis of the usability
of the classdef branch. I'm using it for a while now, and it's
certainly workable.

-----

While on the topic of code analyses, here's an nice blog article IMO,
which also introduces the two analysis "qualities" soundness and
completeness:
http://blog.frama-c.com/index.php?post/2013/04/26/Of-compiler-warnings-discussions


--Philipp
Reply | Threaded
Open this post in threaded view
|

Re: Undefined behavior sanitizing with Clang

PrasannaKumar Muralidharan
> I looked at PrasannaKumar's log too. While it may expose interesting
> issues, with static analysis you always have the possibility of false
> positives. So you always have to look at the individual reports and
> try to reconstruct what the analyzer is trying to complain at. IMO
> Clang's static analysis has nicely annotated output, but the false
> positive rate is too high [*]. The dynamic "sanitizers" on the other
> hand always expose actual issues when they complain, namely constructs
> that are undefined behavior according to the respective language
> standards.
>
>
> [*] For example, sorting the list by path length and starting with the
> shorter ones, even those ones contain false positives because the
> analyzer isn't able to "see the whole picture". Such as in the report
>
> API, Argument with 'nonnull' attribute passed null,
> libinterp/corefcn/mex.cc, 1376, 5,
>
> the analyzer doesn't see that "pi!=NULL iff val.pi!=NULL" and
> concludes that memcpy may be called with a NULL source.

Yes. I did mention about the false positives as it was high in count.
Clang static analyser did a very good job on another project (no false
positives) but it was a bit different with Octave. I feel Octave is a
good candidate to improve Clang static analyser - I do hope to report
the false positives to the Clang static analyser in future and see if
they find it useful.

- PrasannaKumar
Reply | Threaded
Open this post in threaded view
|

Re: Undefined behavior sanitizing with Clang

Philipp Kutin
I started to look at this a bit. My current plan is to split xisint()
into three functions, for simplicity of review:

xisint_sym: the symmetric variant [INT_MIN+1, INT_MAX].
xisint_full: the "normal" variant [INT_MIN, INT_MAX].
xisint: [INT_MIN+1, INT_MIN-1], kept in already correct (but maybe too
strict) code to not confuse anything.

The symmetric one will be used whenever there's the possibility of the
cast value negating, as -INT_MIN is undefined behavior, too.

Right now, I'm a bit confused seeing code like this:

      if (xisint (btmp))
        result(i) = std::pow (a, static_cast<int> (btmp));
      else
        result(i) = std::pow (a, btmp);

The assumption here seems to be that "std::pow(T, int)" will yield a
different result than "std::pow(T, double)" when given an
integer-valued second argument. Why?

--Philipp
Reply | Threaded
Open this post in threaded view
|

Re: Undefined behavior sanitizing with Clang

Mike Miller
On Mon, Nov 18, 2013 at 23:27:58 +0100, Philipp Kutin wrote:

> Right now, I'm a bit confused seeing code like this:
>
>       if (xisint (btmp))
>         result(i) = std::pow (a, static_cast<int> (btmp));
>       else
>         result(i) = std::pow (a, btmp);
>
> The assumption here seems to be that "std::pow(T, int)" will yield a
> different result than "std::pow(T, double)" when given an
> integer-valued second argument. Why?

This is to make use of gcc's std::pow integer specialization, which
does indeed yield a different result when it is available:

  https://github.com/mirrors/gcc/blob/master/libstdc%2B%2B-v3/include/c_global/cmath#L414

This came up on the maintainers list some months ago when it was
noticed that this specialization goes away when using g++ -std=c++11,
see the following discussions and bug report:

http://octave.1599824.n4.nabble.com/round-off-error-in-std-pow-std-complex-lt-T-gt-double-in-C-11-td4649063.html
http://octave.1599824.n4.nabble.com/Welcome-C-11-td4647840.html#a4647903
https://savannah.gnu.org/bugs/?38142

--
mike