Add notes output to -D

RESOLVED FIXED in mozilla9

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sfink, Assigned: djf)

Tracking

unspecified
mozilla9
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
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.
(Assignee)

Comment 2

6 years ago
Created attachment 554215 [details] [diff] [review]
proposed patch with safer casts

add the casts that jorendorff asked for
Attachment #554214 - Attachment is obsolete: true
Attachment #554215 - Flags: review?(sphink)
(Assignee)

Comment 3

6 years ago
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.
Attachment #554215 - Attachment is obsolete: true
Attachment #554215 - Flags: review?(sphink)
Attachment #554219 - Flags: review?(sphink)
(Assignee)

Updated

6 years ago
Assignee: general → dflanagan
OS: Linux → All
Hardware: x86_64 → All
(Reporter)

Comment 4

6 years ago
(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.
(Assignee)

Comment 5

6 years ago
(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.
(Assignee)

Comment 6

6 years ago
(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.)
(Assignee)

Comment 7

6 years ago
Created attachment 554306 [details] [diff] [review]
proposed patch
(Assignee)

Comment 8

6 years ago
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.
(Reporter)

Updated

6 years ago
Attachment #554219 - Flags: review?(sphink)
(Reporter)

Comment 9

6 years ago
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?
Attachment #554306 - Flags: review+
(Reporter)

Updated

6 years ago
Attachment #554219 - Attachment is obsolete: true
(Assignee)

Comment 10

6 years ago
Created attachment 554518 [details] [diff] [review]
change fprintf(%s) to simpler fputs

fprintf->fputs change requested by sfink in his review
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: [inbound]
Congratulations!

http://hg.mozilla.org/mozilla-central/rev/2eb805907da1
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.