1045645 - Silent warning on NS_DebugBreak for Clang static analyzer check

Status: RESOLVED DUPLICATE of bug 1156028

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

Attachment: silent-more.diff

Like in bug 997145, I need to add this flag "MOZ_PRETEND_NORETURN_FOR_STATIC_ANALYSIS" on NS_DebugBreak

It silents about a thousand false positives on:
https://people.mozilla.org/~sledru/reports/scan-build/

Comment 1

[Nathan Froyd [:froydnj]]

Comment on attachment 8464024 [details] [diff] [review]
silent-more.diff

Review of attachment 8464024 [details] [diff] [review]:
-----------------------------------------------------------------

How does this work with the (many) cases where NS_DebugBreak actually returns?  Does the analyzer just ignore paths after that?  I suppose often times we're going to return from the function anyway, but this seems like an excellent way to exclude swathes of code from the analyzer.

Putting it on the functions in Assertions.h was OK because we always crash after those functions; we don't always do that here.

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Quoting http://clang-analyzer.llvm.org/annotations.html#attr_analyzer_noreturn
"This attribute is useful for annotating assertion handlers that actually can return, but for the purpose of using the analyzer we want to pretend that such functions do not return."

So, it should not change the behavior of Firefox (even when built with scan-build).

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #2)
> Quoting
> http://clang-analyzer.llvm.org/annotations.html#attr_analyzer_noreturn
> "This attribute is useful for annotating assertion handlers that actually
> can return, but for the purpose of using the analyzer we want to pretend
> that such functions do not return."
> 
> So, it should not change the behavior of Firefox (even when built with
> scan-build).

Of course.  My concern is that if you have:

  NS_ASSERTION(expr, "message");
  /* 30 more lines of code */

which expands to:

  if (!expr) {
    NS_DebugBreak(...);
  }
  /* 30 more lines of code */

If we add the attribute, then I think the analyzer will assume that |expr| must be true/non-null in the remainder of the code.  And that's definitely not the case.

Comment 4

[Nathan Froyd [:froydnj]]

What sort of false positives get eliminated with this attribute added?

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8464024 [details] [diff] [review]
silent-more.diff

Canceling review while we work out whether the reviewer's concerns are valid or not.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

For firefox, I have applied this patch to evaluate its impact.

I haven't done that for Thunderbird reports yet. So, you can see that for this file:
https://people.mozilla.org/~sledru/reports/tb-scan-build/report-inFlasher.cpp-GetInvert-2-1.html#EndPath

It will silent this warning for example. NS_PRECONDITION calls NS_DebugBreak.

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #6)
> For firefox, I have applied this patch to evaluate its impact.
> 
> I haven't done that for Thunderbird reports yet. So, you can see that for
> this file:
> https://people.mozilla.org/~sledru/reports/tb-scan-build/report-inFlasher.
> cpp-GetInvert-2-1.html#EndPath
> 
> It will silent this warning for example. NS_PRECONDITION calls NS_DebugBreak.

And NS_DebugBreak for NS_PRECONDITION doesn't always abort:

http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsDebugImpl.cpp#434
http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsDebugImpl.cpp#242

Comment 8

[(no longer active)]

I have a better fix for this in bug 1156028.