727955 - Support shark jit profiling via DL trick.

Status: RESOLVED FIXED

(Tamarin Graveyard :: Profiler, defect, P2)

Description

[Edwin Smith]

Its possible to trick Shark into thinking jitcode came from a DL.

Steps:

1. dlopen() an empty DL with (say) 64MB of space, mark it RWX.
2. hack CodeAlloc to use this region.
3. run a test, generate log of jit function names, start/end address, asm, of each jit'd function
4. turn log into .s, compile into new .so
5. in shark, symbolicate using this new .so

In principle this should work with linux/oprofile as well.

Comment 1

[Edwin Smith]

Attachment: Adds DL-based jit profiling instrumentation

Comment 2

[Edwin Smith]

Note, I haven't split the APCS changes into a separate patch yet.  Everything else should be about ready.

Comment 3

[William Maddox]

Comment on attachment 597977 [details] [diff] [review]
Adds DL-based jit profiling instrumentation

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

R+.  I wouldn't hold up landing this over any of the issues noted, but there are a few things that should be fixed.
See detailed comments.  Definitely a feature worth having.  I suspect that the JitObserver mechanism will ultimately
be generalized and merged with deopt metadata support in Halfmoon.

::: core/CodegenLIR.cpp
@@ +2720,5 @@
> +                  LIns* debugger = loadIns(LIR_ldp, offsetof(AvmCore, _debugger),
> +                                           coreAddr, ACCSET_OTHER, LOAD_CONST);
> +                  callIns(FUNCTIONID(debugFile), 2,
> +                          debugger,
> +                          InsConstPtr((String*)filename));

The cast is not needed here.

