llvm dev - Nov 2016 - PVS-Studio analysis of LLVM code

Hi, Jonas!

On Tue, Nov 1, 2016 at 3:26 AM, Jonas Wagner <jonas.wagner at epfl.ch>
wrote:
> Hi Eugene,
>
> I think this is really cool! You've convinced me to try out PVS on some
of
> my own projects :)
>
> Of all the warnings presented in the article, there was one for which I
> thought it's a false positive. By default, LLVM is compiled without
RTTI and
> exceptions. Because of this, I reasoned that operator new would not throw
an
> exception, and so the checks of its result against nullptr were correct...
> right?
>
> Turns out I'm probably wrong. At least according to this stack overflow
> post. But then, what is the correct way to use operator new when compiling
> with -fno-exceptions? Do we need to add (std::nothrow) to all uses of
`new`?
I think this should be added by Clang itself. Or we need to use STL
compiled with -fno-exceptions (libstdc++ or libc++).
> Best,
> Jonas
Eugene.

Hello,

I think this should be added by Clang itself.


I don't think so. There are several sources online that indicate that
developers need to *manually* specify std::nothrow. If they don't, operator
new will throw an exception even if code is compiled with -fno-exception.
This will abort the program.

At some point, this caused Firefox to replace a bunch of `new` with
`new(std::nothrow)` [1] Would this be a good idea in LLVM as well? Or
should we just let LLVM crash on OOM, and remove the null-checks as PVS
suggests?

Or we need to use STL compiled with -fno-exceptions (libstdc++ or
libc++).
>
Well, in this case PVS would have reported a false positive. Would it be
possible to adjust PVS's V668 <http://www.viva64.com/en/w/V668/>, so
that
it could detect whether std::nothrow is needed or not?

Best,
Jonas

[1]
https://blog.mozilla.org/nnethercote/2011/01/18/the-dangers-of-fno-exceptions/
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20161102/05cf5228/attachment.html>

On Wed, Nov 2, 2016 at 9:41 AM, Jonas Wagner via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> Hello,
>
> I think this should be added by Clang itself.
>
>
> I don't think so. There are several sources online that indicate that
> developers need to *manually* specify std::nothrow. If they don't,
> operator new will throw an exception even if code is compiled with
> -fno-exception. This will abort the program.
>
That's correct. The default new should never return nullptr, even with
-fno-exceptions. It should either succeed, throw an exception, or abort.

At some point, this caused Firefox to replace a bunch of `new`
with
> `new(std::nothrow)` [1] Would this be a good idea in LLVM as well? Or
> should we just let LLVM crash on OOM, and remove the null-checks as PVS
> suggests?
>
LLVM shouldn't replace new with new(std::nothrow), it should just abort.
There's effectively zero chance the codebase will ever be able to recover
from OOMing, and with modern OSes, you're almost never going to actually
see a malloc failure until it's way too late, anyways.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20161102/7bdd1abf/attachment.html>

On Wed, Nov 2, 2016 at 9:41 AM, Jonas Wagner via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> At some point, this caused Firefox to replace a bunch of `new` with
> `new(std::nothrow)` [1] Would this be a good idea in LLVM as well? Or
should
> we just let LLVM crash on OOM, and remove the null-checks as PVS suggests?
For the record, we did not replace `new` with `new (std::nothrow)`.
We define our own versions of global `operator new` such that it
crashes the program on OOM (via abort(3) or moral equivalent) rather
than attempting to throw an exception.  (There are, as you might
imagine, many hoops to jump through to do this.)  We do have
non-crashy versions (the moral equivalents of `new (std::nothrow)`),
of course, but the majority of the codebase uses the crashy `operator
new`.

Having PVS tell us where we aren't supposed to be checking for `new`
failure would be really useful: we made `new` infallible a while back,
but there's still a fair amount of code that unnecessarily checks for
`new` returning nullptr...

-Nathan