SamplingTool is broken.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Gavin Barraclough]

Should probably fix that.

[Gavin Barraclough]

Created attachment 26808 [details]
The patch

[Geoffrey Garen]

Comment on attachment 26808 [details]
The patch

> +        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

[Gavin Barraclough]

fixed in 39993