Closed Bug 868285 Opened 7 years ago Closed 7 years ago

Clang static analysis builds currently perma-fail due to build error in nsCycleCollector.cpp

Categories

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

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla24

People

(Reporter: RyanVM, Assigned: jcranmer)

Details

Attachments

(4 files, 1 obsolete file)

Hiding the jobs on tbpl until this is fixed.

https://tbpl.mozilla.org/php/getParsedLog.php?id=22531343&tree=Mozilla-Central

Linux x86-64 mozilla-central debug static analysis build on 2013-05-02 18:35:21 PDT for push cdd14d9b3aae
slave: bld-linux64-ec2-708

/builds/slave/m-cen-l64-dbg-st-an-0000000000/build/clang/bin/clang++ -o AvailableMemoryTracker.o -c -I../../dist/stl_wrappers -I../../dist/system_wrappers -include /builds/slave/m-cen-l64-dbg-st-an-0000000000/build/config/gcc_hidden.h -DXP_LINUX -DMOZ_GLUE_IN_PROGRAM -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DSTATIC_EXPORTABLE_JS_API -DNO_NSPR_10_SUPPORT -DEXCLUDE_SKIA_DEPENDENCIES  -DOS_POSIX=1 -DOS_LINUX=1  -D_IMPL_NS_COM -I/builds/slave/m-cen-l64-dbg-st-an-0000000000/build/ipc/chromium/src -I/builds/slave/m-cen-l64-dbg-st-an-0000000000/build/ipc/glue -I../../ipc/ipdl/_ipdlheaders  -I/builds/slave/m-cen-l64-dbg-st-an-0000000000/build/xpcom/base/../build -I/builds/slave/m-cen-l64-dbg-st-an-0000000000/build/xpcom/ds  -I/builds/slave/m-cen-l64-dbg-st-an-0000000000/build/xpcom/base -I. -I../../dist/include  -I/builds/slave/m-cen-l64-dbg-st-an-0000000000/build/obj-firefox/dist/include/nspr -I/builds/slave/m-cen-l64-dbg-st-an-0000000000/build/obj-firefox/dist/include/nss      -fPIC -Qunused-arguments  -Qunused-arguments -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wsign-compare -Wno-invalid-offsetof -Wno-c++0x-extensions -Wno-extended-offsetof -Wno-unknown-warning-option -Wno-return-type-c-linkage -Wno-mismatched-tags -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -std=gnu++0x -I/builds/slave/m-cen-l64-dbg-st-an-0000000000/build/build/unix/headers -pthread -pipe  -DDEBUG -D_DEBUG -DTRACING -g -Xclang -load -Xclang ../../build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -Os -freorder-blocks  -fno-omit-frame-pointer  -I/builds/slave/m-cen-l64-dbg-st-an-0000000000/build/widget/gtk2/compat -I/usr/include/gtk-2.0 -I/usr/lib64/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng12 -I/usr/include/gtk-unix-print-2.0    -Qunused-arguments  -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MP -MF .deps/AvailableMemoryTracker.o.pp  /builds/slave/m-cen-l64-dbg-st-an-0000000000/build/xpcom/base/AvailableMemoryTracker.cpp
MapsMemoryReporter.cpp
In file included from /builds/slave/m-cen-l64-dbg-st-an-0000000000/build/xpcom/base/nsCycleCollector.cpp:126:
In file included from ../../dist/include/nsIXPConnect.h:18:
In file included from ../../dist/include/xpccomponents.h:14:
In file included from ../../dist/include/xpcexception.h:21:
../../dist/include/jsapi.h:5018:39: error: variable of type 'const HandleId' (aka 'const Handle<jsid>') only valid on the stack
extern JS_PUBLIC_DATA(const HandleId) JSID_VOIDHANDLE;
                                      ^
../../dist/include/jsapi.h:5019:39: error: variable of type 'const HandleId' (aka 'const Handle<jsid>') only valid on the stack
extern JS_PUBLIC_DATA(const HandleId) JSID_EMPTYHANDLE;
                                      ^
