Before we go cleaning up LLVM+Clang of all Static Analyzer Warnings...

Project Infrastructure LLVM Dev List Archives
1

Hi Apelete,

Thanks for trying to help cleanup the LLVM codebase of Clang Static Analyzer warnings.

But it seems a lot of the fixes that are being proposed are somewhat mechanical and may be doing the wrong thing in a few ways.

  • Initializing variables that are only used when initialized through some existing codepath - this can make tools like Memory Sanitizer less useful, because now the value is initialized even in some path where that value is never intended to be used

  • Adding assertions for pointers that are known to be non-null - in some cases this is helpful, when the algorithm that ensures the non-null-ness is sufficiently opaque. But for function parameters - we have /lots/ of non-null function parameters. I think it’d be impractical to assert on all of them.

But if we want to do something like this we should probably have some community discussion about how how to go about any of these things across the codebase. Maybe using nonnull attributes would be a better approach to the parameter issue, for example - but it’ll still be a lot of code churn that there should probably be general consensus on rather than approaching it piecemeal with reviewers in different parts of the codebase.

Thanks,

  • Dave
2

Any reason we don't use more references? I like references because when I read code it is very clear that the argument *can't* be null and I don't have to think if I need to handle the null case.
When I made the DataLayout "mandatory" on the Module last year, I moved every single use of the DataLayout to be a reference, and this helped to remove each and every places that were null-checking the DL.

3

>
> Hi Apelete,
>
> Thanks for trying to help cleanup the LLVM codebase of Clang Static
Analyzer warnings.
>
> But it seems a lot of the fixes that are being proposed are somewhat
mechanical and may be doing the wrong thing in a few ways.
>
> * Initializing variables that are only used when initialized through
some existing codepath - this can make tools like Memory Sanitizer less
useful, because now the value is initialized even in some path where that
value is never intended to be used
>
> * Adding assertions for pointers that are known to be non-null - in some
cases this is helpful, when the algorithm that ensures the non-null-ness is
sufficiently opaque. But for function parameters - we have /lots/ of
non-null function parameters. I think it'd be impractical to assert on all
of them.

Any reason we don't use more references? I like references because when I
read code it is very clear that the argument *can't* be null and I don't
have to think if I need to handle the null case.
When I made the DataLayout "mandatory" on the Module last year, I moved
every single use of the DataLayout to be a reference, and this helped to
remove each and every places that were null-checking the DL.

I'm a big fan of references as well - but there's still some (read: a lot
of) community/codebase momentum behind certain entities being referenced by
pointer because their pointer identity is so intrinsic to their nature
(comparing instruction pointers, etc). Not that one can't take the address
of a thing and compare it, stuff it in a container, etc.

- Dave

4

Hi Apelete,

Thanks for trying to help cleanup the LLVM codebase of Clang Static

Analyzer warnings.

But it seems a lot of the fixes that are being proposed are somewhat

mechanical and may be doing the wrong thing in a few ways.

* Initializing variables that are only used when initialized through

some existing codepath - this can make tools like Memory Sanitizer less
useful, because now the value is initialized even in some path where that
value is never intended to be used

* Adding assertions for pointers that are known to be non-null - in some

cases this is helpful, when the algorithm that ensures the non-null-ness is
sufficiently opaque. But for function parameters - we have /lots/ of
non-null function parameters. I think it'd be impractical to assert on all
of them.

Any reason we don't use more references? I like references because when I
read code it is very clear that the argument *can't* be null and I don't
have to think if I need to handle the null case.
When I made the DataLayout "mandatory" on the Module last year, I moved
every single use of the DataLayout to be a reference, and this helped to
remove each and every places that were null-checking the DL.

I'm a big fan of references as well - but there's still some (read: a lot
of) community/codebase momentum behind certain entities being referenced by
pointer because their pointer identity is so intrinsic to their nature
(comparing instruction pointers, etc). Not that one can't take the address
of a thing and compare it, stuff it in a container, etc.

Note that Chris didn't have very positive experience with agressive use of references in LLVM APIs: [LLVMdev] Guidance on using pointers vs. references for function arguments

5

Hi David,

Hi Apelete,

Thanks for trying to help cleanup the LLVM codebase of Clang Static
Analyzer warnings.

But it seems a lot of the fixes that are being proposed are somewhat
mechanical and may be doing the wrong thing in a few ways.

From the beginning I wondered for each fix if it was the right way of

solving the possible underlying issue pointed to by the Analyzer.
Since I'm not familiar with the codebase, when I was not able to
decide by reading the code I ended-up trusting the tool until advised
otherwise by the reviewers.

