Last Comment Bug 707049 - Dynamic analysis for identifying moving GC hazards
: Dynamic analysis for identifying moving GC hazards
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Brian Hackett (:bhackett)
:
Mentors:
Depends on: 714647 745742 750733
Blocks: GenerationalGC
  Show dependency treegraph
 
Reported: 2011-12-01 17:59 PST by Brian Hackett (:bhackett)
Modified: 2012-05-19 12:35 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (3dca8a1ee5fe) (46.17 KB, patch)
2011-12-01 20:02 PST, Brian Hackett (:bhackett)
no flags Details | Diff | Splinter Review
patch (3dca8a1ee5fe) (228.68 KB, patch)
2011-12-03 19:51 PST, Brian Hackett (:bhackett)
no flags Details | Diff | Splinter Review
patch (3dca8a1ee5fe) (212.92 KB, patch)
2011-12-13 10:32 PST, Brian Hackett (:bhackett)
wmccloskey: review+
Details | Diff | Splinter Review
warnings/failures on trunk clang (2.63 KB, text/plain)
2011-12-31 06:16 PST, Tom Schuster [:evilpie]
no flags Details
fix warnings (3.61 KB, patch)
2011-12-31 06:16 PST, Brian Hackett (:bhackett)
jwalden+bmo: review+
Details | Diff | Splinter Review

Description Brian Hackett (:bhackett) 2011-12-01 17:59:18 PST
A moving GC needs exact stack scanning, which obviates use of the conservative scanner.  The conservative scanner can still be used, however, to identify stack locations that point to GC things but are not explicitly rooted.  A DEBUG-only runtime mode (similar to gczeal) could run the stack scanner when allocating GC things or performing operations that *can* allocate GC things but are unlikely to (ValueToNumber etc.).  Any stack location pointing to a GC thing which has not been explicitly rooted is painted over, such that if it is a live pointer then dereferencing it will crash.

This approach can have false negatives if the GC thing pointer is no longer live but dependent data is (uses of string->chars(), for example), but the conservative GC has the same problem.  Of greater concern are false positives, which can happen if the word being painted over is actually a live integer and may result in incorrect behavior or a crash.  The false positives and true positives can be distinguished by looking at the crash address (a true positive will crash near the painted-over address), and it should work to minimize false positives by running on a 64 bit platform and mmap'ing an unusual portion of the address space.  Hopefully things won't be too flaky.

I'd like to do both this and bug 658676, for thorough coverage of possible GC hazards.  I'd like to do this first, though, as since it is just a runtime mode then it will be simpler to get TBPL coverage.
Comment 1 Brian Hackett (:bhackett) 2011-12-01 20:02:57 PST
Created attachment 578485 [details] [diff] [review]
WIP (3dca8a1ee5fe)

WIP:

- Adds a new set of autorooters for relocatable stack locations (changing the existing AutoGCRooter to HeapGCRooter to distinguish them; the latter is only used to root GC references from heap structures like arrays/vectors).

- Adds a CheckStackRoots method which walks the current thread's native stack and paints over references to GC things which were not explicitly rooted.  For now this is called on every NewGCThing.

- Adds a few rooters, just enough to allocate the global object without crashing and burning when starting the shell.
Comment 2 David Mandelin [:dmandelin] 2011-12-02 11:35:08 PST
I'm really glad to see this.

We need to be careful about the autorooters: we went to a conservative GC because they were considered to be too hard to use correctly and maintain. They are fine as an intermediate step, but I'm still concerned about exposing them to general development.
Comment 3 Brian Hackett (:bhackett) 2011-12-03 19:51:12 PST
Created attachment 578882 [details] [diff] [review]
patch (3dca8a1ee5fe)

Patch with just enough rooting to handle a print('hello world') script.
Comment 4 Brian Hackett (:bhackett) 2011-12-13 10:32:50 PST
Created attachment 581321 [details] [diff] [review]
patch (3dca8a1ee5fe)

