Stop creating dummy call frames when calling out to C functions

Stop creating dummy call frames when calling out to C functions

R=asiva@google.com, regis@google.com

Committed: https://code.google.com/p/dart/source/detail?r=33122

[regis, 2014-02-26 23:53:12]

LGTM, but how about MIPS and ARM?

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/constants_ia32.h
File runtime/vm/constants_ia32.h (right):

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/constants_ia3...
runtime/vm/constants_ia32.h:64: const Register CALLEE_SAVED = EDI;  // Alias for
a callee saved register.
I do not think this constant helps. Just use EDI where you need it as a callee
saved and add a comment.

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/constants_x64.h
File runtime/vm/constants_x64.h (right):

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/constants_x64...
runtime/vm/constants_x64.h:88: const Register CALLEE_SAVED = R13;  // Alias for
a callee saved register.
ditto

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:3988: summary->set_temp(0,
Location::RegisterLocation(CALLEE_SAVED));
CALLEE_SAVED -> EDI));  // Callee saved.

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:3987: summary->set_temp(0,
Location::RegisterLocation(CALLEE_SAVED));
ditto

[siva, 2014-02-27 00:00:26]

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/constants_ia32.h
File runtime/vm/constants_ia32.h (right):

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/constants_ia3...
runtime/vm/constants_ia32.h:64: const Register CALLEE_SAVED = EDI;  // Alias for
a callee saved register.
CALLEE_SAVED seems like a very generic name, how about CALLEE_SAVED_TMP

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:4342: Register temp =
locs()->temp(0).reg();
The index here is wrong, we are essentially using the same temp as the one used
for saving ESP.

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:4368: Register temp =
locs()->temp(0).reg();
Ditto comment about the temp index.
I am wondering if it would be more readable to have an enum of these temp slots
so that it is obvious that kSavedSpSlot is not used in any of the code below.

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/pages.h
File runtime/vm/pages.h (right):

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/pages.h#newco...
runtime/vm/pages.h:52: }
Where are these used? I don't see any code using these accessors.

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/profiler.cc
File runtime/vm/profiler.cc (right):

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:952: if (contained != code_contained) {
Doesn't short circuit when it is not contained in the heap
if (heap_->Contains(pc) && !heap_->CodeContains(pc)) {
  ...
  ...
}
would not call CodeContains(pc) for values that are not in the heap.
The reverse can never be true right, i.e CodeContains returns true and Contains
false.

[Cutch, 2014-02-27 15:52:37]

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/constants_ia32.h
File runtime/vm/constants_ia32.h (right):

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/constants_ia3...
runtime/vm/constants_ia32.h:64: const Register CALLEE_SAVED = EDI;  // Alias for
a callee saved register.
On 2014/02/26 23:53:12, regis wrote:
> I do not think this constant helps. Just use EDI where you need it as a callee
> saved and add a comment.

Done.

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/constants_ia3...
runtime/vm/constants_ia32.h:64: const Register CALLEE_SAVED = EDI;  // Alias for
a callee saved register.
On 2014/02/27 00:00:26, siva wrote:
> CALLEE_SAVED seems like a very generic name, how about CALLEE_SAVED_TMP

Done.

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/constants_x64.h
File runtime/vm/constants_x64.h (right):

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/constants_x64...
runtime/vm/constants_x64.h:88: const Register CALLEE_SAVED = R13;  // Alias for
a callee saved register.
On 2014/02/26 23:53:12, regis wrote:
> ditto

Done.

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:3988: summary->set_temp(0,
Location::RegisterLocation(CALLEE_SAVED));
On 2014/02/26 23:53:12, regis wrote:
> CALLEE_SAVED -> EDI));  // Callee saved.

Done.

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:4342: Register temp =
locs()->temp(0).reg();
On 2014/02/27 00:00:26, siva wrote:
> The index here is wrong, we are essentially using the same temp as the one
used
> for saving ESP.

Done.

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:3987: summary->set_temp(0,
Location::RegisterLocation(CALLEE_SAVED));
On 2014/02/26 23:53:12, regis wrote:
> ditto

Done.

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:4368: Register temp =
locs()->temp(0).reg();
On 2014/02/27 00:00:26, siva wrote:
> Ditto comment about the temp index.
> I am wondering if it would be more readable to have an enum of these temp
slots
> so that it is obvious that kSavedSpSlot is not used in any of the code below.

Added index constants to instruction.

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/pages.h
File runtime/vm/pages.h (right):

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/pages.h#newco...
runtime/vm/pages.h:52: }
On 2014/02/27 00:00:26, siva wrote:
> Where are these used? I don't see any code using these accessors.

Left over debug code. Removed.

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/profiler.cc
File runtime/vm/profiler.cc (right):

https://codereview.chromium.org/180243013/diff/20001/runtime/vm/profiler.cc#n...
runtime/vm/profiler.cc:952: if (contained != code_contained) {
On 2014/02/27 00:00:26, siva wrote:
> Doesn't short circuit when it is not contained in the heap
> if (heap_->Contains(pc) && !heap_->CodeContains(pc)) {
>   ...
>   ...
> }
> would not call CodeContains(pc) for values that are not in the heap.
> The reverse can never be true right, i.e CodeContains returns true and
Contains
> false.

Done.

[siva, 2014-02-27 17:37:03]

lgtm

[Cutch, 2014-02-27 17:39:08]

Committed patchset #3 manually as r33122 (presubmit successful).