Last Comment Bug 680228 - Add notes output to -D
: Add notes output to -D
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla9
Assigned To: David Flanagan [:djf]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-18 13:19 PDT by Steve Fink [:sfink] [:s:]
Modified: 2011-08-21 11:40 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add try notes to -D output (1.44 KB, patch)
2011-08-18 13:19 PDT, Steve Fink [:sfink] [:s:]
no flags Details | Diff | Review
proposed patch (988 bytes, patch)
2011-08-18 14:46 PDT, David Flanagan [:djf]
no flags Details | Diff | Review
proposed patch with safer casts (1.05 KB, patch)
2011-08-18 15:01 PDT, David Flanagan [:djf]
no flags Details | Diff | Review
proposed patch (1.08 KB, patch)
2011-08-18 15:09 PDT, David Flanagan [:djf]
no flags Details | Diff | Review
proposed patch (1.56 KB, patch)
2011-08-18 22:30 PDT, David Flanagan [:djf]
sphink: review+
Details | Diff | Review
change fprintf(%s) to simpler fputs (1.55 KB, patch)
2011-08-19 13:05 PDT, David Flanagan [:djf]
no flags Details | Diff | Review

Description Steve Fink [:sfink] [:s:] 2011-08-18 13:19:29 PDT
Created attachment 554186 [details] [diff] [review]
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] 2011-08-18 14:46:09 PDT
Created attachment 554214 [details] [diff] [review]
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] 2011-08-18 15:01:01 PDT
Created attachment 554215 [details] [diff] [review]
proposed patch with safer casts

add the casts that jorendorff asked for
Comment 3 David Flanagan [:djf] 2011-08-18 15:09:18 PDT
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.
Comment 4 Steve Fink [:sfink] [:s:] 2011-08-18 16:41:35 PDT
(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] 2011-08-18 19:33:03 PDT
(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] 2011-08-18 19:54:31 PDT
(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] 2011-08-18 22:30:18 PDT
Created attachment 554306 [details] [diff] [review]
proposed patch
Comment 8 David Flanagan [:djf] 2011-08-18 22:31:58 PDT
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:] 2011-08-19 12:57:50 PDT
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] 2011-08-19 13:05:51 PDT
Created attachment 554518 [details] [diff] [review]
change fprintf(%s) to simpler fputs

fprintf->fputs change requested by sfink in his review
Comment 11 :Ms2ger 2011-08-21 11:40:03 PDT
Congratulations!

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

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