Cleaned up some, and comments added.  This adds some rooting classes, described in js/src/jscntxt.h for rooting stack variables/locations and creating handles (references) to them.  It also adds a dynamic analysis for finding rooting bugs, enabled by --enable-root-analysis, and adds enough rooting to the VM for the analysis to pas a hello world program.  For now, the rooters don't do anything if the analysis is not enabled (any value they would root will be found by the conservative scanner).

Because of codebase churn I'd like to land this first before adding more rooters in the rest of the VM or changing the API to use handles.  The use of handles is pretty limited at this point; in followup patches it would be good to replace some of the Root<T>'s with handles, but the best roles for the various classes is still not quite clear.
Comment 5 :Ms2ger 2011-12-13 10:37:42 PST
Would this be used by Gecko?
Comment 6 Brian Hackett (:bhackett) 2011-12-13 10:40:21 PST
The analysis will certainly work on Gecko, but I think we will want an API that uses handles, which will largely (but not entirely) obviate the need for the analysis.  Rooting bugs will generally show up as compile errors.
Comment 7 Bill McCloskey (:billm) 2011-12-27 13:40:02 PST
Comment on attachment 581321 [details] [diff] [review]
patch (3dca8a1ee5fe)

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

Very nice patch. I really like the changes to the shape code; the special stack shapes make everything cleaner.

I think we're in agreement that most of the uses of RootX should be removed by passing a handle in as an argument. But I think it's better to get this in before it rots too badly.

A little name bikeshedding: I would prefer RootedXVar to RootVarX.

Also, how hard will it be for us to run the Sixgill analysis locally? How long does it take to run? Is it hard to set up?

::: js/src/configure.in
@@ +4451,5 @@
>  
>  dnl ========================================================
> +dnl = Perform moving GC stack rooting analysis
> +dnl ========================================================
> +JSGC_ROOT_ANALYSIS=1

I'm not configure expert. Why is this line here? It seems like the code for other options doesn't have this.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +5461,5 @@
>       */
>      JSFunction *fun = js_NewFunction(cx, NULL, NULL,
>                                       funbox->function()->nargs,
>                                       funbox->function()->flags,
> +                                     parent,

Could you just pass this as RootVarObject(cx, funbox->function()->getParent()) ?

::: js/src/jsapi.h
@@ +201,5 @@
>  }
>  #endif  /* defined(__GNUC__) */
>  
> +template <typename T>
> +inline void PoisonPtr(T *v)

It seems like this is only used once, and I found the type confusing. Could you make it just PoisonPtr(void **pptr) or something like that?

