397177 - vp parameter to [gs]etPropertyOps is not rooted

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect, P1)

Description

[Blake Kaplan (:mrbkap) (inactive)]

From IRC yesterday:
18:59 < brendan> argh, it's the sprop that is rooted
19:00 < brendan> mrbkap: file a bug
19:00 < brendan> cc the usual suspects

Comment 1

[Brendan Eich [:brendan]]

This will take a bit more time, but it looks doable without adding interpreter overhead.

/be

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

Moving bugs that aren't beta 4 blockers to target final release.

Comment 3

[Mike Schroepfer]

Blake Brendan - we are past RC1 freeze - we ok on this bug?

Comment 4

[Mike Schroepfer]

Per commnets from Bkap:

"IMO this can wait. We don't know of anywhere this is biting us. Fixing it will make new code easier to write without GC hazards, but we can document this 'problem' in the SpiderMonkey API documentation."

So I'm taking off the blocker list - Brendan/BKap adjust if this is wrong..

Comment 5

[Brendan Eich [:brendan]]

Attachment: work in progress

Igor, your comments welcome. If you want to steal this bug, please feel free. Thanks,

/be

Comment 6

[Brendan Eich [:brendan]]

Attachment: proposed patch

Comment 7

[Igor Bukanov]

Comment on attachment 341056 [details] [diff] [review]
proposed patch

> 
>           BEGIN_CASE(JSOP_DELPROP)
>             LOAD_ATOM(0);
>             id = ATOM_TO_JSID(atom);
>-            PROPERTY_OP(-1, OBJ_DELETE_PROPERTY(cx, obj, id, &rval));
>-            STORE_OPND(-1, rval);
>+            PROPERTY_OP(-1, OBJ_DELETE_PROPERTY(cx, obj, id, &regs.sp[-1]));

Here regs.sp[-1] aliases roots the obj. Thus this will work only if deleteProperty sets *rval only after it finished working with obj. This may not be the case. The same problem exists with other cases in the patch. The proper solution would be to use JOF_TMPSLOT with operations that needs the extra rooting.

Comment 8

[Brendan Eich [:brendan]]

Some of those cases are E4X ones that do indeed set *vp only when they are about to return. I want to kick E4X out to library code by desugaring its syntax to plain old function calls, so I would rather not fix these cases with JOF_TMPSLOT.

js_DeleteProperty however clobbers the result early, as you say. Will patch again tomorrow, thanks.

/be

Comment 9

[Igor Bukanov]

