Adding unit tests for profiler's stack tracer.

Adding unit tests for profiler's stack tracer.

The testing is a bit tricky because we need to obtain a frame
pointer (EBP on IA-32) from inside of a function. This is especially
interesting in case of a compiled JavaScript function.

[Søren Thygesen Gjesse, 2009-02-25 12:34:54]

Starting to add unit tests for the profiler is an excelent idea. However I think
you can simplify the tests, and make them platform independent. The current
approach only works your IA32, and having tests that works on all (both)
platforms is preferred. If this for some reason is not possible there should be
different .cc files like for test-assembler-*.

Instead of using code patching to get the FP how about adding a runtime function
Runtime_GetFP to return the FP of the calling JS function like
Runtime_DebugPrint where the FP of the caller is printed? This could then be
called with %GetFP in the test function.

On 2009/02/25 10:42:32, Mikhail Naganov wrote:
>

[Søren Thygesen Gjesse, 2009-02-25 12:36:13]

Missed the inline comments.

http://codereview.chromium.org/28112/diff/1/3
File src/log.h (right):

http://codereview.chromium.org/28112/diff/1/3#newcode276
Line 276: class StackTracer {
Please add BASE_EMBEDDED.

http://codereview.chromium.org/28112/diff/1/5
File test/cctest/test-log-ia32.cc (right):

http://codereview.chromium.org/28112/diff/1/5#newcode141
Line 141: static Handle<JSFunction> Compile(const char* source) {
For compiling JavaScript I think you should stick to using the public API when
possible instead of using internal functions, e.g. CompileFunction in
test-debug.cc. You can always later convert API objects to their internal
counterparts when required.

http://codereview.chromium.org/28112/diff/1/5#newcode150
Line 150: static void CompileRun(const char* source) {
Again use the public API for this.

http://codereview.chromium.org/28112/diff/1/5#newcode208
Line 208: original, patch, sizeof(patch)));
This code patching seems risky and changes to the compiler might break it. How
about adding a runtime function to return the FP of the caller?

http://codereview.chromium.org/28112/diff/1/5#newcode210
Line 210: SetGlobalProperty("JSFuncDoTrace", *call_trace);
Look at CompileFunction in test-debug.cc to compile 'function JSFuncDOTrace() {
... }'.

http://codereview.chromium.org/28112/diff/1/5#newcode212
Line 212: SetGlobalProperty("sample", Smi::FromInt(PtrToSmi(&sample)));
I don't see the need to store these values in the JS global object. Can't they
just be global variables in C++? They are only used from C++ code in this file.

[Mikhail Naganov, 2009-02-25 15:40:38]

http://codereview.chromium.org/28112/diff/1/3
File src/log.h (right):

http://codereview.chromium.org/28112/diff/1/3#newcode276
Line 276: class StackTracer {
On 2009/02/25 12:36:13, Søren Gjesse wrote:
> Please add BASE_EMBEDDED.

Done.

http://codereview.chromium.org/28112/diff/1/5
File test/cctest/test-log-ia32.cc (right):

http://codereview.chromium.org/28112/diff/1/5#newcode141
Line 141: static Handle<JSFunction> Compile(const char* source) {
On 2009/02/25 12:36:13, Søren Gjesse wrote:
> For compiling JavaScript I think you should stick to using the public API when
> possible instead of using internal functions, e.g. CompileFunction in
> test-debug.cc. You can always later convert API objects to their internal
> counterparts when required.

Thanks to your suggestion I removed dozen lines of code. I simply didn't knew
about OpenHandle / ToApi utilities before.

http://codereview.chromium.org/28112/diff/1/5#newcode150
Line 150: static void CompileRun(const char* source) {
On 2009/02/25 12:36:13, Søren Gjesse wrote:
> Again use the public API for this.

Done.

http://codereview.chromium.org/28112/diff/1/5#newcode208
Line 208: original, patch, sizeof(patch)));
On 2009/02/25 12:36:13, Søren Gjesse wrote:
> This code patching seems risky and changes to the compiler might break it. How
> about adding a runtime function to return the FP of the caller?

OK, maybe we will get rid of this in the next iteration. E.g. I didn't know
about inline runtime functions.

http://codereview.chromium.org/28112/diff/1/5#newcode210
Line 210: SetGlobalProperty("JSFuncDoTrace", *call_trace);
On 2009/02/25 12:36:13, Søren Gjesse wrote:
> Look at CompileFunction in test-debug.cc to compile 'function JSFuncDOTrace()
{
> ... }'.
> 

I tried this already, but because of lazy compilation, I haven't got a real
compiled code but instead a stub that calls LazyCompile. And running the
function prior to patching is not an option here.

http://codereview.chromium.org/28112/diff/1/5#newcode212
Line 212: SetGlobalProperty("sample", Smi::FromInt(PtrToSmi(&sample)));
On 2009/02/25 12:36:13, Søren Gjesse wrote:
> I don't see the need to store these values in the JS global object. Can't they
> just be global variables in C++? They are only used from C++ code in this
file.

Good idea! Done.