@@ +205,5 @@
> +inline void PoisonPtr(T *v)
> +{
> +#ifdef DEBUG
> +    uint8 *ptr = (uint8 *) v + 3;
> +    *ptr = JS_FREE_PATTERN;

Isn't this 32-bit specific?

::: js/src/jsarray.cpp
@@ +3676,5 @@
>  
> +    Root<GlobalObject*> parentRoot(cx, &parent);
> +
> +    if (!proto) {
> +        if (!FindProto(cx, &ArrayClass, parentRoot, &proto))

Why split into two ifs?

@@ +3682,5 @@
> +    }
> +
> +    RootObject protoRoot(cx, &proto);
> +    RootVarTypeObject type(cx);
> +    RootVarShape shape(cx);

Why not wait to define shape until it's used?

::: js/src/jscntxt.h
@@ +1516,5 @@
> +#endif
> +
> +        JS_ASSERT(!RootMethods<T>::poisoned(*ptr));
> +
> +        this->ptr = ptr;

One thing that scares me about Root<T> is that if we forget to assign to the given variable before a GC, then the GC will potentially read garbage memory. As much as possible, I think we should try to move away from Root<T> toward RootVar<T>.

@@ +1522,5 @@
> +        JS_GUARD_OBJECT_NOTIFIER_INIT;
> +    }
> +
> +    ~Root()
> +    {

I think the brace should go on the same line here.

@@ +1614,5 @@
> +{
> +  public:
> +    RootVar(JSContext *cx)
> +        : ptr(RootMethods<T>::initial()), root(cx, &ptr)
> +    {}

Could you add a constructor that takes an initial value? It seems like this could be useful throughout, at least when the value to be initially assigned isn't too long.

::: js/src/jsfun.cpp
@@ +131,5 @@
>      if (!proto)
>          return NULL;
>  
> +    RootVarTypeObject type(cx);
> +    RootVarShape emptyArgumentsShape(cx);

Could the shape be moved down to its first use?

@@ +626,5 @@
>  static inline JSObject *
>  NewDeclEnvObject(JSContext *cx, StackFrame *fp)
>  {
> +    RootVarTypeObject type(cx);
> +    RootVarShape emptyDeclEnvShape(cx);

Same here.

@@ +1404,5 @@
>          flagsword = (fun->nargs << 16) | fun->flags;
>          script = fun->script();
>      } else {
> +        RootVarObject parent(cx);
> +        fun = js_NewFunction(cx, NULL, NULL, 0, JSFUN_INTERPRETED, parent, NULL);

Should just be able to pass RootVarObject(cx, NULL) and eliminate parent.

::: js/src/jsfun.h
@@ +292,5 @@
>  
>  inline JSFunction *
>  JSObject::toFunction()
>  {
> +    JS_ASSERT_IF(this != NULL, JS_ObjectIsFunction(NULL, this));

No need for != NULL.

::: js/src/jsgc.cpp
@@ +915,3 @@
>   */
>  inline ConservativeGCTest
> +TestForGCThingWord(JSTracer *trc, jsuword w, bool markThing)

It looks like this markThing parameter isn't used. And since you use the EmptyMarkCallback, it seems unnecessary.

@@ +3273,5 @@
> +
> +    ThreadData *td = JS_THREAD_DATA(cx);
> +
> +    ConservativeGCThreadData *ctd = &td->conservativeGC;
> +    ctd->recordStackTop();

It's easier to use RecordNativeStackTop here. And you also need to use AutoCopyFreeListToArenas, since the conservative scanner traverses the free lists. Also, please add a JS_ASSERT(!JS_THREADSAFE) to this code so that it's clear that it's shell-only.

::: js/src/jsgcinlines.h
@@ +390,5 @@
>      if (cx->runtime->needZealousGC())
>          js::gc::RunDebugGC(cx);
>  #endif
>  
> +    js::gc::MaybeCheckStackRoots(cx);

Don't forget to fix up the methodjit allocator, whenever you get to that.

::: js/src/jsinferinlines.h
@@ +1334,3 @@
>          return false;
> +    if (!self->types->hasScope()) {
> +        js::CheckRoot root(cx, &self);

Why not just root self?

::: js/src/jsiter.cpp
@@ +421,5 @@
>  NewIteratorObject(JSContext *cx, uintN flags)
>  {
>      if (flags & JSITER_ENUMERATE) {
> +        RootVarTypeObject type(cx);
> +        RootVarShape emptyEnumeratorShape(cx);

Why not declare these when they're used, and pass the initial value as a parameter to the ctor?

::: js/src/jsobj.cpp
@@ +2923,5 @@
>      HeapValue *slots;
>      if (!PreallocateObjectDynamicSlots(cx, shape, &slots))
>          return NULL;
>  
> +    JSObject* obj = JSObject::create(cx, kind, shape, typeRoot, slots);

Could you make this |JSObject *obj|?

@@ +3515,5 @@
>  {
>      JSObject *obj;
>  
> +    RootVarTypeObject type(cx);
> +    RootVarShape emptyWithShape(cx);

Please move the def down to the use.

@@ +3555,5 @@
>  JSObject *
>  js_NewBlockObject(JSContext *cx)
>  {
> +    RootVarTypeObject type(cx);
> +    RootVarShape emptyBlockShape(cx);

Move this down as well.

@@ +3575,5 @@
>  {
>      JS_ASSERT(proto->isStaticBlock());
>  
> +    RootVarTypeObject type(cx);
> +    RootVarShape shape(cx);

Move down.

@@ +5031,5 @@
>              return true;
>          }
>      }
>  
> +    OBJ_TO_INNER_OBJECT(cx, *obj.address());

It would be nice to kill this macro...

::: js/src/jsobjinlines.h
@@ +1718,5 @@
>      return JSProto_Null;
>  }
>  
>  inline bool
> +FindProto(JSContext *cx, js::Class *clasp, HandleObject parent, JSObject ** proto)

Could you fix |** proto|?

::: js/src/jspropertytree.cpp
@@ +60,5 @@
>  
>  inline bool
>  ShapeHasher::match(const Key k, const Lookup l)
>  {
> +    return k->matches(*l);

I think we can improve this code by:
  - Changing Lookup to be typedefed to StackShape (not const StackShape *)
  - Changing the signature of match and hash to take a const Lookup &

Then...

@@ +87,5 @@
> +    StackShape nkid1(kid1);
> +    StackShape nkid2(kid2);
> +
> +    JS_ALWAYS_TRUE(hash->putNew(&nkid1, kid1));
> +    JS_ALWAYS_TRUE(hash->putNew(&nkid2, kid2));

You can just make these be
    JS_ALWAYS_TRUE(hash->putNew(kid1, kid1));
and the same for all the others below. I think it should automatically make a temporary StackShape with its Shape* constructor.

::: js/src/jsprvtd.h
@@ +281,5 @@
> +    THING_ROOT_STRING,
> +    THING_ROOT_SCRIPT,
> +    THING_ROOT_ID,
> +    THING_ROOT_VALUE,
> +    THING_ROOT_COUNT

I think we usually use _LIMIT instead of _COUNT.

::: js/src/jsscope.cpp
@@ +412,3 @@
>                  return NULL;
>          }
> +        shape->initDictionaryShape(child, (Shape **) &self->shape_);

Doesn't this fail to execute the write barrier for self->shape_?

@@ +416,5 @@
>          shape = JS_PROPERTY_TREE(cx).getChild(cx, parent, child);
>          if (!shape)
>              return NULL;
> +        //JS_ASSERT(shape->parent == parent);
> +        //JS_ASSERT_IF(parent != lastProperty(), parent == lastProperty()->parent);

Could we keep these assertions if |parent| is rooted?

@@ +459,5 @@
> +
> +        Shape **listp = dictionaryShape ? (Shape **) &dictionaryShape->parent : root.address();
> +
> +        StackShape child(shape);
> +        dprop->initDictionaryShape(child, listp);

This one also fails to execute a write barrier, but it's probably okay here--dictionaryShape is newly allocated, and we probably aren't going to allocate shapes in the nursery.

@@ +728,3 @@
>      /* Search for id in order to claim its entry if table has been allocated. */
> +    Shape **spp = Shape::tableSearch(cx, lastProperty(), id, true);
> +    Shape *shape = spp ? SHAPE_FETCH(spp) : Shape::linearSearch(self->lastProperty(), id);

Could you add an |adding| parameter to Shape::search that defaults to false? Then I think you could use that here.

@@ +930,5 @@
> +    RootId idRoot(cx, &id);
> +    RootVarShape shape(cx);
> +
> +    Shape **spp = Shape::tableSearch(cx, lastProperty(), id);
> +    shape = spp ? SHAPE_FETCH(spp) : Shape::linearSearch(self->lastProperty(), id);

Why can't you use Shape::search here?

@@ +1083,5 @@
>  
> +    Shape *oldShape = self->lastProperty();
> +
> +    StackShape nshape(self->lastProperty());
> +    newShape->initDictionaryShape(nshape, (Shape **) &self->shape_);

Again, I think a write barrier is missed here.

@@ +1247,5 @@
>      JSDHashNumber hash = base->flags;
>      hash = JS_ROTATE_LEFT32(hash, 4) ^ (jsuword(base->clasp) >> 3);
> +    hash = JS_ROTATE_LEFT32(hash, 4) ^ (jsuword(base->parent) >> 3);
> +    hash = JS_ROTATE_LEFT32(hash, 4) ^ jsuword(base->rawGetter);
> +    hash = JS_ROTATE_LEFT32(hash, 4) ^ jsuword(base->rawSetter);

Why did you take out the if tests on rawGetter and rawSetter? It seems like it makes the hash function less precise.

@@ +1365,5 @@
> +    /*
> +     * This may be invoked on GC heap allocated bindings, in which case this
> +     * is pointing to an internal value of a JSScript that can't itself be
> +     * relocated. The script itself will be rooted, and will not be moved, so
> +     * mark the stack value as non-relocatable for the stack root analysis.

I'm not sure I understand this comment. Even without conservative scanning, do you envision still pinning objects in some cases? That doesn't seem very appealing. What if we had a version of RootVar that allowed you to specify an offset to the actual GC thing start?

This brings up another question: do we allow the pointer stored in a Root or RootVar to be a non-GC thing? As long as it's a pointer, I don't think this would be a problem. We just wouldn't touch it if it's not a GC thing.

::: js/src/jsscope.h
@@ +507,5 @@
> +    StackBaseShape(Class *clasp, JSObject *parent, uint32 objectFlags)
> +      : flags(objectFlags), clasp(clasp), parent(parent), rawGetter(NULL), rawSetter(NULL)
> +    {}
> +
> +    inline void updateGetterSetter(uint8 attrs, PropertyOp rawGetter, StrictPropertyOp rawSetter);

It seems like this is always called after the constructor. Why not add a constructor that takes BaseShape*, attrs, getter, and setter? I guess it could be confusing, since BaseShape has its own getter and setter. But I think people would probably understand.

@@ +648,5 @@
>              cursor = cursor->parent;
>          }
>      };
>  
> +    class RootRange {

It seems like it would make more sense for this to be a member of Shape::Range.

@@ +1046,5 @@
> +    int16            shortid;
> +
> +    StackShape(UnownedBaseShape *base, jsid propid, uint32 slot,
> +               uint32 nfixed, uintN attrs, uintN flags, intN shortid)
> +      : base(base), propid(propid), slotInfo(slot | (nfixed << Shape::FIXED_SLOTS_SHIFT)),

One initializer per line is nicer if it will be more than one line.

@@ +1058,5 @@
> +
> +    StackShape(const Shape *shape)
> +      : base(shape->base()->unowned()), propid(shape->maybePropid()),
> +        slotInfo(shape->slotInfo & ~Shape::LINEAR_SEARCHES_MASK),
> +        attrs(shape->attrs), flags(shape->flags), shortid(shape->shortid_)

One initializer per line here too.

@@ +1121,5 @@
>  /*
> + * If shape has a property table, search the table and return the table entry
> + * which would contain the specified id, whether there is such a shape or not.
> + * Return NULL if the shape does not yet have a table, and may construct one
> + * (possibly triggering GC) if enough linear searches have been performed.

The last sentence is a little confusing. How about:
"This method may construct a table (possibly triggering a GC) if enough linear searches have been performed. Otherwise it return NULL."

::: js/src/jsstr.cpp
@@ +1564,3 @@
>      RegExpStatics *res = cx->regExpStatics();
>      Value rval;
>      if (!DoMatch(cx, res, str, *rep, MatchCallback, arg, MATCH_ARGS, &rval))

Just looking at the code for MatchCallback, it seems like there's more work to be done there. But I guess there's no need for completeness yet.

::: js/src/vm/Debugger.cpp
@@ +3496,5 @@
> +        debugCtor(cx),
> +        debugProto(cx),
> +        frameProto(cx),
> +        scriptProto(cx),
> +        objectProto(cx);

Why not declare these right before their uses?

::: js/src/vm/GlobalObject.cpp
@@ +124,2 @@
>      {
> +        functionProto = NewObjectWithGivenProto(cx, &FunctionClass, objectProto, self)->toFunction();

This can call ->toFunction() on NULL, which I think it's best to avoid.

@@ +171,3 @@
>          if (!objectCtor)
>              return NULL;
> +        //JS_ASSERT(ctor == objectCtor);

Just remove this, or else do the same swap as above.

@@ +269,5 @@
>      JS_ASSERT(clasp->flags & JSCLASS_IS_GLOBAL);
>  
> +    RootVar<GlobalObject*> obj(cx);
> +
> +    obj = NewObjectWithGivenProto(cx, clasp, NULL, NULL)->asGlobal();

Same as above about ->asGlobal() and NULL.

::: js/src/vm/GlobalObject.h
@@ +369,5 @@
>  
>  js::GlobalObject *
>  JSObject::asGlobal()
>  {
> +    JS_ASSERT_IF(this != NULL, isGlobal());

No != NULL, please.
Comment 8 Brian Hackett (:bhackett) 2011-12-28 07:28:31 PST
(In reply to Bill McCloskey (:billm) from comment #7)
> A little name bikeshedding: I would prefer RootedXVar to RootVarX.

How about RootedVarX?  Then it works similarly to HeapPtrX, and (as with HeapPtr) it is nice for the typedef to basically be removing <> i.e. HeapPtr<JSObject> === HeapPtrObject.

> Also, how hard will it be for us to run the Sixgill analysis locally? How
> long does it take to run? Is it hard to set up?

It would be better to run sixgill on a shared machine, e.g. dm-sixgill01.  It may be easier to run locally now, as for a while it required a custom GCC patch but now that patch has made its way into GCC trunk (not sure about a release).  Run time is not long (30min to 1hr, depending on specs) on the JS engine, but a few hours and a beefy machine are necessary for Firefox itself or a similarly large project.

> @@ +205,5 @@
> > +inline void PoisonPtr(T *v)
> > +{
> > +#ifdef DEBUG
> > +    uint8 *ptr = (uint8 *) v + 3;
> > +    *ptr = JS_FREE_PATTERN;
> 
> Isn't this 32-bit specific?

This should work for both 32 bit and 64 bit systems, but requires that the byte which gets poisoned will actually repoint the word to unaddressed memory.  The problem with poisoning other parts of the word or the entire word is that it is possible that the compiler will allocate an integer over a portion of a dead stack GC pointer, and we don't want to inadvertently poison that integer.  This placement avoids false positives on the testing done so far.

I'll add a comment about this.

> @@ +1365,5 @@
> > +    /*
> > +     * This may be invoked on GC heap allocated bindings, in which case this
> > +     * is pointing to an internal value of a JSScript that can't itself be
> > +     * relocated. The script itself will be rooted, and will not be moved, so
> > +     * mark the stack value as non-relocatable for the stack root analysis.
> 
> I'm not sure I understand this comment. Even without conservative scanning,
> do you envision still pinning objects in some cases? That doesn't seem very
> appealing. What if we had a version of RootVar that allowed you to specify
> an offset to the actual GC thing start?

This comment is presuming that scripts won't be allocated in the nursery, and won't be moved.  That assumption should hold for a generational GC, but probably won't for a compacting GC.  So this CheckRoot should be removed if we want to be able to move JSScripts around, which would require refactoring the code a bit.  The basic problem is that the pointers stored in Root* structures should be pointers to GC things, but not pointers to internal bits of GC things, like JSScript::bindings.  Moving such internal pointers would be complicated/impossible with a heterogenous nursery.

> This brings up another question: do we allow the pointer stored in a Root or
> RootVar to be a non-GC thing? As long as it's a pointer, I don't think this
> would be a problem. We just wouldn't touch it if it's not a GC thing.

I think the root structures should only hold either NULL or a pointer to a valid GC thing, to avoid problems with internal pointers as described above.
Comment 9 Brian Hackett (:bhackett) 2011-12-30 19:13:31 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d642e4e08cf
Comment 10 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-30 23:13:28 PST
I can't build the browser or shell with this patch landed, either using clang nearly-trunk or gcc 4.6.1.  Shell build options are pretty pedestrian:

  ../configure --enable-debug --disable-optimize --enable-readline --enable-valgrind

Browser build options are basically the same.

Linking the JS engine fails with an undefined reference to js::Shape::~Shape, called in EmptyShape::EmptyShape(UnownedBaseShape *base, uint32_t nfixed):

jsscope.o: In function `js::EmptyShape::EmptyShape(js::UnownedBaseShape*, unsigned int)':
/home/jwalden/moz/slots/js/src/dbg/../jsscopeinlines.h:355: undefined reference to `js::Shape::~Shape()'
/usr/bin/ld: libmozjs.so: hidden symbol `_ZN2js5ShapeD2Ev' isn't defined
/usr/bin/ld: final link failed: Bad value
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[1]: *** [libmozjs.so] Error 1
make: *** [default] Error 2

If I comment out ~Shape(), things build.  Is it really kosher to declare a destructor but not define it, then inherit from that class?  I don't know the spec language well enough to say this is definitely wrong, but it definitely seems dodgy.
Comment 11 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-30 23:15:34 PST
Oh, the patch here introduced warnings on every platform into the JS engine.  Please fix them.

And if you're privately declaring a method to ensure that using it will trigger an error, as the comments by Shape(const Shape&) and ~Shape() indicate, you should use MOZ_DELETE on them, from the "mozilla/Attributes.h" header (in mfbt/Attributes.h).
Comment 12 Tom Schuster [:evilpie] 2011-12-31 06:16:08 PST
Created attachment 585131 [details]
warnings/failures on trunk clang

Ms2ger backed this out for me, thanks! Please fix the build errors, this even regressed SM builds on the tinderbox.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4282a285d381
Comment 13 Brian Hackett (:bhackett) 2011-12-31 06:16:58 PST
Created attachment 585133 [details] [diff] [review]
fix warnings

Remove shape destructor, and fix warnings showing up on fail-on-warnings builds on inbound.  I didn't get any warnings with my build configuration.
Comment 14 Brian Hackett (:bhackett) 2011-12-31 06:35:30 PST
Backout backed out.  Clang build errors are not cause to backout patches.  I'm testing the comment 13 patch on try and will push that if it looks good.

https://hg.mozilla.org/integration/mozilla-inbound/rev/10f831bfaf08
Comment 15 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-31 10:34:40 PST
Comment on attachment 585133 [details] [diff] [review]
fix warnings

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

The build failure was more than with just clang -- it was hitting my builds with gcc 4.6.1 as well.

::: js/src/jsscope.h
@@ +707,5 @@
>      /* Used by EmptyShape (see jsscopeinlines.h). */
>      Shape(UnownedBaseShape *base, uint32_t nfixed);
>  
>      /* Copy constructor disabled, to avoid misuse of the above form. */
> +    Shape(const Shape &other) MOZ_DELETE;

You need to add a #include "mozilla/Attributes.h" to this file so that it's not bootlegging it from who knows where.
Comment 16 Brian Hackett (:bhackett) 2011-12-31 11:40:15 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/905e6efc3cce
Comment 17 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-31 12:13:37 PST
gcc 4.6.1 has better warnings than whichever versions you've tested or which are on tinderbox, and the not-used warning is still there:

In file included from /home/jwalden/moz/slots/js/src/jsclass.h:49:0,
                 from /home/jwalden/moz/slots/js/src/jsfriendapi.h:43,
                 from /home/jwalden/moz/slots/js/src/jscntxt.h:50,
                 from /home/jwalden/moz/slots/js/src/jsarray.h:45,
                 from /home/jwalden/moz/slots/js/src/jsobj.cpp:56:
/home/jwalden/moz/slots/js/src/jsprvtd.h: In member function ‘void js::Handle<T>::testAssign() [with S = js::types::TypeObject*, T = js::types::TypeObject*]’:
/home/jwalden/moz/slots/js/src/jscntxt.h:1577:5:   instantiated from ‘js::Handle<T>::Handle(const js::Root<S>&) [with S = js::types::TypeObject*, T = js::types::TypeObject*]’
/home/jwalden/moz/slots/js/src/jsobj.cpp:2931:70:   instantiated from here
/home/jwalden/moz/slots/js/src/jsprvtd.h:331:11: warning: variable ‘a’ set but not used [-Wunused-but-set-variable]
/home/jwalden/moz/slots/js/src/jsprvtd.h: In member function ‘void js::Handle<T>::testAssign() [with S = JSObject*, T = JSObject*]’:
/home/jwalden/moz/slots/js/src/jscntxt.h:1577:5:   instantiated from ‘js::Handle<T>::Handle(const js::Root<S>&) [with S = JSObject*, T = JSObject*]’
/home/jwalden/moz/slots/js/src/jsobj.cpp:3012:49:   instantiated from here
/home/jwalden/moz/slots/js/src/jsprvtd.h:331:11: warning: variable ‘a’ set but not used [-Wunused-but-set-variable]
Comment 19 :Ms2ger 2012-01-01 04:32:57 PST
https://hg.mozilla.org/mozilla-central/rev/0d642e4e08cf#l36.170

  36.172 +Shape::insertIntoDictionary(HeapPtrShape *dictp)
  36.173  {
  36.189 +    listp = (HeapPtrShape *) dictp;

What's the point of this cast?

(Note that the signature in the reviewed patch is Shape::insertIntoDictionary(Shape **dictp).)
Comment 20 Igor Bukanov 2012-01-01 19:37:56 PST
I got many warnings with GCC:

/home/igor/m/mc/js/src/jsprvtd.h: In member function ‘void js::Handle<T>::testAssign() [with S = js::GlobalObject*, T = JSObject*]’:
/home/igor/m/mc/js/src/jscntxt.h:1577:5:   instantiated from ‘js::Handle<T>::Handle(const js::Root<S>&) [with S = js::GlobalObject*, T = JSObject*]’
/home/igor/m/mc/js/src/jsarray.cpp:3679:5:   instantiated from ‘JSObject* js::NewArray(JSContext*, uint32_t, JSObject*) [with bool allocateCapacity = false, JSObject = JSObject, JSContext = JSContext, uint32_t = unsigned int]’
/home/igor/m/mc/js/src/jsarray.cpp:3716:40:   instantiated from here
/home/igor/m/mc/js/src/jsprvtd.h:331:11: warning: variable ‘a’ set but not used [-Wunused-but-set-variable]


This happens due to testAssign not using a after the assignment. To fix the warning I have to change testAssign to:

#ifdef DEBUG
    template <typename S>
    T testAssign() {
        T a = RootMethods<T>::initial();
        S b = RootMethods<S>::initial();
        a = b;
        return a;
    }
#else
    template <typename S>
    void testAssign() {
    }
#endif
Comment 21 Brian Hackett (:bhackett) 2012-01-02 09:24:24 PST
(In reply to Igor Bukanov from comment #20)
> I got many warnings with GCC:
> 
> /home/igor/m/mc/js/src/jsprvtd.h: In member function ‘void
> js::Handle<T>::testAssign() [with S = js::GlobalObject*, T = JSObject*]’:
> /home/igor/m/mc/js/src/jscntxt.h:1577:5:   instantiated from
> ‘js::Handle<T>::Handle(const js::Root<S>&) [with S = js::GlobalObject*, T =
> JSObject*]’
> /home/igor/m/mc/js/src/jsarray.cpp:3679:5:   instantiated from ‘JSObject*
> js::NewArray(JSContext*, uint32_t, JSObject*) [with bool allocateCapacity =
> false, JSObject = JSObject, JSContext = JSContext, uint32_t = unsigned int]’
> /home/igor/m/mc/js/src/jsarray.cpp:3716:40:   instantiated from here
> /home/igor/m/mc/js/src/jsprvtd.h:331:11: warning: variable ‘a’ set but not
> used [-Wunused-but-set-variable]

These should be fixed with bug 714563.
Comment 22 Brian Hackett (:bhackett) 2012-01-02 09:25:22 PST
(In reply to Ms2ger from comment #19)
> https://hg.mozilla.org/mozilla-central/rev/0d642e4e08cf#l36.170
> 
>   36.172 +Shape::insertIntoDictionary(HeapPtrShape *dictp)
>   36.173  {
>   36.189 +    listp = (HeapPtrShape *) dictp;
> 
> What's the point of this cast?
> 
> (Note that the signature in the reviewed patch is
> Shape::insertIntoDictionary(Shape **dictp).)

I'll remove the cast, it's unnecessary now.  The signature of insertIntoDictionary previously required casts in both the caller and the callee, with the new signature neither are necessary.
Comment 23 :Ms2ger 2012-01-03 03:59:31 PST
I removed a couple uint8s added in this bug:

https://hg.mozilla.org/mozilla-central/rev/e433daca145f

Note You need to log in before you can comment on or make changes to this bug.