645111 - Add call frame info to Jaeger* inline assembly routines

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: Insert .cfi directives into inline asm

While attempting to clean up my patch for bug 642320, I tackled properly adding call frame information to JaegerTrampoline and friends. (I was generating my own when generating debug info for the JITted code and glomming them together. Which is a huge waste because you repeat the same stuff for every function compiled, and ugly besides.)

It turns out to be extremely simple through .cfi_* asm directives, far simpler than generating it by hand. And it can be committed independently of bug 642320, doesn't need to be protected by build flags, etc.

It doesn't get a whole lot of the gain, either, since it just fixes up the situation when you have a trampoline of some sort on the stack (and they don't call much, so usually that means you're executing the trampoline itself). So if you're executing within the body of JITted code, this is useless. It's still good for completeness.

It'll need to be adjusted when we free up ebp on x86, of course.

This also only uses the C++ stack. So JS stack frames are all collapsed into a single entry. I hope to incorporate the JS stack (VMFrames, that is) if that's possible with the way gdb wants it, which it looks like it should be.

Comment 1

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

Comment on attachment 521884 [details] [diff] [review]
Insert .cfi directives into inline asm

Looks like integrating the JS stack is trickier than I thought, so requesting review for the straightforward thing here.

Comment 2

[Jim Blandy :jimb]

Steve, to help me review this, could you run 'readelf -s -wfF' on the .o file, and attach it to the bug?

Comment 3

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

Attachment: Insert .cfi directives into inline asm

Unbitrotted. (Kind of surprised it was necessary. Maybe the capitalization change from entryFp -> entryfp?)

I will attach what you requested in a second, though you'll need more than that.

Comment 4

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

Attachment: output of readelf -s --debug-dump=frames MethodJIT.o

My readelf does not accept -wfF nor the equivalent --debug-dump=frames-interp, and its command-line processing is horribly mangled. Here's the output of

  readelf -s --debug-dump=frames MethodJIT.o

which still isn't of much use because JaegerTrampoline and friends do not appear in the symbol table, so you can't tell which FDE corresponds to them. Using

% nm MethodJIT.o | fgrep JaegerT
0000000000000538 T JaegerThrowpoline
00000000000004d1 T JaegerTrampoline
0000000000000513 T JaegerTrampolineReturn

and dwarfdump, I can tell you the FDE offsets:

JaegerTrampoline: fde offset 0x12a8
JaegerThrowpoline: fde offset 0x1320
JaegerTrampolineReturn: fde offset 0x1300

Here are the relevant excerpts from the dump file:

DWARF section [397] '.debug_frame' at offset 0xcf730:

 [     0] CIE length=20
   CIE_id:                   18446744073709551615
   version:                  1
   augmentation:             ""
   code_alignment_factor:    1
   data_alignment_factor:    -8
   return_address_register:  16

   Program:
     def_cfa r7 (reg7) at offset 8
     offset r16 (reg16) at cfa-8
     nop
     nop
     nop
     nop
     nop
     nop

 [  12a8] FDE length=44 cie=[     0]
   CIE_pointer:              0
   initial_location:         0
   address_range:            0x42

   Program:
     advance_loc 1 to 0x1
     def_cfa_offset 16
     offset r6 (reg6) at cfa-16
     advance_loc 3 to 0x4
     def_cfa_register r6 (reg6)
     advance_loc 9 to 0xd
     offset r12 (reg12) at cfa-24
     offset r13 (reg13) at cfa-32
     offset r14 (reg14) at cfa-40
     offset r15 (reg15) at cfa-48
     offset r3 (reg3) at cfa-56
     nop
     nop
     nop
     nop
     nop

 [  1320] FDE length=28 cie=[     0]
   CIE_pointer:              0
   initial_location:         0
   address_range:            0x29

   Program:
     advance_loc 37 to 0x25
     def_cfa_register r7 (reg7)
     def_cfa_offset 8
     nop
     nop
     nop

 [  1300] FDE length=28 cie=[     0]
   CIE_pointer:              0
   initial_location:         0
   address_range:            0x25

   Program:
     advance_loc 29 to 0x1d
     def_cfa_register r7 (reg7)
     def_cfa_offset 8
     nop
     nop
     nop

or if you prefer dwarfdump's format, which is pretty much just a dump of the resulting table:

<  1><0x4d1:0x513><><fde offset 0x12a8 length: 0x2c><eh offset none>
    0x000004d1: <off cfa=08(r7) > <off r16=-8(cfa) > 
    0x000004d2: <off cfa=16(r7) > <off r6=-16(cfa) > <off r16=-8(cfa) > 
    0x000004d5: <off cfa=16(r6) > <off r6=-16(cfa) > <off r16=-8(cfa) > 
    0x000004de: <off cfa=16(r6) > <off r3=-56(cfa) > <off r6=-16(cfa) > <off r12=-24(cfa) > <off r13=-32(cfa) > <off r14=-40(cfa) > <off r15=-48(cfa) > <off r16=-8(cfa) > 
<  2><0x513:0x538><><fde offset 0x1300 length: 0x1c><eh offset none>
    0x00000513: <off cfa=16(r6) > <off r3=-56(cfa) > <off r6=-16(cfa) > <off r12=-24(cfa) > <off r13=-32(cfa) > <off r14=-40(cfa) > <off r15=-48(cfa) > <off r16=-8(cfa) > 
    0x00000530: <off cfa=08(r7) > <off r3=-56(cfa) > <off r6=-16(cfa) > <off r12=-24(cfa) > <off r13=-32(cfa) > <off r14=-40(cfa) > <off r15=-48(cfa) > <off r16=-8(cfa) > 
