Last Comment Bug 726226 - IonMonkey: reverse Pool code does not use the correct pool
: IonMonkey: reverse Pool code does not use the correct pool
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: ARM Linux
: -- normal (vote)
: ---
Assigned To: general
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-10 16:28 PST by Marty Rosenberg [:mjrosenb]
Modified: 2012-02-17 19:50 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
/home/mrosenberg/patches/help-r0.patch (9.36 KB, patch)
2012-02-10 16:28 PST, Marty Rosenberg [:mjrosenb]
dvander: review+
Details | Diff | Splinter Review

Description Marty Rosenberg [:mjrosenb] 2012-02-10 16:28:05 PST
Created attachment 596219 [details] [diff] [review]
/home/mrosenberg/patches/help-r0.patch

There are two separate pools that are carried around, one for forward references, one for backwards references.  When we go to update the backwards reference, we do a check based on the forward pool, not the backwards one.  I also cleaned up some edge cases that I'd been conservative about.
Comment 1 David Anderson [:dvander] 2012-02-10 17:21:17 PST
Comment on attachment 596219 [details] [diff] [review]
/home/mrosenberg/patches/help-r0.patch

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

::: js/src/ion/shared/IonAssemblerBufferWithConstantPools.h
@@ +57,2 @@
>      const int alignment;
> +  public:

nit: need newlines above public: and private:

@@ +125,4 @@
>      bool checkFullBackref(int poolOffset, int codeOffset) {
> +        if (!limitingUser.assigned()) {
> +            return false;
> +        }

nit: no braces needed here

@@ +143,5 @@
>          JS_ASSERT(!isBackref);
> +        // Not full if there aren't any uses.
> +        if (!limitingUser.assigned()) {
> +            return false;
> +        }

ditto

@@ +202,5 @@
>      bool isAligned(uint32 ptr) {
>          return ptr == align(ptr);
>      }
> +    bool getAlignment() {
> +        return alignment;

getAlignment returns a bool and alignment is an int.

@@ +414,5 @@
>          for (cur = &pools[numPoolKinds-1]; cur >= pools; cur--) {
>              // fetch the pool for the backwards half.
>              tmp = cur->other;
> +            if (p == cur) {
> +                tmp->updateLimiter(this->nextOffset());

nit: no braces needed here

@@ +446,4 @@
>          uint32 poolOffset = nextOffset;
>          Pool *tmp;
>          // If we need a guard instruction, reserve space for that.
> +        if (perforatedNode == NULL)

style nit: if (!perforatedNode)

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