[Mesa-dev] [RFC PATCH 0/4] proposed scan-build cleanups

[Giuseppe Bilotta]

Just for kicks, I tried building mesa with Clang 4.0 scan-build, to see
if there were any ‘easy picks’ up for fixing. The build produces lots of
warnings, so going through each of them will require time and care, but
at least some of them seem relatively obvious.

I'm not sure what the recommended approach to follow in this case would
be (a single huge patch series, individual patches for each warning,
larger patches for individual classes of warnings, or anything else), so
I come to the list with an RFC about methodology, and while I'm at it
also about a couple of patches, as general examples of what to do for
individual _kinds_ of errors.

The ones I present specifically are:

1. the addition of a check to malloc return value; this was rather
trivial per se, but for example while going to through the source code I
noticed that for _reallocs_ the ‘safe’ approach is generally not
followed (store the old pointer before the realloc so that if the
realloc fails we don't get a memory leak), and I was wondering if a
patchset to fortify `realloc()` would be worth it (especially
considering that there's mostly small reallocs and if they fail the
situation is catastrophic enough that the memory leak would be the last
of the problems).

2. an early bailout for some checks; this is a minor optimization, and
while the rest of the logic is not deeply affected because there would
be a return-with-error anyway later in the code , bailing out early does
avoid scan-build warnings about uninitisalized values being used (even
if the subsequent assignments would be discarded anyway by the error
return in the end).

3. and 4. are the ones I am least confident about. I spotted them while
following one of the scan-build paths about use of uninitisalized
values, and essentially replace a few

	assert(0);
	return something anyway;

with

	unreachable("some motivation");

The reason why I am not very confident about them is that the paths
that lead to them are often quite long, and I'm not _entirely_ sure
that those switch values are actually unreachable rather than just
being _unlikely_ unless a client is horribly broken, and if pretending
to return something (even simply a bogus uninitialized value) would
still be a safer alternative. More in general, I would like to know if
the `assert + return` has a specific semanting difference from the
`unreachable`, or if it's just e.g. a leftover from an older approach.

Thanks and best regards,

Giuseppe Bilotta (4):
  mesa/pack: check malloc return value
  glsl: early bailout if local size too large
  ff_fragment_shader: mark impossible switch values with unreachable
  mesa get: unreachable

 src/compiler/glsl/ast_to_hir.cpp     |  4 ++--
 src/mesa/main/ff_fragment_shader.cpp | 18 +++++++-----------
 src/mesa/main/get.c                  |  3 +--
 src/mesa/main/pack.c                 |  5 +++++
 4 files changed, 15 insertions(+), 15 deletions(-)

-- 
2.13.0.rc0.207.gb442654931