[Development] QtCore missing check for memory allocation
thiago.macieira at intel.com
Fri Feb 27 01:12:27 CET 2015
On Thursday 26 February 2015 20:54:32 Oswald Buddenhagen wrote:
> > Let me put it this way: who's going to write the unit tests to ensure we
> > get coverage for all those conditionals? Any volunteers?
> which conditionals? the malloc wrapper would throw/qFatal (depending on
> the build configuration). your dream of never-failing malloc would be a
> or we "just" use new() everywhere. this one already has said wrapper.
The conditionals for checking the result of malloc or new (nothrow). If no one
is testing, how can we check that we caught all places?
> regarding replacing the malloc implementation ... is it possible to link
> an implementation that would actually throw?
No, malloc() cannot ever throw. If you throw and your caller is C code, you
crash. Also, malloc() is defined as nothrow() in the glibc headers.
The best it could do is raise(SIGABRT) or somesuch.
> > Sorry, the compiler *is* allowed to remove undefined behaviour because, by
> > the very definition of undefined behaviour, ANYTHING can happen,
> > including absolutely nothing.
> then explain edd2d9bd0a7f5dbe059aea0902d519b728acc01a.
Who says that's good code? I fixed a warning, I didn't make the code good.
If the compiler starts removing the undefined behaviour, we'll get a test
failure and we can easily correct it. We can't do that in a library because we
don't know when it will fail.
> > > > > [...] Q_CHECK_PTR should mean "If the pointer is 0 either throw
> > > > > an exception or abort right away. Don't just continue."
> > > >
> > > > I understand your arguments, but I still disagree we should act.
> > >
> > > well, and i say you are wrong. see the problem with this kind of
> > > argumentation?
> > We've both exposed our technical argumentation for our suggestions but
> > arrived at no consensus.
> you missed the point. you didn't provide arguments, only an opinion (at
The argument is that it implies runtime overhead. See Robin's email for
numbers. This is asking for making the code slower on the very devices where
it needs to run faster.
The argument is that we're in the habit of not checking it, so even if we had
a massive effort to fix all the cases, we'll soon add more that we don't check.
The argument is that we have no infrastructure for testing OOM situations to
guarantee that we reliably take the action we decided. And no one is
volunteering to write the tests. Just sprinkling Q_CHECK_PTR all over the
place is no good if we miss critical ones.
The argument is that this is a massive change. Most of our modules (except for
QtCore, QtConcurrent, QtXmlPatterns) are compiled with exceptions disabled and
yet we use the regular, non-nothrow operator new, so we'd need to replace it
EVERYWHERE with the nothrow version plus the check.
The argument is that we use libraries that don't care either for OOM
situations. So even if we made the effort ourselves, it might be all for
nothing as soon as we make a call to one of those libraries (think glib &
The argument is that even if we happened to catch all failures to malloc() in
all libraries, there's also all those pesky system calls that may return
ENOMEM and we're not checking those. So wrapping malloc() and operator new are
not enough: we need to check system calls too. And extend that to all the
calls to libraries that do catch malloc() and system call failures and report
errors to us.
The argument is that chain is as strong as its weakest link. If we don't fix
all the holes, the bucket will still leak (pardon my switch of metaphors).
The argument is that, even with all of this, there's no way to reliably catch
an OOM situation, due to overcommit. The application may suddenly disappear.
And the conclusion, which is not technical but subjective, based on the
complexity of the problem as stated above that this is a waste of our time
when we have more important things to do. We've had this discussion before and
we're now spending time discussing it again. And if we were to accept it, we'd
spend a lot of development time that could be better directed elsewhere.
> the point of the discussion this quote refers to). it isn't constructive
> to state your position without backing it up. if you stand by your
> previously made arguments, post links, or refer to a search engine
> (possibly suggesting keywords). not everyone witnessed the previous
> discussions, or remembers them.
And even if I had no technical arguments, my opinion counts. I'm not pulling
this out of thin air. I have over a decade of experience, so if I say that it
sounds wrong, it probably is. And I'm the maintainer for QtCore, so I'm the
one that needs to be convinced, not the other way around.
Thiago Macieira - thiago.macieira (AT) intel.com
Software Architect - Intel Open Source Technology Center
More information about the Development