On 2012/06/13 09:11:29, Yang wrote:
On 2012/05/28 19:13:08, Yang wrote:
> On 2012/05/28 16:24:00, m.m.capewell wrote:
> > Description from the patch:
> > The emission of constant pool depends on the size of the code
generated
and
> > the number of RelocInfo recorded.
> > The Debug mechanism needs to map code offsets between two versions of
a
> > function, compiled with and without debugger support (see for example
> > Debug::PrepareForBreakPoints()).
> > Compiling functions with debugger support generates additional code
> > (Debug::GenerateSlot()). This may affect the emission of the constant
> > pools and cause the version of the code with debugger support to have
> > constant pools generated in different places.
> > Recording the position and size of emitted constant pools allows to
> > correctly compute the offset mappings between the different versions
of a
> > function in all situations.
>
> I'll take a look as soon as I'm back from vacation (Monday in a! week).
Sorry that it took this long.
Putting ifdefs in all places seems rather ugly to me, and not really
consistent
too (e.g. IsConstPool is not ifdef'ed). We also try to keep
platform-dependent
code to a minimum. Also, it probably safer not to make the assumption that
constant pools are limited to ARM.
It seems that the constant pool handling is not part of any performance
critical
code, and having code to handle constant pools even on Intel platforms
does no
harm (e.g. the second block in debug.cc's
RedirectActivationsToRecompiledCodeOnThread would work on ia32 and x64 as
well,
even though there is no need to check for constant pools). I think it's
better
(for code health and readability) to simply include constant pool related
code
on all platforms, and only take advantage of that infrastructure on ARM.
Drive-by comment: It would be nice to have a regression test for this bug.
We
now actually have indirect coverage via mjsunit/debug-liveedit-breakpoints,
but
that's just by accident. So it might be good to have a targeted regression
test
for just this bug. BTW, I opened an issue to track this.
http://code.google.com/p/v8/issues/detail?id=2179
https://chromiumcodereview.appspot.com/10449047/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
