680228 - Add notes output to -D

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

Attachment: Add try notes to -D output

djf>	Too bad the try opcode doesn't have an offset.
	jorendorff: thanks for your help.
	<jorendorff>	djf: well, there's a source note that gives the offset; we have the information
	<jorendorff>	djf: if you use dis() you'll see it
	<jorendorff>	it is helpfully described as a "while" offset
	<djf>	jorendorff: I'll take a look at that. Think it would be possible to patch the -D code to output that offset the way dis() does?
	<jorendorff>	djf: it would be possible, sure
	<djf>	I'll look into that, too, then.

Comment 1

[David Flanagan [:djf]]

Attachment: proposed patch

This patch modifies the output of try instructions js_Disassemble1 in jsopcode.cpp. When it disassembles a try, it scans the trynotes looking for the matching catch clause. If it finds one, it includes the address of the catch as part of the disassembly, as if try was a branch instruction. 

The idea is to simplify code coverage analysis based on -D output from the JS shell. The try notes are not otherwise available to programs that use the output of the -D flag.

Comment 2

[David Flanagan [:djf]]

Attachment: proposed patch with safer casts

add the casts that jorendorff asked for

Comment 3

[David Flanagan [:djf]]

Attachment: proposed patch

now I check the type of the trynote too, so it doesn't add an offset for try/finally, only for catch.

Comment 4

[Steve Fink [:sfink] [:s:]]

(In reply to David Flanagan from comment #3)
> Created attachment 554219 [details] [diff] [review]
> proposed patch
> 
> now I check the type of the trynote too, so it doesn't add an offset for
> try/finally, only for catch.

The patch looks good, but let me check first whether it gives you what you want:

First, why don't you want an offset for try/finally? try can flow through to finally, so how can you find the correct finally block?

  function f() {
    throw("kaboom");
  }

  try {
    print("reached 1");
    f();
  } finally {
    print("reached 2");
  }

  try {
    print("reached 3");
  } catch(e) {
    print("not reached 4");
  } finally {
    print("reached 5");
  }

Second, does this give you what you need for multiple catch clauses?

  cond = false;
  try {
    throw("kaboom");
  } catch (e1 if cond) {
    print("catch 1");
  } catch (e2) {
    print("catch 2");
  }

(it's a nonstandard extension.)

I have a patch where I added a try notes dump to -D, but I like your approach much better.

Comment 5

[David Flanagan [:djf]]

(In reply to Steve Fink [:sfink] from comment #4)
> (In reply to David Flanagan from comment #3)
> > Created attachment 554219 [details] [diff] [review]
> > proposed patch
> > 
> > now I check the type of the trynote too, so it doesn't add an offset for
> > try/finally, only for catch.
> 
> The patch looks good, but let me check first whether it gives you what you
> want:
> 
> First, why don't you want an offset for try/finally? try can flow through to
> finally, so how can you find the correct finally block?

The finally blocks appear to all be implemented using a gosub instruction, and if I treat that as a conditional branch (and treat retsub like return) then I already have what I need for finally (I think).
 
>   function f() {
>     throw("kaboom");
>   }
> 
>   try {
>     print("reached 1");
>     f();
>   } finally {
>     print("reached 2");
>   }
> 
>   try {
>     print("reached 3");
>   } catch(e) {
>     print("not reached 4");
>   } finally {
>     print("reached 5");
>   }
> 
> Second, does this give you what you need for multiple catch clauses?
> 

I'd forgotten about those.... Maybe I should take the early return out of the for loop and allow it to emit the address of multiple catch clauses?  I guess it depends on how the conditional catch clauses are translated to bytecode.  I'll take a look.

Thanks.  

>   cond = false;
>   try {
>     throw("kaboom");
>   } catch (e1 if cond) {
>     print("catch 1");
>   } catch (e2) {
>     print("catch 2");
>   }
> 
> (it's a nonstandard extension.)
> 
> I have a patch where I added a try notes dump to -D, but I like your
> approach much better.

Comment 6

[David Flanagan [:djf]]

(In reply to David Flanagan from comment #5)
> (In reply to Steve Fink [:sfink] from comment #4)

> > 
> > Second, does this give you what you need for multiple catch clauses?
> > 
> 
> I'd forgotten about those.... Maybe I should take the early return out of
> the for loop and allow it to emit the address of multiple catch clauses?  I
> guess it depends on how the conditional catch clauses are translated to
> bytecode.  I'll take a look.

I haven't looked at the trynotes to see if there are multiple notes for multiple conditional catch clauses, but the -D disassembly indicates that the catch clauses link to each other with gotos, so for coverage analysis, I think I'm good with one offset on the try instruction.

(It looks to me as if there is a different bug with source notes... The first two opcodes (throwing, leaveblock) after the jump from one catch clause to the next are attributed to the line on which the first catch appears rather than being attributed to the second catch, and that makes it appear as if the last line in the body of a catch claues is partially covered when it is either uncovered or fully covered...  But that's an issue for a separate bug report.)

Comment 7

[David Flanagan [:djf]]

Attachment: proposed patch

Comment 8

[David Flanagan [:djf]]

This latest version of the patch is the same change to jsopcode.cpp but also includes a 2-character change to jsdbgapi.cpp to remove the extra newline that is always printed out before "--- END SCRIPT".

I figure a tiny change like that isn't worth a bug report of its own.

Comment 9

[Steve Fink [:sfink] [:s:]]

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

     js_Disassemble(cx, script, true, &sprinter);
-    fprintf(stdout, "%s\n", sprinter.base);
+    fprintf(stdout, "%s", sprinter.base);
     fprintf(stdout, "--- END SCRIPT %s:%d ---\n", script->filename, script->lineno);


Total nit, but can you switch this to fputs(sprinter.base, stdout) now?

Comment 10

[David Flanagan [:djf]]

Attachment: change fprintf(%s) to simpler fputs

fprintf->fputs change requested by sfink in his review

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Congratulations!

http://hg.mozilla.org/mozilla-central/rev/2eb805907da1