Once put together, the fixes do look somewhat mechanical indeed due to
the fact that the same patterns were found throughout the codebase by
the analyzer.
As of this writing, these patterns result in 90+ warnings still
emitted when scanning LLVM+Clang+Compiler-RT (down from 150+ warnings
when I fooled myself into starting to look at it a few weeks ago).

I'm glad you pointed out some of the patterns in the fixes, let's see
how I can address those:

* Initializing variables that are only used when initialized through some
existing codepath - this can make tools like Memory Sanitizer less useful,
because now the value is initialized even in some path where that value is
never intended to be used

* I guess this case could be related to ⚙ D19971 [scan-build] fix warnings emitted on LLVM TargetRecip code base
  and ⚙ D19975 [scan-build] fix warning emitted on LLVM Bitcode code base for instance.
  I wasn't happy with the fixes in the first place but I
  ended-up trusting the tool because it proposed a codepath where the
  variables were used without being initialized. I don't know about
  msan (yet :slight_smile: but if the warnings are false positives is there a way
  to tell scan-build in such cases ?

* Adding assertions for pointers that are known to be non-null - in some
cases this is helpful, when the algorithm that ensures the non-null-ness is
sufficiently opaque. But for function parameters - we have /lots/ of
non-null function parameters. I think it'd be impractical to assert on all
of them.

* This is where I ended-up asserting function parameters in a
  mechanical manner to some extent (as a result of 40+ warnings about
  some object pointers being null). Let's take ⚙ D19385 [scan-build] fix warnings emitted on Clang Format code base
  for instance.
  The right fix in that case was to pass the non-null parameter by
  reference instead of asserting its value, not unlike what you were
  discussing in the previous post (sorry I'm not quoting the right
  post here). For the cases where using references might not be
  possible, how do we deal with the warnings ?

But if we want to do something like this we should probably have some
community discussion about how how to go about any of these things across
the codebase. Maybe using nonnull attributes would be a better approach to
the parameter issue, for example - but it'll still be a lot of code churn
that there should probably be general consensus on rather than approaching
it piecemeal with reviewers in different parts of the codebase.

I agree we should discuss the bigger issue of how to deal with each
type of warning emitted by Clang Static Analyzer. I would prefer to
propose fixes that solve the underlying issues instead of just
suppressing the warnings.

Thanks again for taking the time to suggest a different approach.

Cheers.

6

Hi David,

> Hi Apelete,
>
> Thanks for trying to help cleanup the LLVM codebase of Clang Static
> Analyzer warnings.
>
> But it seems a lot of the fixes that are being proposed are somewhat
> mechanical and may be doing the wrong thing in a few ways.

From the beginning I wondered for each fix if it was the right way of
solving the possible underlying issue pointed to by the Analyzer.
Since I'm not familiar with the codebase, when I was not able to
decide by reading the code I ended-up trusting the tool until advised
otherwise by the reviewers.

Once put together, the fixes do look somewhat mechanical indeed due to
the fact that the same patterns were found throughout the codebase by
the analyzer.
As of this writing, these patterns result in 90+ warnings still
emitted when scanning LLVM+Clang+Compiler-RT (down from 150+ warnings
when I fooled myself into starting to look at it a few weeks ago).

I'm glad you pointed out some of the patterns in the fixes, let's see
how I can address those:

> * Initializing variables that are only used when initialized through some
> existing codepath - this can make tools like Memory Sanitizer less
useful,
> because now the value is initialized even in some path where that value
is
> never intended to be used

* I guess this case could be related to ⚙ D19971 [scan-build] fix warnings emitted on LLVM TargetRecip code base
  and ⚙ D19975 [scan-build] fix warning emitted on LLVM Bitcode code base for instance.
  I wasn't happy with the fixes in the first place but I
  ended-up trusting the tool because it proposed a codepath where the
  variables were used without being initialized. I don't know about
  msan (yet :slight_smile: but if the warnings are false positives is there a way
  to tell scan-build in such cases ?

My first guess with any warning like this would be that there's some
complex invariant (eg: A implies B, so if the variable is conditionally
initialized under A then used under B it's actually fine) that is obvious
enough in the code, but opaque to the static analyzer.

My first guess would be to assert this (assert the condition of A in the
use under B) and only if we find a case where that fails, add a test case
and initialize the variable as appropriate. Adding the variable
initialization without a test case seems suspect.

MSan is a tool for dynamic analysis - it would track the uninitialized
memory and tell us when/where we've used it without initializing it.

> * Adding assertions for pointers that are known to be non-null - in some
> cases this is helpful, when the algorithm that ensures the non-null-ness
is
> sufficiently opaque. But for function parameters - we have /lots/ of
> non-null function parameters. I think it'd be impractical to assert on
all
> of them.

* This is where I ended-up asserting function parameters in a
  mechanical manner to some extent (as a result of 40+ warnings about
  some object pointers being null). Let's take
⚙ D19385 [scan-build] fix warnings emitted on Clang Format code base
  for instance.
  The right fix in that case was to pass the non-null parameter by
  reference instead of asserting its value, not unlike what you were
  discussing in the previous post (sorry I'm not quoting the right
  post here). For the cases where using references might not be
  possible, how do we deal with the warnings ?

I think the tool needs to be fixed, or not used on the LLVM codebase if it
assumes all pointer parameters may be null. But it seems like it doesn't
assume that or I would expect way more errors from the tool, no? I was
pretty sure the Clang Static Analyzer had been designed to only warn about
null dereferences if it saw a pointer null test somewhere in the function
for that pointer.

Do you know what's going on here?

7

> Hi David,
>
>
> > * Initializing variables that are only used when initialized through some
> > existing codepath - this can make tools like Memory Sanitizer less useful,
> > because now the value is initialized even in some path where that valu is
> > never intended to be used
>
> * I guess this case could be related to ⚙ D19971 [scan-build] fix warnings emitted on LLVM TargetRecip code base
> and ⚙ D19975 [scan-build] fix warning emitted on LLVM Bitcode code base for instance.
> I wasn't happy with the fixes in the first place but I
> ended-up trusting the tool because it proposed a codepath where the
> variables were used without being initialized. I don't know about
> msan (yet :slight_smile: but if the warnings are false positives is there a way
> to tell scan-build in such cases ?
>

My first guess with any warning like this would be that there's some
complex invariant (eg: A implies B, so if the variable is conditionally
initialized under A then used under B it's actually fine) that is obvious
enough in the code, but opaque to the static analyzer.

My first guess would be to assert this (assert the condition of A in the
use under B) and only if we find a case where that fails, add a test case
and initialize the variable as appropriate. Adding the variable
initialization without a test case seems suspect.

MSan is a tool for dynamic analysis - it would track the uninitialized
memory and tell us when/where we've used it without initializing it.

Ok, the invariant example is a good insight I wish I kept in mind when
I started. I took interest in Clang Static Analyzer reports because I
wondered how the tool works (and how well). Knowing how to treat the
warnings will definitely help making better decisions (and better
patches).

Any further advice will be greatly appreciated :).

> > * Adding assertions for pointers that are known to be non-null - in some
> > cases this is helpful, when the algorithm that ensures the non-null-ness is
> > sufficiently opaque. But for function parameters - we have /lots/ of
> > non-null function parameters. I think it'd be impractical to assert on all
> > of them.
>
> * This is where I ended-up asserting function parameters in a
> mechanical manner to some extent (as a result of 40+ warnings about
> some object pointers being null). Let's take ⚙ D19385 [scan-build] fix warnings emitted on Clang Format code base
> for instance.
> The right fix in that case was to pass the non-null parameter by
> reference instead of asserting its value, not unlike what you were
> discussing in the previous post (sorry I'm not quoting the right
> post here). For the cases where using references might not be
> possible, how do we deal with the warnings ?
>

I think the tool needs to be fixed, or not used on the LLVM codebase if it
assumes all pointer parameters may be null. But it seems like it doesn't
assume that or I would expect way more errors from the tool, no? I was
pretty sure the Clang Static Analyzer had been designed to only warn about
null dereferences if it saw a pointer null test somewhere in the function
for that pointer.

Do you know what's going on here?

Judging from the 55 warnings I got by running the analyzer on
LLVM+Clang+Compiler-RT codebase when I started, it does *not* seem to
assume all pointer parameters may be null indeed.
However, the tool seems to be able to warn not only about "null
dereferences if it saw a pointer null test", but also about null
dereferences if it saw a may-be-null pointer passed as a parameter
in a function call in the same compilation unit.

This last sentence should read: the tool also seems to be able to follow the
function call flow inside a compilation unit and warn about null
dereferences at any point during that call flow (e.g.
⚙ D19084 [scan-build] fix warnings emitted on Clang AST code base in lib/AST/NestedNameSpecifier.cpp).

Some of these latter warnings may well be false positives I agree, but
what makes it hard to ignore is when the tool actually suggests a call
path to the null dereference. Ignoring it then looks to me like a
case of "the pointer is *expected* to be non-null, so the warning
should be ignored regardless of the hints provided by the tool".

I'd rather add an assert in a defensive stance in that case, but
that's just me; in the end I take the reviewer's advice into account :).

Cheers.

8

>
> > Hi David,
> >
> >
> > > * Initializing variables that are only used when initialized through
some
> > > existing codepath - this can make tools like Memory Sanitizer less
useful,
> > > because now the value is initialized even in some path where that
valu is
> > > never intended to be used
> >
> > * I guess this case could be related to ⚙ D19971 [scan-build] fix warnings emitted on LLVM TargetRecip code base
> > and ⚙ D19975 [scan-build] fix warning emitted on LLVM Bitcode code base for instance.
> > I wasn't happy with the fixes in the first place but I
> > ended-up trusting the tool because it proposed a codepath where the
> > variables were used without being initialized. I don't know about
> > msan (yet :slight_smile: but if the warnings are false positives is there a way
> > to tell scan-build in such cases ?
> >
>
> My first guess with any warning like this would be that there's some
> complex invariant (eg: A implies B, so if the variable is conditionally
> initialized under A then used under B it's actually fine) that is obvious
> enough in the code, but opaque to the static analyzer.
>
> My first guess would be to assert this (assert the condition of A in the
> use under B) and only if we find a case where that fails, add a test case
> and initialize the variable as appropriate. Adding the variable
> initialization without a test case seems suspect.
>
> MSan is a tool for dynamic analysis - it would track the uninitialized
> memory and tell us when/where we've used it without initializing it.

Ok, the invariant example is a good insight I wish I kept in mind when
I started. I took interest in Clang Static Analyzer reports because I
wondered how the tool works (and how well). Knowing how to treat the
warnings will definitely help making better decisions (and better
patches).

Any further advice will be greatly appreciated :).

> > > * Adding assertions for pointers that are known to be non-null - in
some
> > > cases this is helpful, when the algorithm that ensures the
non-null-ness is
> > > sufficiently opaque. But for function parameters - we have /lots/ of
> > > non-null function parameters. I think it'd be impractical to assert
on all
> > > of them.
> >
> > * This is where I ended-up asserting function parameters in a
> > mechanical manner to some extent (as a result of 40+ warnings about
> > some object pointers being null). Let's take
⚙ D19385 [scan-build] fix warnings emitted on Clang Format code base
> > for instance.
> > The right fix in that case was to pass the non-null parameter by
> > reference instead of asserting its value, not unlike what you were
> > discussing in the previous post (sorry I'm not quoting the right
> > post here). For the cases where using references might not be
> > possible, how do we deal with the warnings ?
> >
>
> I think the tool needs to be fixed, or not used on the LLVM codebase if
it
> assumes all pointer parameters may be null. But it seems like it doesn't
> assume that or I would expect way more errors from the tool, no? I was
> pretty sure the Clang Static Analyzer had been designed to only warn
about
> null dereferences if it saw a pointer null test somewhere in the function
> for that pointer.
>
> Do you know what's going on here?

Judging from the 55 warnings I got by running the analyzer on
LLVM+Clang+Compiler-RT codebase when I started, it does *not* seem to
assume all pointer parameters may be null indeed.
However, the tool seems to be able to warn not only about "null
dereferences if it saw a pointer null test", but also about null
dereferences if it saw a may-be-null pointer passed as a parameter
in a function call in the same compilation unit.

This last sentence should read: the tool also seems to be able to follow
the
function call flow inside a compilation unit and warn about null
dereferences at any point during that call flow (e.g.
⚙ D19084 [scan-build] fix warnings emitted on Clang AST code base in lib/AST/NestedNameSpecifier.cpp).

Yes, the static analyzer is interprocedural within a single translation
unit, so this might be the issue (it sees a null test of a pointer inside
one function A, then assumes that the parameter to function B (which calls
A and passes along the pointer) must also be possibly null). It would be
good to test that, then fix the analyzer to not make this assumption - it
seems incorrect. Just because a pointer may be null in one function,
doesn't imply much of anything about it in callers to that function - those
callers may have guaranteed non-null pointers still.

Some of these latter warnings may well be false positives I agree, but
what makes it hard to ignore is when the tool actually suggests a call
path to the null dereference. Ignoring it then looks to me like a
case of "the pointer is *expected* to be non-null, so the warning
should be ignored regardless of the hints provided by the tool".

I'd rather add an assert in a defensive stance in that case, but
that's just me; in the end I take the reviewer's advice into account :).

My argument against this is that it seems we'd end up with a very haphazard
set of assertions - the significant majority of cases where we have
non-null pointers aren't being diagnosed by the tool, so some
semi-arbitrary subset of those cases would end up with asserts and the rest
not. I'm not sure how constructive that would be, which is why I'm inclined
to push back a bit on that approach due to the lack of consistency in the
end result we'd get.

- Dave

9

As a sidenote, the issue of a function’s/API’s intended specs is an issue Microsoft dealt with through the use of SAL, as both API callers and API implementers get static-analyzer checking of their code vs their intentions… but it also explicit what the API specifies & how it is intended to be used.