(In reply to comment #7)
> The proper solution would be to use JOF_TMPSLOT with operations that needs the extra rooting.

Another alternative is to change deleteProperty etc. signatures from

  (JSContext *cx, JSObject *obj, jsid id, jsval *vp) 

to

  (JSContext *cx, jsid id, jsval *vp);

using vp both to pass the object and to return the result. This avoids introducing temp slots but may force to use temp_roots on slow paths in implementations of the JSObjectOps hooks.

Comment 10

[Brendan Eich [:brendan]]

Attachment: proposed patch, v2

As noted, I've left some E4X internal APIs known to set *vp just before returning as hazardous but currently correct, to avoid wasting more time on E4X pending its demotion/generalization to plugable syntactic sugar for function calls. This passes the testsuite with no novel failures.

This exercise was useful. JOF_TMPSLOT is essentially free, and we need it in many cases. I'm building a TM-related patch on top of this, so fairly prompt review to help it land in tracemonkey is appreciated. Thanks,

/be

Comment 11

[Igor Bukanov]

(In reply to comment #10)
> Created an attachment (id=341579) [details]
> proposed patch, v2
> 
> As noted, I've left some E4X internal APIs known to set *vp just before
> returning as hazardous but currently correct, 

But why bother with those E4X cases at all increasing chances, howeever small, of GC hazards? I.e. why not leave them alone taking rval as before (but probably making that rval case-local to help the compiler). 

> This exercise was useful. JOF_TMPSLOT is essentially free, 

tmpslot can be completely free if the stack space would be zeroed on allocation and the GC would zero unused portions of the stack. But that is for another bug.

Comment 12

[Igor Bukanov]

Comment on attachment 341579 [details] [diff] [review]
proposed patch, v2

>           BEGIN_CASE(JSOP_DELPROP)
>             LOAD_ATOM(0);
>             id = ATOM_TO_JSID(atom);
>-            PROPERTY_OP(-1, OBJ_DELETE_PROPERTY(cx, obj, id, &rval));
>-            STORE_OPND(-1, rval);
>+            PROPERTY_OP(-1, OBJ_DELETE_PROPERTY(cx, obj, id, &regs.sp[0]));
>+            regs.sp[-1] = regs.sp[0];
>           END_CASE(JSOP_DELPROP)


This is wrong: to get a root regs.sp has to be adjusted as the gc uses the current sp for the roots assuming that the interpreter takes care to set it properly.

> 
>           BEGIN_CASE(JSOP_DELELEM)
>-            ELEMENT_OP(-1, OBJ_DELETE_PROPERTY(cx, obj, id, &rval));
>-            regs.sp--;
>-            STORE_OPND(-1, rval);
>+            ELEMENT_OP(-1, OBJ_DELETE_PROPERTY(cx, obj, id, &regs.sp[0]));
>+            regs.sp[-2] = regs.sp[0];
>+            regs.sp--;
>           END_CASE(JSOP_DELELEM)

The same goes here. I will review the patch in full when all such issues will be addressed.

Comment 13

[Brendan Eich [:brendan]]

Of course, I should have seen this right away. It would be better not to add more stack pointer adjustments than needed. One idea is to have the GC add one or two slots to sp based on the current op at *fp->pc having JOF_TMPSLOT_MASK bits set. Comments?

/be

Comment 14

[Igor Bukanov]

Dependency on bug 448828: if we would fix that bug, the latest patch would work.

Comment 15

[Brendan Eich [:brendan]]

Igor, would you please take this bug too? Thanks,

/be

Comment 16

[Andrew Paprocki]

I think I just hit a crash caused by this. I was storing a new object in *vp in a get_property op and it was being killed immediately after in the next JSAPI call with gczeal enabled. I followed vp up the call chain and it is indeed coming from jsinterp so it looks like the same issue as this. Is this being worked on?

Comment 17

[Brendan Eich [:brendan]]

Yes, this bug is important. Igor, if you have not got to it yet and do not mind, I would like to take it back and work on it tomorrow. Thanks,

/be

Comment 18

[Igor Bukanov]

(In reply to comment #17)
> Yes, this bug is important. Igor, if you have not got to it yet and do not
> mind, I would like to take it back and work on it tomorrow. Thanks,

I hope to have a patch for bug 448828 before Monday which should make it very simple to address this bug. But efficient pc->pcdepth calculations are notoriously thorny.

Comment 19

[Igor Bukanov]

This goes back to the pool.

Comment 20

[Mike Moening]

I'm hitting this bug too.
Comment# 16 describes what is happening to me as well.

I've got JS stability problems that I ***think*** are related to new multi-threaded changes and JIT.  But I'm trying to make sure my code is clean by turning on GC_ZEAL level 2 before I cry wolf.

Any cheap workarounds?
OR
better yet can we fix this in the engine?

Comment 21

[Jason Orendorff [:jorendorff]]

Taking.

Comment 22

[Jason Orendorff [:jorendorff]]

My theory is that these places aren't so performance-sensitive anymore.  Reading a property with a non-default getter forces us off trace, which is many times more painful than incrementing sp.

So I plan to fix this the easy way--no examining bytecode from the GC (as I suspect comment 18 was implying).

Comment 23

[Igor Bukanov]

(In reply to comment #22)
> So I plan to fix this the easy way--no examining bytecode from the GC (as I
> suspect comment 18 was implying).

Yes, this is what the comment 18 implied.

Comment 24

[Brendan Eich [:brendan]]

Problem is we are still lacking trace coverage for common patterns (recursion, apply-arguments, a few others), so with good blacklisting (which is still being worked on, but looking better now) we'll fall back to interp performance. So the interpreter paths can be performance-sensitive.

The extra stores of JSVAL_VOID or JSVAL_NULL (either's good, possibly NULL is a faster instruction on some architectures) shouldn't hurt as they're async and we have resources on the CPU and in the caches to retire them. Dave Mandelin studied our load/store traffic a bit. We were overwhelming the x86 reservation stations in some cases. There are many causes, so one more might not hurt, but we should not assume.

We would like to adjust the stack pointer as few times as possible. If we could increase it if needed before the op and reduce it if needed after, then some ops would need two adjustments, but many would need only one depending on their stack balance and need for a vp-referenced local root. This seems like something to derive from jsopcode.tbl and js_Interpret via static analysis. Maybe it's hard but I thought I'd raise it.

/be

Comment 25

[Jason Orendorff [:jorendorff]]

We could calculate those sp increments and decrements directly from jsopcode.tbl just using the C preprocessor.  Maybe we should.  The compiler would automatically throw away increments/decrements that amount to zero.  I would have to rewrite everything that uses PUSH and POP, I guess.

Do you think this is worth doing?  I would rather just write it such that I think the compiler *may* consolidate two sp adjustments into one, then look at the assembly and check that it does so.

Comment 26

[Jason Orendorff [:jorendorff]]

In case it's not obvious, the preprocessor trick I was talking about in
comment 25 is:

enum {
#define OPDEF(...) \
    op##_NUSES = nuses, \
    op##_NDEFS = ndefs, \
    op##_NEXTRA = nextra,
#include "jsopcode.tbl"
#undef OPDEF
};

...

#define BEGIN_CASE(op) \
    ... \
    regs.sp += op##_NEXTRA; \
    ...

#define DO_OP(op) \
    ... \
    regs.sp += op##_NDEFS - op##_NUSES - op##_NEXTRA; \
    ...

Comment 27

[Igor Bukanov]

(In reply to comment #26)
> #define DO_OP(op) \
>     ... \
>     regs.sp += op##_NDEFS - op##_NUSES - op##_NEXTRA; \

This does not work: for gc safety the sp should be set to the maximum possible value to cover all the area where GC-roots may leave before the operation bytecode and drop only at the end.

Comment 28

[Brendan Eich [:brendan]]

Jason: agree on letting the C++ compiler optimize in straightforward ways (CSE, constant prop., etc.).

To Igor's point, can we use the jsopcode.tbl-generated deltas to compute the maximum stack adjustment to do before the opcode body, to root everything, and then of course DO_OP adjusts for the net stack balance?

/be

Comment 29

[Jason Orendorff [:jorendorff]]

OK, to recap, the nominees are:

1. Automate sp adjustments as described in comment 26, as amended by #28. This will either cost a store, to null out the extra slot; or make GC a bit less precise, particularly in cases where the extra slot is seldom used (and the leftover value there could be anything).

2. Fiddle with sp around each place that needs an extra slot. I went so far as to partly implement this. The pattern is:

>-            if (!js_NativeSet(cx, obj, sprop, vp))                          \
>+            PUSH_OPND(JSVAL_VOID); /* extra root */                         \
>+            ok = js_NativeSet(cx, obj, sprop, &regs.sp[-1]);                \
>+            *(vp) = POP_OPND();                                             \
>+            if (!ok)                                                        \ 
>                 goto error;                                                 \
>         }                                                                   \

I thought this would be easier to get done quickly. I'm having second thoughts.

Comment 30

[Jason Orendorff [:jorendorff]]

Attachment: WIP 3

Still ambivalent about this approach, but some sort of fix has to go in.  I need to measure the performance impact.

Comment 31

[Brendan Eich [:brendan]]

Stealing con permiso.

/be

Comment 32

[Brendan Eich [:brendan]]

I failed as owner, although as a thief I stole this bug very efficiently. It needs a better owner. Igor, Jason, Gregor come to mind. Anyone?

dmandelin and I were talking about the deeper issue that we lack a static analysis for exact GC root safety. Even with conservative stack scanning we could use some help. David thought that perhaps Taras gave up cuz the analysis was inter-procedural and that seemed out of reach at the time, but since then dwitte and others have done some work. Can we get an analysis?

/be

Comment 33

[(dormant account)]

(In reply to comment #32)
> I failed as owner, although as a thief I stole this bug very efficiently. It
> needs a better owner. Igor, Jason, Gregor come to mind. Anyone?
> 
> dmandelin and I were talking about the deeper issue that we lack a static
> analysis for exact GC root safety. Even with conservative stack scanning we
> could use some help. David thought that perhaps Taras gave up cuz the analysis
> was inter-procedural and that seemed out of reach at the time, but since then
> dwitte and others have done some work. Can we get an analysis?
> 

We reduce it down to an intra-procedual analysis and then we still needed to flow values through the heap and that got non-deterministic pretty fast(ie keeping track of how different values are rooted within structs/unions).

Now that we have C++ we can try again by using smart pointers to enforce analysis boundaries better.

Slava did something similar in factor. http://factor-language.blogspot.com/2009/05/factor-vm-ported-to-c.html

Comment 34

[[e-mail]]

I'm hoping to get back to analysis in a couple months - I could look at this then (or perhaps our analysis intern could take a stab, depending on how far in the deep end we drop him!).

Comment 35

[Brendan Eich [:brendan]]

Igor, will you take this bug? Please reassign if so.

The static analysis idea is good too, may already have a separate bug. We need both (can't rely on even C++ alone, I think -- although perhaps with enough effort we could tolerate RAII pattern helpers all over).

Thanks,

/be

Comment 36

[Luke Wagner [:luke]]

Woohoo conservative stack scanning!