855264 - BaselineCompiler: Fix compiler warnings

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1: Avoid zero-sized arrays

Avoid empty array if ProtoChainDepth is 0 by including the shape of the object itself in the array. MSVC warns about this and it's undefined behavior in C++.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2: Fix other MSVC warnings

Fixes warnings like:

warning C4146: unary minus operator applied to unsigned type, result still unsigned

warning C4355: 'this' : used in base member initializer list

The fix for the latter adds a thisFromCtor method, we use this trick elsewhere, for instance the JSRuntime constructor.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 3: Fix GCC warnings

Fixes warnings with GCC 4.7 (the dreaded "used but never defined" and a warning about an possibly-uninitialized variable in opt builds.

Comment 4

[Kannan Vijayan [:djvj]]

Comment on attachment 730114 [details] [diff] [review]
Part 1: Avoid zero-sized arrays

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

::: js/src/ion/BaselineIC.cpp
@@ +3570,5 @@
>          }
>  
>          if (kind == ICStub::SetElem_DenseAdd && iter->isSetElem_DenseAdd()) {
>              ICSetElem_DenseAdd *dense = iter->toSetElem_DenseAdd();
> +            if (obj->lastProperty() == dense->toImpl<0>()->shape(0) && obj->getType(cx) == dense->type())

|toImpl| here asserts that the proto chain depth matches, which will fail with this call.

You can use a manual cast here, or maybe add a |toImplUnchecked| method that doesn't do the assert.

@@ +3673,5 @@
>          if (curObj)
>              ++*protoDepthOut;
>      }
>  
> +    if (*protoDepthOut > ICSetElem_DenseAdd::MAX_PROTO_CHAIN_DEPTH)

Nice catch...

::: js/src/ion/BaselineIC.h
@@ +3200,5 @@
>  class ICSetElem_DenseAddImpl : public ICSetElem_DenseAdd
>  {
>      friend class ICStubSpace;
>  
> +    HeapPtrShape shapes_[ProtoChainDepth + 1];

Instead of using |ProtoChainDepth + 1| here and in all cases below, it would be clearer and reflect intent better to define a 'numShapes()' method and use that instead.

Comment 5

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/projects/ionmonkey/rev/3fc13eee44b5
https://hg.mozilla.org/projects/ionmonkey/rev/7d43d5cc0572
https://hg.mozilla.org/projects/ionmonkey/rev/71ff63071039

(In reply to Kannan Vijayan [:djvj] from comment #4)
> |toImpl| here asserts that the proto chain depth matches, which will fail
> with this call.
> 
> You can use a manual cast here, or maybe add a |toImplUnchecked| method that
> doesn't do the assert.

Good catch, I added toImplUnchecked.

> Instead of using |ProtoChainDepth + 1| here and in all cases below, it would
> be clearer and reflect intent better to define a 'numShapes()' method and
> use that instead.

Agreed. A method didn't work but fortunately "static const size_t NumShapes = ProtoChainDepth + 1;" did the trick.