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

[Nicolai Hähnle]

On 22.04.2017 09:52, Giuseppe Bilotta wrote:
> 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.

Definitely split the changes up along a combination of where they are in 
the source and what types of errors they are. I'd say the split you've 
chosen looks reasonable to me.


> 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).

I'm generally in favor of fixing our use of realloc. I occasionally do 
that in code that I'm working on anyway.


> 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).

Looks good.


> 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.

Well, you've more or less already explained the difference: the asset + 
return will actually return something possibly meaningful in optimized 
builds, whereas with unreachable, almost anything might happen: it's 
basically at the whim of the compiler.

For example: In the case of large switch statements that end up being 
lowered into jump tables, the unreachable will probably end up jumping 
to an arbitrary address and likely crash, while for small switch 
statements, an arbitrary non-unreachable path is likely to be executed.

For the most part, I think the use of assert + return is there because 
the code was written at a time when unreachable builtins simply didn't 
exist. I'm generally in favor of using unreachable; it contains an 
assert, so testing on builds with assertions enabled will catch them.

I have one remark on patch 3. Patches 1,2,4 are:

Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>


>
> 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(-)
>


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.