Closed Bug 997145 Opened 6 years ago Closed 5 years ago

Add a attribute in order to silent a Clang static analyzer check

Categories

(Firefox Build System :: Source Code Analysis, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla32

People

(Reporter: Sylvestre, Assigned: Sylvestre)

References

Details

Attachments

(1 file, 5 obsolete files)

Attached patch attribute-on-assert.diff (obsolete) — Splinter Review
In the attached patch, we add an attribute to the function declaration.
This attribute will silent more than 7000 false positives regarding "Dereference of null pointer"  found by the clang static analyzer.
Comment on attachment 8407502 [details] [diff] [review]
attribute-on-assert.diff

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

::: mfbt/Assertions.h
@@ +123,5 @@
>   * method is primarily for internal use in this header, and only secondarily
>   * for use in implementing release-build assertions.
>   */
>  static MOZ_ALWAYS_INLINE void
> +MOZ_ReportAssertionFailure(const char* s, const char* file, int ln) MOZ_ANALYZER_NORETURN

This can't be right.  This function really does return.

@@ +135,5 @@
>  #endif
>  }
>  
>  static MOZ_ALWAYS_INLINE void
> +MOZ_ReportCrash(const char* s, const char* file, int ln) MOZ_ANALYZER_NORETURN

Likewise.

::: mfbt/Attributes.h
@@ +163,5 @@
> + *
> + *   void abort(const char* msg) MOZ_ANALYZER_NORETURN;
> + *
> + * This modifier permits some static analyzer (like scan-build from clang) to
> + * skip some false positive.

Why are you making this a separate thing rather than cleverly rolling it into MOZ_NORETURN?

@@ +170,5 @@
> + */
> +#if defined(MOZ_HAVE_ANALYZER_NORETURN)
> +#  define MOZ_ANALYZER_NORETURN          MOZ_HAVE_ANALYZER_NORETURN
> +#else
> +//#error "MOZ_ANALYZER_NORETURN = KOOOOOOOOOOOOOOO"

This should be removed.

::: xpcom/build/nsXPCOM.h
@@ +243,5 @@
>   */
>  XPCOM_API(void)
>  NS_DebugBreak(uint32_t aSeverity,
>                const char *aStr, const char *aExpr,
> +              const char *aFile, int32_t aLine) MOZ_ANALYZER_NORETURN;

This function can certainly return sometimes...

::: xpcom/glue/standalone/nsXPCOMGlue.cpp
@@ +798,5 @@
>  }
>  
>  XPCOM_API(void)
>  NS_DebugBreak(uint32_t aSeverity, const char *aStr, const char *aExpr,
> +              const char *aFile, int32_t aLine) MOZ_ANALYZER_NORETURN

