On 2012/06/13 09:22:04, Michael Starzinger wrote:
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
Ooops, pasted the wrong link, should have been ...
http://code.google.com/p/v8/issues/detail?id=2177
https://chromiumcodereview.appspot.com/10449047/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
