Closed
Bug 868285
Opened 12 years ago
Closed 12 years ago
Clang static analysis builds currently perma-fail due to build error in nsCycleCollector.cpp
Categories
(Developer Infrastructure :: Source Code Analysis, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla24
People
(Reporter: RyanVM, Assigned: jcranmer)
Details
Attachments
(4 files, 1 obsolete file)
12.62 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
922 bytes,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
5.16 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
16.70 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
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....
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.
Assignee | ||
Comment 2•12 years ago
|
||
MOZ_IGNORE_STACK lets the static checking get ignored. This also adds it to the variable in question.
Assignee | ||
Comment 3•12 years ago
|
||
This fixes bug 859523.
Attachment #746401 -
Flags: review?(benjamin)
Assignee | ||
Comment 4•12 years ago
|
||
How did I forget to change this... and how did this build in the first place?
Attachment #746402 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #746404 -
Flags: review?(benjamin)
Updated•12 years ago
|
Attachment #746402 -
Flags: review?(mh+mozilla) → review+
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.
Flags: needinfo?(sphink)
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.
Flags: needinfo?(sphink)
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.
Attachment #746400 -
Flags: review?(ehsan) → review-
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?
Flags: needinfo?(ehsan)
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.
Flags: needinfo?(ehsan)
Updated•12 years ago
|
Attachment #746401 -
Flags: review?(benjamin) → review+
Updated•12 years ago
|
Attachment #746404 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 11•12 years ago
|
||
Attachment #746400 -
Attachment is obsolete: true
Attachment #754329 -
Flags: review?(ehsan)
Updated•12 years ago
|
Attachment #754329 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 12•12 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
Comment 13•12 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/
Comment 14•12 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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Updated•7 years ago
|
Product: Core → Firefox Build System
Updated•2 years ago
|
Product: Firefox Build System → Developer Infrastructure
You need to log in
before you can comment on or make changes to this bug.
Description
•