/builds/slave/m-cen-l64-dbg-st-an-0000000000/build/clang/bin/clang++ -o MapsMemoryReporter.o -c -I../../dist/stl_wrappers -I../../dist/system_wrappers -include /builds/slave/m-cen-l64-dbg-st-an-0000000000/build/config/gcc_hidden.h -DXP_LINUX -DMOZ_GLUE_IN_PROGRAM -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DSTATIC_EXPORTABLE_JS_API -DNO_NSPR_10_SUPPORT -DEXCLUDE_SKIA_DEPENDENCIES  -DOS_POSIX=1 -DOS_LINUX=1  -D_IMPL_NS_COM -I/builds/slave/m-cen-l64-dbg-st-an-0000000000/build/ipc/chromium/src -I/builds/slave/m-cen-l64-dbg-st-an-0000000000/build/ipc/glue -I../../ipc/ipdl/_ipdlheaders  -I/builds/slave/m-cen-l64-dbg-st-an-0000000000/build/xpcom/base/../build -I/builds/slave/m-cen-l64-dbg-st-an-0000000000/build/xpcom/ds  -I/builds/slave/m-cen-l64-dbg-st-an-0000000000/build/xpcom/base -I. -I../../dist/include  -I/builds/slave/m-cen-l64-dbg-st-an-0000000000/build/obj-firefox/dist/include/nspr -I/builds/slave/m-cen-l64-dbg-st-an-0000000000/build/obj-firefox/dist/include/nss      -fPIC -Qunused-arguments  -Qunused-arguments -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wsign-compare -Wno-invalid-offsetof -Wno-c++0x-extensions -Wno-extended-offsetof -Wno-unknown-warning-option -Wno-return-type-c-linkage -Wno-mismatched-tags -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -std=gnu++0x -I/builds/slave/m-cen-l64-dbg-st-an-0000000000/build/build/unix/headers -pthread -pipe  -DDEBUG -D_DEBUG -DTRACING -g -Xclang -load -Xclang ../../build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -Os -freorder-blocks  -fno-omit-frame-pointer  -I/builds/slave/m-cen-l64-dbg-st-an-0000000000/build/widget/gtk2/compat -I/usr/include/gtk-2.0 -I/usr/lib64/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/freetype2 -I/usr/include/libpng12 -I/usr/include/gtk-unix-print-2.0    -Qunused-arguments  -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MP -MF .deps/MapsMemoryReporter.o.pp  /builds/slave/m-cen-l64-dbg-st-an-0000000000/build/xpcom/base/MapsMemoryReporter.cpp
mkdir -p "../../dist/bin/components/"
nsIConsoleListener.idl
/builds/slave/m-cen-l64-dbg-st-an-0000000000/build/obj-firefox/_virtualenv/bin/python /builds/slave/m-cen-l64-dbg-st-an-0000000000/build/config/pythonpath.py \
	  -I/builds/slave/m-cen-l64-dbg-st-an-0000000000/build/other-licenses/ply \
	  -I/builds/slave/m-cen-l64-dbg-st-an-0000000000/build/xpcom/typelib/xpt/tools \
	  /builds/slave/m-cen-l64-dbg-st-an-0000000000/build/obj-firefox/dist/sdk/bin/typelib.py -I/builds/slave/m-cen-l64-dbg-st-an-0000000000/build/xpcom/base -I../../dist/idl /builds/slave/m-cen-l64-dbg-st-an-0000000000/build/xpcom/base/nsIConsoleListener.idl -d .deps/nsIConsoleListener.xpt.pp -o _xpidlgen/nsIConsoleListener.xpt
nsIConsoleMessage.idl
2 warnings and 2 errors generated.
make[6]: *** [nsCycleCollector.o] Error 1
make[6]: *** Waiting for unfinished jobs....
On brief discussion with :sfink, the best way forward with this to allow an escape hatch for MOZ_STACK_CLASS and mark the affected variables as permissible off-stack.
MOZ_IGNORE_STACK lets the static checking get ignored. This also adds it to the variable in question.
Assignee: nobody → Pidgeot18
Status: NEW → ASSIGNED
Attachment #746400 - Flags: review?(ehsan)
This fixes bug 859523.
Attachment #746401 - Flags: review?(benjamin)
Attached patch Part 3: OopsSplinter Review
How did I forget to change this... and how did this build in the first place?
Attachment #746402 - Flags: review?(mh+mozilla)
Attachment #746402 - Flags: review?(mh+mozilla) → review+
I think this patch creates a footgun which I'm not really comfortable with.  Ignoring the case of this code in js for a moment, conceptually it makes no sense for something to be "stack only" except when people mark it as not!  That sort of defeats the purpose of MOZ_STACK_CLASS.

