Ryan VanderMeulen [:RyanVM] Reporter
	Description • 12 years ago

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

	Joshua Cranmer [:jcranmer] Assignee
	Comment 1 • 12 years ago

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.

	Joshua Cranmer [:jcranmer] Assignee
	Comment 2 • 12 years ago

MOZ_IGNORE_STACK lets the static checking get ignored. This also adds it to the variable in question.

	Joshua Cranmer [:jcranmer] Assignee
	Comment 3 • 12 years ago

This fixes bug 859523.

	Joshua Cranmer [:jcranmer] Assignee
	Comment 4 • 12 years ago

How did I forget to change this... and how did this build in the first place?

	(no longer active)
	Comment 6 • 12 years ago

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.

	Steve Fink [:sfink] [:s:]
	Comment 7 • 12 years ago

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

	(no longer active)
	Comment 8 • 12 years ago

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.

	Steve Fink [:sfink] [:s:]
	Comment 9 • 12 years ago

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?

	(no longer active)
	Comment 10 • 12 years ago

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

	Joshua Cranmer [:jcranmer] Assignee
	Comment 12 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/68a574b28567
https://hg.mozilla.org/integration/mozilla-inbound/rev/44f3413b9bc6
https://hg.mozilla.org/integration/mozilla-inbound/rev/978ca38a477c
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d6a04be8c0f

	Mats Palmgren (inactive)
	Comment 13 • 11 years ago

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/

	Phil Ringnalda (:philor)
	Comment 14 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/68a574b28567
https://hg.mozilla.org/mozilla-central/rev/44f3413b9bc6
https://hg.mozilla.org/mozilla-central/rev/978ca38a477c
https://hg.mozilla.org/mozilla-central/rev/1d6a04be8c0f