BaselineCompiler: Fix compiler warnings

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Comment hidden (empty)
(Assignee)

Comment 1

6 years ago
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++.
Attachment #730114 - Flags: review?(kvijayan)
(Assignee)

Comment 2

6 years ago
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.
Attachment #730117 - Flags: review?(kvijayan)
(Assignee)

Comment 3

6 years ago
Fixes warnings with GCC 4.7 (the dreaded "used but never defined" and a warning about an possibly-uninitialized variable in opt builds.
Attachment #730118 - Flags: review?(kvijayan)
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.
Attachment #730114 - Flags: review?(kvijayan) → review+
Attachment #730117 - Flags: review?(kvijayan) → review+
Attachment #730118 - Flags: review?(kvijayan) → review+
(Assignee)

Comment 5

6 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.