Re: ARM: Fix literal pool handling for breakpoints in debugger.... (issue 10449047)

[yan...@chromium.org]

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.

http://codereview.chromium.org/10449047/

[mstar...@chromium.org]

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/

[mstar...@chromium.org]

Ooops, pasted the wrong link, should have been ...

http://code.google.com/p/v8/issues/detail?id=2177

https://chromiumcodereview.appspot.com/10449047/

[m.m.ca...@googlemail.com]

Updated to address review comments.

http://codereview.chromium.org/10449047/

[yan...@chromium.org]

LGTM. Landing.

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

https://chromiumcodereview.appspot.com/10449047/

[Alexandre Rames]

I was about to answer on that point.

This fix was made to fix a bug on top of a modified MAsm stressing literal pools generation. I don't see how to create a good and predictable test with the current code base.

Manually generating the bug would require fine control over literal pools and debug slots generation. For now we don't have such deep control over code generation from the tests.

So I agree we need one, but I think it should come with the incoming work on strengthening this mechanisms (See Rodolph's comment on the other related patch), which should provide options to influence and test literal pools.

Alexandre

Regarding Michael's comment, I think such a test is hard to create.
Manually creating a situation where the bug occurs requires fine control over the literal pools and debug slots generation. Tests don't have such deep control over code generation.

I think we should try to include a test when we introduce additional checking and control mechanisms around the literal pools generation.

--
v8-dev mailing list
v8-...@googlegroups.com
http://groups.google.com/group/v8-dev