[Development] QtCore missing check for memory allocation

[Thiago Macieira]

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
> reality.
> 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 & 
friends).

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