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

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

Attachment: attribute-on-assert.diff

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 1

[Nathan Froyd [:froydnj]]

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.

Comment 2

[Sylvestre Ledru [:Sylvestre]]

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

Comment 3

[Nathan Froyd [:froydnj]]

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

Comment 4

[Sylvestre Ledru [:Sylvestre]]

Attachment: attribute-on-assert-.diff

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.

Comment 5

[Nathan Froyd [:froydnj]]

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

Comment 6

[Nathan Froyd [:froydnj]]

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

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Attachment: attribute on assert functions

With the comment updated (thanks)

Comment 8

[Nathan Froyd [:froydnj]]

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

Comment 9

[Jeff Walden [:Waldo]]

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

Comment 10

[Jeff Walden [:Waldo]]

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

Comment 11

[Sylvestre Ledru [:Sylvestre]]

(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

Comment 12

[Jeff Walden [:Waldo]]

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 13

[Nathan Froyd [:froydnj]]

Comment on attachment 8407534 [details] [diff] [review]
attribute-on-assert-.diff

Somehow this review didn't get canceled...or something.

Comment 14

[Nathan Froyd [:froydnj]]

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

Comment 15

[Sylvestre Ledru [:Sylvestre]]

https://hg.mozilla.org/mozilla-central/rev/c34781199047

Comment 16

[Phil Ringnalda (:philor)]

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

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug-997145.diff

+ silent the gcc-compat warning (it is done elsewhere in the code).

TBPL build:
https://tbpl.mozilla.org/?tree=Try&rev=0cbe1dc08e44

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Attachment: scan-build.diff

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)

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Attachment: scan-build.diff

Sorry, made a mistake with the patch. Here is the right one.

Comment 20

[Nathan Froyd [:froydnj]]

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.

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Thanks Nathan.

https://hg.mozilla.org/integration/mozilla-inbound/rev/31bf4de12ff1

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/31bf4de12ff1