108257 - source notes broken for functions compiled via JS_CompileUCFunctionForPrincipals

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Robert Ginda]

Similar to bug 82188, but affects functions compiled via
JS_CompileUCFunctionForPrincipals.  Patch coming up.

Comment 1

[Robert Ginda]

Attachment: proposed patch; testing now

Comment 2

[Robert Ginda]

Attachment: makefile for testcase

Comment 3

[Robert Ginda]

Attachment: jsnotes.c; testcase showing the problem

Comment 4

[Robert Ginda]

save the makefile and testcase somewhere on your disk, edit the makefile to
point to your js/src directory, and make jsnotes.

the testcase compiles the same script via JS_CompileScript and
JS_CompileFunction.  Source notes are dumped for each script, first for
CompileScript, then for CompileFunction.  Unpatched I get this output...

Source notes:
  0:     8 [   8] xdelta  
  1:     8 [   0] newline 
  2:    18 [  10] xdelta  
  3:    18 [   0] newline 
  4:    28 [  10] xdelta  
  5:    28 [   0] newline 

Source notes:
  0:     8 [   8] xdelta  
  1:     8 [   0] newline 
  2:    18 [  10] xdelta  
  3:    18 [   0] newline 
  4:    28 [  10] xdelta  
  5:    28 [   0] newline 
  6:    38 [  10] xdelta  
  7:    38 [   0] setline  lineno 0
Notice the extra 6th and 7th note when CompileFunction is used.  With this
patch, the testcase produces...

Source notes:
  0:     8 [   8] xdelta  
  1:     8 [   0] newline 
  2:    18 [  10] xdelta  
  3:    18 [   0] newline 
  4:    28 [  10] xdelta  
  5:    28 [   0] newline 

Source notes:
  0:     8 [   8] xdelta  
  1:     8 [   0] newline 
  2:    18 [  10] xdelta  
  3:    18 [   0] newline 
  4:    28 [  10] xdelta  
  5:    28 [   0] newline 

Comment 5

[Robert Ginda]

testing on linux/debug shows no regressions.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 56298 [details] [diff] [review]
proposed patch; testing now

How about a comment:

/* Code already emitted in Statements' loop body. */

before the new code.

js_EmitFunctionBody could be inlined now that it's called in only one place in the whole engine.
No worries, I'll clean that later.

Comment and sr=[e-mail]
 -- thanks,
/be

Comment 7

[Brendan Eich [:brendan]]

Also, maybe it's time to JS_ASSERT that we never emit a SRC_SETLINE(0).

/be

Comment 8

[Robert Ginda]

Attachment: patch including brendan's suggestions

Comment 9

[Robert Ginda]

oops, I'll remove the space between JS_ASSERT and (.
shaver, care to r=?

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 56324 [details] [diff] [review]
patch including brendan's suggestions

rginda, please move that ASSERT into UPDATE_LINENO_NOTES -- that's the only
place that could do the bad thing.

/be

Comment 11

[Robert Ginda]

Attachment: third time's a charm

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 56330 [details] [diff] [review]
third time's a charm

Money, baby!

/be

Comment 13

[Mike Shaver (:shaver emeritus)]

Comment on attachment 56330 [details] [diff] [review]
third time's a charm

So money. r=shaver

Comment 14

[Robert Ginda]

cha ching.

Comment 15

[Phil Schwartau]

Verified Fixed using Rob's testcase on a current Linux debug JS shell. 
NOTE: Rob found that the makefile for the test, jsnotes.mak, should be
corrected as follows in order for the test to compile:

The line ending in  -DDEBUG  -I$(JSDIR) jsnotes.c
should end as:      -DDEBUG  -I$(JSLIBDIR) -I$(JSDIR) jsnotes.c


The output of the jsnotes executable is now the same for both
JS_CompileScript and JS_CompileFunction:

Source notes:
  0:     8 [   8] xdelta  
  1:     8 [   0] newline 
  2:    18 [  10] xdelta  
  3:    18 [   0] newline 
  4:    28 [  10] xdelta  
  5:    28 [   0] newline 

Source notes:
  0:     8 [   8] xdelta  
  1:     8 [   0] newline 
  2:    18 [  10] xdelta  
  3:    18 [   0] newline 
  4:    28 [  10] xdelta  
  5:    28 [   0] newline