419622 - Hook up gcc-dehydra to our build system

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Benjamin Smedberg]

Attachment: gcc-dehydra turned on --with-statick-checking=

The patch here will hook up gcc-dehydra to our build and set up a couple of simple class annotations: NS_STACK_CLASS and NS_FINAL_CLASS.

There's one dehydra assertion I'm currently hitting in nsDOMParser.cpp, but I don't think this landing needs to depend on that being fixed.

Setting r?luser for the build stuff, r?dbaron for nscore.h and nsCOMPtr.h, and r?tglek for static-checker.js

Comment 1

[(not currently active) Ted Mielczarek]

Comment on attachment 305753 [details] [diff] [review]
gcc-dehydra turned on --with-statick-checking=

>diff --git a/configure.in b/configure.in
>--- a/configure.in
>+++ b/configure.in
>@@ -6161,6 +6161,21 @@ fi
> fi
> 
> dnl ========================================================
>+dnl = Enable static checking using gcc-dehydra
>+dnl ========================================================
>+
>+MOZ_ARG_WITH_STRING(static-checking,
>+[  --with-static-checking=path/to/gcc_dehydra.so
>+                            Enable static checking of code using GCC-dehydra],
>+    DEHYDRA_PATH=$withval,
>+    DEHYDRA_PATH= )
>+
>+if test -n "$DEHYDRA_PATH"; then
>+    AC_DEFINE(NS_STATIC_CHECKING)
>+fi
>+AC_SUBST(DEHYDRA_PATH)

You should probably test -f $DEHYDRA_PATH here while you're at it, and error if it doesn't exist.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 305753 [details] [diff] [review]
gcc-dehydra turned on --with-statick-checking=

>diff --git a/xpcom/base/nscore.h b/xpcom/base/nscore.h
>+#define NS_STACK_CLASS __attribute__((user("NS_stack")))
>+#define NS_FINAL_CLASS __attribute__((user("NS_final")))

So, |info gcc|, section 5.25 ("Attribute Syntax") seems to suggest (although not very precisely) that only one __attribute__ is allowed in the various places where __attribute__ is allowed, and a comma-separated list can be placed inside the __attribute__(()).  Is it true that gcc really coalesces multiple __attribute__ declarations properly (as though they were specified as a single comma-separated list?

(If it does work and is supported, can you get the gcc folks to make their docs clearer?)

>diff --git a/xpcom/glue/nsCOMPtr.h b/xpcom/glue/nsCOMPtr.h

>+class
>+  NS_FINAL_CLASS
>+  NS_STACK_CLASS
>+nsDerivedSafe : public T

I'm not a fan of this indentation style.  Maybe put NS_FINAL_CLASS and NS_STACK_CLASS on the same line as "class"?  Although I suppose it's ok as-is if you really want it that way.

r=dbaron on the nsCOMPtr.h and nscore.h changes.

Comment 3

[Benjamin Smedberg]

It does work... not sure about "supported" but I'll ask to the gcc list about getting the docs clarified.

Talked to shaver about the attribute macros and he prefers the indented style with me... it will make adds/removes easier to read in diffs, among other things.

Comment 4

[Benjamin Smedberg]

Woohoo! pushed to mozilla-central for mozilla2.

Comment 5

[Benjamin Smedberg]

I think the gcc docs were cleaned up to explicitly allow multiple __attribute__ specifiers:

"An attribute specifier list is a sequence of one or more attribute specifiers, not separated by any other tokens."