@@ +2726,5 @@
> +#endif // DEBUGGER
> +#ifdef VMCFG_VTUNE
> +              Ins(LIR_file, InsConstPtr(filename));
> +#endif /* VMCFG_VTUNE */
> +              if (jit_observer) {

The JitObserver support is not conditionalized on VMCFG_SHARK.  This may be intentional, as it could be used elsewhere.  We seem to be a bit inconsistent, however, about where we leave in runtime tests and where we completely configure out a diagnostic mechanism.  Do we anticipate other JIT observers?  I'm thinking we might merge this with the metadata collection for deopt, so perhaps it should be seen as more general than just support for VMCFG_SHARK.

::: core/LirHelper.cpp
@@ +491,5 @@
> +            AVMPI_freeCodeMemory(addr, nbytes);
> +        }
> +    }
> +
> +    void CodeAlloc::markCodeChunkExec(void* /*addr*/, size_t /*nbytes*/) {

We usually write the arguments, then void them in the body.  I prefer that style, but no big deal.

::: core/exec-jit.cpp
@@ +806,5 @@
> +{
> +#if defined( ANDROID )
> +    char path[256];
> +//      snprintf(path, sizeof(path)-1, "/sdcard/%d-jit.log", getpid());
> +    snprintf(path, sizeof(path)-1, "/data/data/air.ShoppingList/%d-jit.log", getpid());

Need a comment here.  Why is the previous line commented out?  What is "air.ShoppingList"?

::: nanojit/Assembler.h
@@ +169,4 @@
>          return n;
>      }
>  
> +#define VMCFG_APCS 1

This is also defined in NativeARM.cpp.  This is not the right place for this.
Also, do we want to enable this unconditionally, or just on VMCFG_SHARK?

::: utils/sharkprof/README.txt
@@ +7,5 @@
> +allocate code from.
> +
> +I've modified Scott's original code so that if the function entry point occurs within a block,
> +the block is trimmed so that disassembly will start at the function entry, not the physical
> +stat of the code block.

I think this is something that I wrote, and it looks like you've dropped
the hack I added to implement this.

::: utils/sharkprof/log2s.cpp
@@ +94,5 @@
> +
> +    std::string file() const
> +    {
> +        if (tag() != 'file'
> +            )

Should be on previous line.  See other misplaced close parens below.

@@ +130,5 @@
> +{
> +    const Chunk *meth;
> +    const Chunk *block;
> +    const Chunk *code;
> +    const Chunk *mstrt; //###

Should remove the hacky change markers.

@@ +227,5 @@
> +            break;
> +            //###
> +        case 'STRT':
> +            strt = &chunk;
> +            break;

You dropped the code to emit these in the log.

Comment 4

[Edwin Smith]

(In reply to William Maddox from comment #3)

> I suspect that
> the JitObserver mechanism will ultimately
> be generalized and merged with deopt metadata support in Halfmoon.

Yes, and there is definitely overlap between the purpose of LIR_file + LIR_line
(only used by vtune) and LIR_pc (only used by shark).  Both exist for the same
reason.

> ::: core/CodegenLIR.cpp
> @@ +2720,5 @@
> > +                  LIns* debugger = loadIns(LIR_ldp, offsetof(AvmCore, _debugger),
> > +                                           coreAddr, ACCSET_OTHER, LOAD_CONST);
> > +                  callIns(FUNCTIONID(debugFile), 2,
> > +                          debugger,
> > +                          InsConstPtr((String*)filename));
> 
> The cast is not needed here.

fixed.

> @@ +2726,5 @@
> > +#endif // DEBUGGER
> > +#ifdef VMCFG_VTUNE
> > +              Ins(LIR_file, InsConstPtr(filename));
> > +#endif /* VMCFG_VTUNE */
> > +              if (jit_observer) {
> 
> The JitObserver support is not conditionalized on VMCFG_SHARK.  This may be
> intentional, as it could be used elsewhere.  We seem to be a bit
> inconsistent, however, about where we leave in runtime tests and where we
> completely configure out a diagnostic mechanism.  Do we anticipate other JIT
> observers?  I'm thinking we might merge this with the metadata collection
> for deopt, so perhaps it should be seen as more general than just support
> for VMCFG_SHARK.

I intentionally followed a "dont #ifdef unless absolutely necessary" style, so yeah
it might be inconsistent but I only care if its a real problem (code size, speed,
portability).

For this patch my intention was only to #ifdef VMCFG_SHARK around the 
shark logging specific patches.  We could even make those unconditional
(or at least, mac-specific) if we changed CodeAlloc.allocChunk (etc) to virtual
calls, so we could support different code allocators based on a runtime flag.
[although, the logging + RW code + LD trick could be a security vuln when
enabled, so I dont mind the #ifdef for that reason].

> ::: core/exec-jit.cpp
> @@ +806,5 @@
> > +{
> > +#if defined( ANDROID )
> > +    char path[256];
> > +//      snprintf(path, sizeof(path)-1, "/sdcard/%d-jit.log", getpid());
> > +    snprintf(path, sizeof(path)-1, "/data/data/air.ShoppingList/%d-jit.log", getpid());
> 
> Need a comment here.  Why is the previous line commented out?  What is
> "air.ShoppingList"?

I have no idea.  I'm tempted to completely rip out the android/arm code,
since it's totally untested.  Maybe I will, then attach it to a different bug.

> > +#define VMCFG_APCS 1
> 
> This is also defined in NativeARM.cpp.  This is not the right place for this.
> Also, do we want to enable this unconditionally, or just on VMCFG_SHARK?

this should be VMCFG_OPROFILE probably, see above - i vote to strip it for now.

> I think this is something that I wrote, and it looks like you've dropped
> the hack I added to implement this.

Cleaned up.

> Should remove the hacky change markers.
> You dropped the code to emit these in the log.

stripped STRT tag support and the mstrt field.

Comment 5

[Tamarin Bot]

changeset: 7242:c57eb8acfee0
user:      Edwin Smith <[e-mail]>
summary:   Bug 727955 - Support shark jit profiling via DL trick (r=wmaddox+)

http://hg.mozilla.org/tamarin-redux/rev/c57eb8acfee0