<  2><0x538:0x561><><fde offset 0x1320 length: 0x1c><eh offset none>
    0x00000538: <off cfa=16(r6) > <off r3=-56(cfa) > <off r6=-16(cfa) > <off r12=-24(cfa) > <off r13=-32(cfa) > <off r14=-40(cfa) > <off r15=-48(cfa) > <off r16=-8(cfa) > 
    0x0000055d: <off cfa=08(r7) > <off r3=-56(cfa) > <off r6=-16(cfa) > <off r12=-24(cfa) > <off r13=-32(cfa) > <off r14=-40(cfa) > <off r15=-48(cfa) > <off r16=-8(cfa) > 

Note that I attempted to do the x86 cfi info too, but haven't tested it. And I haven't done ARM.

I tested the x86_64 stuff by singlestepping through JaegerTrampoline and JaegerTrampolineReturn and verifying that I could get a call stack.

Comment 5

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

Attachment: objdump output

Comment 6

[Jim Blandy :jimb]

Comment on attachment 525515 [details] [diff] [review]
Insert .cfi directives into inline asm

Review of attachment 525515 [details] [diff] [review]:
-----------------------------------------------------------------

Yay!

::: js/src/methodjit/MethodJIT.cpp
@@ +272,5 @@
>      "popq %r14"                          "\n"
>      "popq %r13"                          "\n"
>      "popq %r12"                          "\n"
>      "popq %rbp"                          "\n"
> +    ".cfi_def_cfa_register rsp"          "\n"

For spots like this, I think you can just say: '.cfi_def_cfa REGISTER, OFFSET' and reset them both at the same time, with a single DWARF opcode.

Comment 7

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

Attachment: Insert .cfi directives into inline asm

Updated to use .cfi_def_cfa as per jimb's comment. Carrying forward the r+.

Note that there is now a new chunk of native code, JaegerInterpoline, which does not yet have CFI info. I'll open a separate bug for that, since I'm not planning on doing it immediately.

Comment 8

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

Attachment: Insert .cfi directives into inline asm

Patch bounced (giving a perfect example of why I should always use the try server first.)

The main problem is that OSX still has gcc-4.2 (though I think it's the gas version that matters), which doesn't recognize .cfi directives. So I commented them out unless #defined __GCC_HAVE_DWARF2_CFI_ASM, which appears to be the thing to use. (I don't know what the equivalent of .cfi is for MSVC.)

I also attempted to remove my use of ".cfi_sections .debug_frame" at the top, because I was only doing that to save space in the binary. But it's a trivial amount of space, and it looks like we're probably compiling with options that discard .eh_frame. Or at least, when I tried removing the .cfi_sections directive, gdb was no longer able to unwind past JaegerTrampoline. (-fno-exceptions doesn't seem like it ought to be enough to kill .eh_frame, as per the discussion in http://gcc.gnu.org/bugzilla/show_bug.cgi?id=43232 which says you'd also need -fno-asynchronous-unwind-tables and even that might not work. But at least in my gcc 4.5.1, *something* seems to be eating it. Or, more likely, using .debug_frame for automatic CFI and .eh_frame from manual CFI, then dropping .eh_frame because it's believed to be unnecessary.)

Finally, just to be annoying, I tightened up the random register info -- instead of

   push r12
   push r13
   push r14
   .cfi for r12
   .cfi for r13
   .cfi for r14

I switched to

   push r12
   .cfi for r12
   push r13
   .cfi for r13
   push r14
   .cfi for r14

Try server hasn't complained yet... https://tbpl.mozilla.org/?tree=Try&rev=a7406566a82c

Comment 9

[Jim Blandy :jimb]

Comment on attachment 563938 [details] [diff] [review]
Insert .cfi directives into inline asm

Review of attachment 563938 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, with the x86_64 bits fixed.

A five-month review turnaround is not something I'm proud of. Thanks for being patient.

::: js/src/methodjit/MethodJIT.cpp
@@ +220,5 @@
>  SYMBOL_STRING(JaegerTrampoline) ":"       "\n"
>      /* Prologue. */
> +    CFI(".cfi_startproc"                 "\n")
> +    CFI(".cfi_def_cfa_register rsp"      "\n")
> +    CFI(".cfi_def_cfa_offset 8"          "\n")

It seems like you didn't do the .cfi_def_cfa simplification for the x86_64 code.

Comment 10

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

Attachment: Insert .cfi directives into inline asm

Carrying forward jimb's r+.

jimb: I have a basically good try run for this patch + a couple of others, but I see you have a version with the interpoline handled. Would you rather I wait and push the combined patch, or should I push what I have already?

The try runs are

https://tbpl.mozilla.org/?tree=Try&rev=db00b5ed70c0
https://tbpl.mozilla.org/?tree=Try&rev=c973bc26a38c

The 2nd there is the original, the 1st is rebased against mozilla-inbound. I list both because the 1st shows some scary errors that appears to be noise, and the combined picture is happier.

Comment 11

[Jim Blandy :jimb]

My patch is on top of yours. So go ahead and land yours.

Comment 12

[Jim Blandy :jimb]

Don't know how the 'confidental' flag got ticked. Removing.

Comment 13

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d2816c9cd41

Comment 14

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/6d2816c9cd41