PVS-Studio analysis of LLVM code

Hi!

Company behind PVS-Studio analyzed relatively recent version of LLVM
and some results are reported in Finding bugs in the code of LLVM project with the help of PVS-Studio.

Eugene.

Hi Eugene,

I think this is really cool! You’ve convinced me to try out PVS on some of my own projects :slight_smile:

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?

Best,
Jonas

Hi, Jonas!

Hi Eugene,

I think this is really cool! You've convinced me to try out PVS on some of
my own projects :slight_smile:

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, 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/

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.

LLVM shouldn't replace new with new(std::nothrow), it should just abort.

Does it? When we were evaluating embedding LLVM a few years back, that was
one of the major pain points for us. Something goes wrong and LLVM/Clang
calls abort, crashing the whole program. Library shouldn't make these type
of decisions lightly: return an error, throw an exception, fire up a smoke
signal - whatever - but there's just not enough context to decide right at
that level.

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.

IMO the best way is to allow user to specify their own allocation strategy
with default fallback to std::new: JITs have bursts of activity between
longer quiet periods and may want to defer memory cleanup until later; IDE
with code completion may want to keep memory consumption in check and
choose to disable whole feature, rather than impede other processes; and so
on.

- Yury

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

LLVM shouldn't replace new with new(std::nothrow), it should just abort.

Does it? When we were evaluating embedding LLVM a few years back, that was
one of the major pain points for us. Something goes wrong and LLVM/Clang
calls abort, crashing the whole program. Library shouldn't make these type
of decisions lightly: return an error, throw an exception, fire up a smoke
signal - whatever - but there's just not enough context to decide right at
that level.

I agree you could make that argument for many of the places LLVM uses
assert() today. LLVM uses assert not just in checking invariants, but as an
error handling mechanism.

However, the level of extra complexity that would be required in order to
try to handle *memory allocation failure* is not worth it. Not even close
to worth it. There's only VERY limited classes of programs where that is a
worthwhile thing to try to do (example: PID 1). A compiler library isn't
one of them.

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.

IMO the best way is to allow user to specify their own allocation strategy
with default fallback to std::new: JITs have bursts of activity between
longer quiet periods and may want to defer memory cleanup until later; IDE
with code completion may want to keep memory consumption in check and
choose to disable whole feature, rather than impede other processes; and so
on.

The global operator new is already overridable per the C++ standard. And,
malloc/free themselves are overridable, in practice, on common systems.
LLVM doesn't need to do anything special to allow users to plug in their
own allocator if they want to.