454315 - TM: incorrect results for raytrace due to undefined passed for missing arguments

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Robert Sayre]

My correctness check for this sunspider test was incorrect. :(

Comment 1

[Robert Sayre]

Attachment: js file that checks its result

works in JSC, v8, and SM with no JIT

Comment 2

[Robert Sayre]

correct.sh in the TM repo will check for this now

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Any idea what is actually wrong here?

Brendan was hoping to use vlad's parallel raytrace example next Monday, but the example sorta sucks at the moment due to bug 454435. Jonas thinks that it'll work great with tracing enabled since we won't allocate all those doubles any longer.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Seems there's a mishandling of NaNs here, the interpreter is taking one path and the JIT is taking a different (incorrect) one.  triangle.intersect's (t > far) check is breaking.  Will continue to look into this.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

So what happens is that we compile triangle.intersect with "far" as an integer variable.  When we see an "undefined" in ValueToNative() and expect "far" to be an integer, we assume we can plop an integer 0 into that slot, despite an undefined having different semantics.

Marking this as a blocker for the meta-bug about undefined types on trace entry.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: breaking case

Example of breaking case attached, returns different output with the JIT on.  Forcing ValueToNative() to return false (and reject the trace) for type=JSVAL_INT,v=JSVAL_VOID generates correct answers.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: proposed patch to fix undefined->0

Here's an attempt at fixing this.  A few notes:

- I didn't pack the defUseMap bits into the stackTypeMap array -- yet.  If that's really preferred I can post another patch, I just want to make sure the logic is correct first.

- snapshot() calls get().  I introduced peek() which get() now calls instead, so values won't accidentally get marked as "used before defined" there.

- We win less on raytrace now.  Perf went from 1.6X the interpreter to 1X.  It hurts that we have to abort the tree call.  OTOH the results are now correct.  Multiple trees per entry point would really help here.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Correction on comment: peek() which determineSlotType() now calls instead.

Comment 9

[Brendan Eich [:brendan]]

I'll review this later today (GMT+1).

/be

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 338184 [details] [diff] [review]
proposed patch to fix undefined->0

>-BuildNativeStackFrame(JSContext* cx, unsigned callDepth, uint8* mp, double* np)
>+BuildNativeStackFrame(JSContext* cx, unsigned callDepth, uint8* mp, double* np, uint8* defUseMap)
> {
>     debug_only_v(printf("stack: ");)
>     FORALL_SLOTS_IN_PENDING_FRAMES(cx, callDepth,
>         debug_only_v(printf("%s%u=", vpname, vpnum);)
>-        if (!ValueToNative(cx, *vp, *mp, np))
>-            return false;
>-        ++mp; ++np;
>+        if (!ValueToNative(cx, *vp, *mp, np, (*defUseMap == JSTRACER_VAL_USE_BEFORE_DEF)))

Can't this index out of bounds in defUseMap, which covers only the entry stack frame?

I have a patch, have to get it out of my mq, that uses singleton void_ins (and notset_ins, but that's not needed to do what your patch does) to detect use before set. There's no need to have a separate map with its attendant out-of-bounds indexing hazard. My patch does add a JSVAL_NOTUSED flag to the typemap entry. More when I can get and refresh this patch.

/be

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Hrm, BuildNativeStackFrame is only ever called from js_ExecuteTree with entryTypeMap, so it should be sane.

I'll hold off if your patch is going to do the same thing.

Comment 12

[Brendan Eich [:brendan]]

(In reply to comment #11)
> Hrm, BuildNativeStackFrame is only ever called from js_ExecuteTree with
> entryTypeMap, so it should be sane.

My apologies -- the unwanted callDepth parameter misled me. If it's always 0 then we should get rid of it, but if it could be non-zero we have trouble right here in River City, with or without your patch.

> I'll hold off if your patch is going to do the same thing.

Thanks. I will get the patch in shape.

One tricky thing about it: guards currently snapshot their entire stack's type state, including the entry frame, in separate memory per guard.

So any tree calling based on such a guard-generated typemap will not see the JSVAL_NOTUSED flag or whatever typemap entry mutations might be made to the entry frame's typemap. Not sure this matters, have to remind myself of what I found over the weekend hacking that patch.

/be

Comment 13

[Brendan Eich [:brendan]]

Attachment: alterna-patch, uses singleton void_ins to detect used-before-set

And stores the USED_BEFORE_SET flag in treeInfo->stackTypeMap.

David, if you could test this to ensure it works like your patch does that would be most appreciated. I've got a bunch of other bugs in my mq but I can take this bug if you want -- or if you take the patch and land it with r=you, even better! ;-)

/be

Comment 14

[Andreas Gal :gal]

Is this going to work with branches that get attached to the tree? And inner trees?

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 339847 [details] [diff] [review]
alterna-patch, uses singleton void_ins to detect used-before-set

Also avoids adding single-use TraceRecorder::peek via direct tracker.get(vp) call in determineSlotType.

/be

Comment 16

[Brendan Eich [:brendan]]

(In reply to comment #14)
> Is this going to work with branches that get attached to the tree? And inner
> trees?

for (...) {
  var maybe_ubs;
  for (...) {
    if (metastable())
      maybe_ubs = 42;
    else
      sink = maybe_ubs;
    ...
  }
  if (metastable2())
    maybe_ubs = 43;
  else
    sink2 = maybe_ubs;
}

Alternatively, make maybe_ubs a formal parameter to a function f wrapping the above. That's more like 3d-raytrace.js, if a recorded call passes an actual for maybe_ubs but a later call does not pass any actual for the formal parameter.

We can't be sure that a parameter or variable is USED_BEFORE_SET based on this kind of dynamic analysis. We only know that if it appears to be, we must abort rather than import undefined as int 0. But if we trace a path where a variable is set before use, then we have to trash the tree if we attach a trace that does not see set before use and therefore flips USED_BEFORE_SET.

The set before use in the first trace could be downstream of a guard at which the secondary trace (that uses before setting) attaches.

So yeah, more work needed.

/be

Comment 17

[Brendan Eich [:brendan]]

Attachment: revised alterna-patch, works

David, here's that patch based on yours that uses the typemap spare bits instead of a separate defUseMap.

/be

Comment 18

[Brendan Eich [:brendan]]

This patch should go in for b1 and come out when bug 450833 is fixed.

/be

Comment 19

[Andreas Gal :gal]

Comment on attachment 339897 [details] [diff] [review]
revised alterna-patch, works

I want to get rid of this once we have multi-trees in so please link this bug to that bug once this is pushed. This is not safe in the presence of inner trees.

Comment 20

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: fixes against last patch

Fixes a few missing JSVAL_TAGs causing regressions in trace-tests.js.

Comment 21

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: experimental version to rebuild the tree

With the correctness fix we go from 2.15X -> 1.00X.  I'm trying to play around with ways to win this back though when push comes to shove correctness should win.

I hacked BuildNativeStackFrame/ValueToNative to trash the tree and mark the slot as undemotable, so we recompile the tree instead of blacklisting it.

That got us 1.12X.  I think I'm grasping at straws until we get multi-trees though.

Comment 22

[David Anderson [:dvander] - inactive, e-mail if emergency]

Going to hold off until we can get multitrees working, marking for beta 2.

Comment 23

[David Anderson [:dvander] - inactive, e-mail if emergency]

Added test case as changeset http://hg.mozilla.org/tracemonkey/rev/10ea4db73d63

Fixed now that bug 450833 landed; new bug 462247 about making raytrace faster.

Comment 24

[Bob Clary [:bc] (inactive)]

test already in js1_8_1/trace/trace-test.js

Comment 25

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2