I think for the purpose of the code in question, the js engine needs to remove this hackery and use a different C++ types for this special handles.
Flags: needinfo?(sphink)
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #6)
> I think this patch creates a footgun which I'm not really comfortable with. 
> Ignoring the case of this code in js for a moment, conceptually it makes no
> sense for something to be "stack only" except when people mark it as not! 
> That sort of defeats the purpose of MOZ_STACK_CLASS.

I disagree. The point of static analysis in my mind is to allow people to make assertions about code that you can't or don't want to make via the type system (which is itself a built-in static analysis.) Some things are simply impossible to use the built-in type system for; others add more additional complexity than you gain from the assertion. Allowing more granular assertions via a static analysis does not defeat its purpose -- to the contrary, it allows it to be applied to more situations that you might otherwise reject it for entirely because of one or two minor exceptions.

If you accept that, then it comes down to the specifics of the case in question.

> I think for the purpose of the code in question, the js engine needs to
> remove this hackery and use a different C++ types for this special handles.

For GC rooting, we use the type system to enforce that all pointers held live across a GC are rooted (discovered by a GC, and updated when a moving GC requires it). Specifically, lots of functions now require Handle<> parameters, which are essentially const references to rooted pointers. Handle<>s should be stack-allocated when they refer to a rooted location.

In a number of situations, we use a sentinel value (eg JSVAL_NULL, JSVAL_VOID, JSID_EMPTY, JSID_VOID). They're used for the same sorts of things as NULL, -1, 0, etc. We would like it to be easy to use these without constructing a dummy Handle on the stack. So it's handy to be able to make extern Handles and statically allocate them.

The alternative, to create a non-stack superclass or subclass of Handle<>, just seems like busywork to keep the analysis happy. I'm also not convinced it'll be straightforward to get working, because we rely on both implicit conversions and template instantiation for this Handle<> stuff (eg to autoconvert Handle<T> to T), and inheritance tends to make that harder.
Flags: needinfo?(sphink)
Comment on attachment 746400 [details] [diff] [review]
Part 1: Poke a hole in the static analysis

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

I still think I'm right in arguing against this, but this can quickly turn into a conversation about semantics, which I'm not interested in.

In the interest of making progress, I suggest the following:

* Add a new MOZ_IGNORABLE_STACK_CLASS attribute for classes that are stack classes, but not really.
* Reimplement this to only apply to ignorable stack classes.  The semantics of MOZ_IGNORABLE_STACK_CLASS should otherwise be similar to MOZ_STACK_CLASS.

Sorry to ask you to redo this, but I believe MOZ_IGNORABLE_STACK_CLASS can at least document that the author has considered its usage carefully, and will hopefully result in fewer feet being shot by this footgun.
Attachment #746400 - Flags: review?(ehsan) → review-
I talked to terrence briefly about this, and he had a good point -- we really want a MOZ_NONHEAP_CLASS annotation. We're fine with either stack allocation or static allocation. Only heap allocation is bad. Most Handle<>s are stack allocated, but the sentinels are statically allocated. What do you think?
Flags: needinfo?(ehsan)
(In reply to comment #9)
> I talked to terrence briefly about this, and he had a good point -- we really
> want a MOZ_NONHEAP_CLASS annotation. We're fine with either stack allocation or
> static allocation. Only heap allocation is bad. Most Handle<>s are stack
> allocated, but the sentinels are statically allocated. What do you think?

That sounds fine.
Flags: needinfo?(ehsan)
Attachment #746401 - Flags: review?(benjamin) → review+
Attachment #746404 - Flags: review?(benjamin) → review+
Attachment #746400 - Attachment is obsolete: true
Attachment #754329 - Flags: review?(ehsan)
Attachment #754329 - Flags: review?(ehsan) → review+
Comment on attachment 754329 [details] [diff] [review]
Add MOZ_NONHEAP_CLASS

Typo in the MOZ_NONHEAP_CLASS documentation in mfbt/Attributes.h:
"it is considered to be a stack class as well"
I think you meant s/stack/nonheap/
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.