Silent warning on NS_DebugBreak for Clang static analyzer check

RESOLVED DUPLICATE of bug 1156028

Status

()

Core
Rewriting and Analysis
RESOLVED DUPLICATE of bug 1156028
3 years ago
3 years ago

People

(Reporter: sylvestre, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
Created attachment 8464024 [details] [diff] [review]
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/
Attachment #8464024 - Flags: review?(nfroyd)
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.
(Reporter)

Comment 2

3 years ago
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).
(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.
What sort of false positives get eliminated with this attribute added?
Flags: needinfo?(sledru)
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.
Attachment #8464024 - Flags: review?(nfroyd)
(Reporter)

Comment 6

3 years ago
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.
Flags: needinfo?(sledru)
(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

3 years ago
I have a better fix for this in bug 1156028.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1156028
You need to log in before you can comment on or make changes to this bug.