[webkit-reviews] review granted: [Bug 23390] SamplingTool is broken. : [Attachment 26808] The patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Gavin Barraclough
<barraclough at apple.com>'s request for review:
Bug 23390: SamplingTool is broken.
https://bugs.webkit.org/show_bug.cgi?id=23390

Attachment 26808: The patch
https://bugs.webkit.org/attachment.cgi?id=26808&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
> +	   void samplingToolTrackCodeBlock()
> +	   {
> +#if ENABLE(CODEBLOCK_SAMPLING)
> +#if PLATFORM(X86_64)
> +	       move(ImmPtr(m_interpreter->sampler()->codeBlockSlot()),
X86::ecx);
> +	       storePtr(ImmPtr(m_codeBlock), X86::ecx);
> +#else
> +	       storePtr(ImmPtr(m_codeBlock),
m_interpreter->sampler()->codeBlockSlot());
> +#endif
> +#endif
> +	   }
> +
> +#if ENABLE(OPCODE_SAMPLING)
> +	   void samplingToolTrackCodeBlock(Instruction* instruction, bool
inHostFunction=false)
> +	   {
> +#if PLATFORM(X86_64)
> +	       move(ImmPtr(m_interpreter->sampler()->sampleSlot()), X86::ecx);
> +	      
storePtr(ImmPtr(m_interpreter->sampler()->encodeSample(instruction,
inHostFunction)), X86::ecx);
> +#else
> +	      
storePtr(ImmPtr(m_interpreter->sampler()->encodeSample(instruction,
inHostFunction)), m_interpreter->sampler()->sampleSlot());
> +#endif
> +	   }
> +#else
> +	   void samplingToolTrackCodeBlock(Instruction*, bool) {}
> +#endif

I think it's confusing to have two functions named
"samplingToolTrackCodeBlock", when one records a CodeBlock and the other
records an Instruction and some flags.

How about "sampleCodeBlock" and "sampleInstruction" instead?

(You might also want to pass the CodeBlock* to sampleCodeBlock. It's a bit
subtle that the function automatically supplies the CodeBlock you're currently
compiling. Its real purpose, I think, is just to abstract out 32bit vs 64bit.)

r=me