Hook up gcc-dehydra to our build system

RESOLVED FIXED in mozilla2.0

Status

Firefox Build System
Source Code Analysis
RESOLVED FIXED
10 years ago
3 months ago

People

(Reporter: Benjamin Smedberg, Assigned: Benjamin Smedberg)

Tracking

unspecified
mozilla2.0
x86
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

10 years ago
Created attachment 305753 [details] [diff] [review]
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
Attachment #305753 - Flags: review?(tglek)
Attachment #305753 - Flags: review?(ted.mielczarek)
Attachment #305753 - Flags: review?(dbaron)
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.
Attachment #305753 - Flags: review?(ted.mielczarek) → review+
(Assignee)

Updated

10 years ago
Depends on: 419624
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.
Attachment #305753 - Flags: review?(dbaron) → review+

Updated

10 years ago
Attachment #305753 - Flags: review?(tglek) → review+
(Assignee)

Comment 3

10 years ago
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.
(Assignee)

Comment 4

10 years ago
Woohoo! pushed to mozilla-central for mozilla2.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0
(Assignee)

Comment 5

10 years ago
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."

Updated

3 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.