You shouldn't need this on function declarations.
(In reply to Nathan Froyd (:froydnj) from comment #1)
> >  static MOZ_ALWAYS_INLINE void
> > +MOZ_ReportAssertionFailure(const char* s, const char* file, int ln) MOZ_ANALYZER_NORETURN
> 
> This can't be right.  This function really does return.
Actually, maybe I should rename the define. The attribute is called "analyzer_noreturn"
See 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."
What do you think?


> ::: mfbt/Attributes.h
> @@ +163,5 @@
> > + *
> > + *   void abort(const char* msg) MOZ_ANALYZER_NORETURN;
> > + *
> > + * This modifier permits some static analyzer (like scan-build from clang) to
> > + * skip some false positive.
> 
> Why are you making this a separate thing rather than cleverly rolling it
> into MOZ_NORETURN?
Because I was trying to make changes with minimal impact on the base code itself. If I can use MOZ_NORETURN, no worries!


> @@ +170,5 @@
> > + */
> > +#if defined(MOZ_HAVE_ANALYZER_NORETURN)
> > +#  define MOZ_ANALYZER_NORETURN          MOZ_HAVE_ANALYZER_NORETURN
> > +#else
> > +//#error "MOZ_ANALYZER_NORETURN = KOOOOOOOOOOOOOOO"
> 
> This should be removed.
Yeh, I fixed that right after I upload the patch. Shame on me :)
(In reply to Sylvestre Ledru [:sylvestre] from comment #2)
> (In reply to Nathan Froyd (:froydnj) from comment #1)
> > >  static MOZ_ALWAYS_INLINE void
> > > +MOZ_ReportAssertionFailure(const char* s, const char* file, int ln) MOZ_ANALYZER_NORETURN
> > 
> > This can't be right.  This function really does return.
> Actually, maybe I should rename the define. The attribute is called
> "analyzer_noreturn"
> See 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."
> What do you think?

Ah, OK.  Then it's probably OK for the MFBT report functions.  NS_DebugBreak being annotated as such is probably incorrect, as we'd think that:

  NS_WARNING(...);

does not return, which is sure to cause problems.

I think the comment for whatever the macro gets named should make it clear that it's potentially OK to add it to functions that don't return.  The "enables analyzer to skip false positives" sort of implies that, but it's not very obvious.

Renaming the macro to MOZ_PRETEND_NORETURN_FOR_STATIC_ANALYSIS or somesuch might help unwary code readers, too.
Attached patch attribute-on-assert-.diff (obsolete) — Splinter Review
Ok. Thanks. So, I removed the annotation on NS_DebugBreak. I will try to find a better solution later.
I also improved the description of the define and renamed it to your suggestion.
Thanks.
Attachment #8407502 - Attachment is obsolete: true
Attachment #8407534 - Flags: review?(nfroyd)
Comment on attachment 8407534 [details] [diff] [review]
attribute-on-assert-.diff

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

So I think this is mostly-OK, but I'm also wondering why we couldn't just make MOZ_Report{Assertion,Crash} out-of-line functions, move the MOZ_REALLY_CRASH bits into those functions, and mark them as MOZ_NORETURN instead.  Presumably that would even enable the compiler to generate better code, though possibly at the expense of debugging pleasantness.

::: mfbt/Attributes.h
@@ +167,5 @@
> + *
> + *   void abort(const char* msg) MOZ_PRETEND_NORETURN_FOR_STATIC_ANALYSIS;
> + *
> + * This modifier permits some static analyzer (like scan-build from clang) to
> + * skip some false positive.

This entire comment isn't quite right.  "...indicates that the given function might not return" isn't consistent with the upstream documentation you cite.  How about something like this:

MOZ_PRETEND_NORETURN_FOR_STATIC_ANALYSIS, specified at the end of a function declaration, indicates that for the purposes of static analysis, this function does not return.  (The function definition does not need to be annotated.)

  ...example function declaration that's not abort()...

Some static analyzers, like scan-build from clang, can use this information to eliminate false positives.  From the upstream documentation of scan-build: "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."
(In reply to Nathan Froyd (:froydnj) from comment #5)
> Review of attachment 8407534 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So I think this is mostly-OK, but I'm also wondering why we couldn't just
> make MOZ_Report{Assertion,Crash} out-of-line functions, move the
> MOZ_REALLY_CRASH bits into those functions, and mark them as MOZ_NORETURN
> instead.  Presumably that would even enable the compiler to generate better
> code, though possibly at the expense of debugging pleasantness.

Waldo, do you know why we didn't just take this route in the first place?  Were we trying to think of the poor people debugging their code?
Flags: needinfo?(jwalden+bmo)
Attached patch attribute on assert functions (obsolete) — Splinter Review
With the comment updated (thanks)
Attachment #8407534 - Attachment is obsolete: true
Attachment #8407534 - Flags: review?(nfroyd)
Attachment #8407534 - Flags: review?(nfroyd)
(In reply to Nathan Froyd (:froydnj) from comment #6)
> Waldo, do you know why we didn't just take this route in the first place? 
> Were we trying to think of the poor people debugging their code?

We were apparently trying to support the people who don't want "external dependencies" on Gecko code, despite writing code that goes in Gecko: bug 719776.

Sigh.  I guess this patch is OK, then.
Flags: needinfo?(jwalden+bmo)
Attachment #8408373 - Flags: review+
(In reply to Nathan Froyd (:froydnj) from comment #6)
> > So I think this is mostly-OK, but I'm also wondering why we couldn't just
> > make MOZ_Report{Assertion,Crash} out-of-line functions, move the
> > MOZ_REALLY_CRASH bits into those functions, and mark them as MOZ_NORETURN
> > instead.  Presumably that would even enable the compiler to generate better
> > code, though possibly at the expense of debugging pleasantness.
> 
> Waldo, do you know why we didn't just take this route in the first place? 
> Were we trying to think of the poor people debugging their code?

We didn't move crashing into an OOL function because then crashing in a debugger breaks at the wrong location, and you have to up 1 or whatever to get to the actual location of the crash.  That gets *really* old really fast.

I really thought the really-crash bits had noreturn semantics, and those semantics were made known to the compiler, at one point.  Why isn't that the case?
...wait, clang's static analyzer complains about dereferencing null in here?  Why can't we just annotate that warning away on that particular location?  Surely that's possible somehow.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10)
> ...wait, clang's static analyzer complains about dereferencing null in here?
> Why can't we just annotate that warning away on that particular location? 
> Surely that's possible somehow.
It is what I am doing with this attribute:
http://clang-analyzer.llvm.org/annotations.html#attr_analyzer_noreturn
I meant annotating the specific, particular dereference-of-null statement.  It only *happens* the warning-print function call happens just before then, such that it can be marked as fake-noreturn for equivalent effect.
Comment on attachment 8407534 [details] [diff] [review]
attribute-on-assert-.diff

Somehow this review didn't get canceled...or something.
Attachment #8407534 - Flags: review?(nfroyd)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #12)
> I meant annotating the specific, particular dereference-of-null statement. 
> It only *happens* the warning-print function call happens just before then,
> such that it can be marked as fake-noreturn for equivalent effect.

Well, there's nothing like that, AFAICS from the analyzer documentation.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #9)
> > Waldo, do you know why we didn't just take this route in the first place? 
> > Were we trying to think of the poor people debugging their code?
> 
> We didn't move crashing into an OOL function because then crashing in a
> debugger breaks at the wrong location, and you have to up 1 or whatever to
> get to the actual location of the crash.  That gets *really* old really fast.

I can see the argument here, but I don't think that's a hard blocker.  Just add some gdbinit.in bits or similar to take care of that.

> I really thought the really-crash bits had noreturn semantics, and those
> semantics were made known to the compiler, at one point.  Why isn't that the
> case?

Well, at least the patch in bug 719776 doesn't have any indication of MOZ_NORETURN.

Having those things would be better, but if we want to support things like bug 719776, we can't do that.  One option would be to define MOZ_ASSERT &co. differently for analyzer builds with SCARY WARNINGS about changing code for non-analyzer builds and keeping the two versions synchronized.  Then analyzer builds could use proper noreturn semantics, and non-analyzer builds could use the inlined no-external-dependency versions...
https://hg.mozilla.org/mozilla-central/rev/c34781199047
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Backed out in https://hg.mozilla.org/mozilla-central/rev/a0e703ba2ae6 - at least so far, b2g OS X desktop and Firefox OS X agree that won't build, https://tbpl.mozilla.org/php/getParsedLog.php?id=38374067&tree=Mozilla-Central and https://tbpl.mozilla.org/php/getParsedLog.php?id=38374306&tree=Mozilla-Central
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Bug-997145.diff (obsolete) — Splinter Review
+ silent the gcc-compat warning (it is done elsewhere in the code).

TBPL build:
https://tbpl.mozilla.org/?tree=Try&rev=0cbe1dc08e44
Assignee: nobody → sledru
Attachment #8408373 - Attachment is obsolete: true
Blocks: 711241
Attached patch scan-build.diff (obsolete) — Splinter Review
After some discussions with Nathan, we think that the proposed solution is better. The attribute is only defined when the code is built with clang analyzer (which comes with its own define)
Attachment #8411591 - Attachment is obsolete: true
Attachment #8417886 - Flags: review?(nfroyd)
Attached patch scan-build.diffSplinter Review
Sorry, made a mistake with the patch. Here is the right one.
Attachment #8417886 - Attachment is obsolete: true
Attachment #8417886 - Flags: review?(nfroyd)
Attachment #8417926 - Flags: review?(nfroyd)
Comment on attachment 8417926 [details] [diff] [review]
scan-build.diff

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

::: mfbt/Attributes.h
@@ +108,5 @@
> + * to mark some false positives
> + */
> +#ifdef __clang_analyzer__
> +#  if __has_extension(attribute_analyzer_noreturn)
> +#  define MOZ_HAVE_ANALYZER_NORETURN __attribute__((analyzer_noreturn))

Nit: indent this |define| two extra spaces.
Attachment #8417926 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/mozilla-central/rev/31bf4de12ff1
Status: REOPENED → RESOLVED
Closed: 6 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.