ARM: Fix literal pool handling for breakpoints in debugger....

ARM: Fix literal pool handling for breakpoints in debugger.

BUG=2177
TEST=none

Committed: https://code.google.com/p/v8/source/detail?r=11812

[m.m.capewell, 2012-05-28 16:24:00]

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.

[Yang, 2012-05-28 19:13:08]

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).

[Yang, 2012-06-13 09:11:29]

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.

[Michael Starzinger, 2012-06-13 09:22:04]

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

[Michael Starzinger, 2012-06-13 09:23:15]

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

[m.m.capewell, 2012-06-14 10:58:53]

Updated to address review comments.

[Yang, 2012-06-14 11:07:13]

LGTM. Landing.

Though Michael has a point. We would certainly welcome a regression test for
this.