Assertion failure when using --log-regexp

Assertion failure when using --log-regexp

RegExpCompileEvent acquieres mutex from Log class during MessageBuilder creation. LogRegExpSource, called from RegExpCompileEvent creates another MessageBuilder object which also acquires the same mutex. This mutex is not recursive, so during second acquirement, assertion fail is happening. Solution: LogRegExpSource should use the same MessageBuilder object as RegExpCompileEvent.

Committed: https://crrev.com/7be96aa2e7995a19ec96dc04da38216837f1eb6e
Cr-Commit-Position: refs/heads/master@{#29347}

[Djordje.Pesic, 2015-06-23 14:01:05]

djordje.pesic@imgtec.com changed reviewers:
+ akos.palfi@imgtec.com, balazs.kilvady@imgtec.com,
dusan.milosavljevic@imgtec.com, jkummerow@chromium.org, loislo@chromium.org,
paul.lind@imgtec.com

[dusmil.imgtec, 2015-06-24 13:33:05]

Lgtm, needs approval from Google team.

[Jakob Kummerow, 2015-06-24 13:40:01]

General approach is fine, but I have style nits.

https://codereview.chromium.org/1207433002/diff/1/src/log.h
File src/log.h (right):

https://codereview.chromium.org/1207433002/diff/1/src/log.h#newcode13
src/log.h:13: #include "src/flags.h"
I don't think flags.h is needed below.

https://codereview.chromium.org/1207433002/diff/1/src/log.h#newcode360
src/log.h:360: void LogRegExpSource(Handle<JSRegExp> regexp,
Log::MessageBuilder& msg);
The style guide forbids non-const references. Either use a const reference, or
use a pointer, which would have the additional benefit that you can
forward-declare Log::MessageBuilder (and drop the log-utils.h include).

[Djordje.Pesic, 2015-06-25 08:06:16]

https://codereview.chromium.org/1207433002/diff/1/src/log.h
File src/log.h (right):

https://codereview.chromium.org/1207433002/diff/1/src/log.h#newcode13
src/log.h:13: #include "src/flags.h"
On 2015/06/24 13:40:01, Jakob wrote:
> I don't think flags.
I have to include flags.h because log-utils.h uses flags defined in flags.h.
Alternatively, I can include flags.h in log-utils.h (log-utils.h doesn't include
it). Objects.h includes flags.h, but presubmit script complains about
alphabetical order of includes.Also, presubmit script doesn't allow forward
declarations like Log::MessageBuilder.

https://codereview.chromium.org/1207433002/diff/1/src/log.h#newcode360
src/log.h:360: void LogRegExpSource(Handle<JSRegExp> regexp,
Log::MessageBuilder& msg);
On 2015/06/24 13:40:01, Jakob wrote:
> The style guide forbids non-const references. Either use a const reference, or
> use a pointer, which would have the additional benefit that you can
> forward-declare Log::MessageBuilder (and drop the log-utils.h include).

I have to include log-utils.h because class MessageBuilder is defined in class
Log, so forward-declaration of Log::MessageBuilder doesn't compile without
log-utils.h included. So, I used const reference.

[Djordje.Pesic, 2015-06-25 08:08:07]

> So, I used const reference.
Typo, I used pointer.

[Jakob Kummerow, 2015-06-29 09:11:50]

Sorry for the delay, I was OOO.

https://codereview.chromium.org/1207433002/diff/1/src/log.h
File src/log.h (right):

https://codereview.chromium.org/1207433002/diff/1/src/log.h#newcode13
src/log.h:13: #include "src/flags.h"
On 2015/06/25 08:06:16, Djordje.Pesic wrote:
> I have to include flags.h because log-utils.h uses flags defined in flags.h.
> Alternatively, I can include flags.h in log-utils.h (log-utils.h doesn't
include
> it).

Yes, please include it exactly where it's needed (so log-utils.h in this case). 

As a general rule, every file should include what it needs, no more, no less.
V8's includes are a big mess and rather far from following this rule everywhere,
but whenever we make changes we should strive for incremental improvement,
rather than making the situation even worse.

https://codereview.chromium.org/1207433002/diff/1/src/log.h#newcode360
src/log.h:360: void LogRegExpSource(Handle<JSRegExp> regexp,
Log::MessageBuilder& msg);
On 2015/06/25 08:06:16, Djordje.Pesic wrote:
> I have to include log-utils.h because class MessageBuilder is defined in class
> Log, so forward-declaration of Log::MessageBuilder doesn't compile without
> log-utils.h included. 

Hm... But I'd really like to get as far away as possible from "every header
includes every other header".

As far as I can see, LogRegExpSource only has a single call site in
RegExpCompileEvent, and only uses the logger to get the isolate. So you could
move it out of the Logger class and into an anonymous namespace, and
additionally pass in the isolate, to avoid the additional include (log.cc
includes log-utils.h anyway).

> So, I used pointer.

OK.

[Sven Panne, 2015-06-29 09:25:16]

On 2015/06/24 13:40:01, Jakob wrote:
> The style guide forbids non-const references. Either use a const reference, or
> use a pointer, which would have the additional benefit that you can
> forward-declare Log::MessageBuilder (and drop the log-utils.h include).

Just a quick note: There seems to be some confusion here, you *can*
forward-declare types used via references, in this regard they behave like plain
old pointers.

Apart from that, I fully agree with Jakob: Just nuke the helper function from
the header and define it in an anonymous namespace where it's needed.
Introducing more dependencies because of local helpers is a no-no. :-)

[Sven Panne, 2015-06-29 09:25:24]

svenpanne@chromium.org changed reviewers:
+ svenpanne@chromium.org

[Sven Panne, 2015-06-29 09:25:25]

DBC

[Djordje.Pesic, 2015-06-29 13:08:38]

https://codereview.chromium.org/1207433002/diff/1/src/log.h
File src/log.h (right):

https://codereview.chromium.org/1207433002/diff/1/src/log.h#newcode13
src/log.h:13: #include "src/flags.h"
On 2015/06/29 09:11:50, Jakob wrote:
> On 2015/06/25 08:06:16, Djordje.Pesic wrote:
> > I have to include flags.h because log-utils.h uses flags defined in flags.h.
> > Alternatively, I can include flags.h in log-utils.h (log-utils.h doesn't
> include
> > it).
> 
> Yes, please include it exactly where it's needed (so log-utils.h in this
case). 
> 
> As a general rule, every file should include what it needs, no more, no less.
> V8's includes are a big mess and rather far from following this rule
everywhere,
> but whenever we make changes we should strive for incremental improvement,
> rather than making the situation even worse.

Done.

https://codereview.chromium.org/1207433002/diff/1/src/log.h#newcode360
src/log.h:360: void LogRegExpSource(Handle<JSRegExp> regexp,
Log::MessageBuilder& msg);
On 2015/06/29 09:11:50, Jakob wrote:
> On 2015/06/25 08:06:16, Djordje.Pesic wrote:
> > I have to include log-utils.h because class MessageBuilder is defined in
class
> > Log, so forward-declaration of Log::MessageBuilder doesn't compile without
> > log-utils.h included. 
> 
> Hm... But I'd really like to get as far away as possible from "every header
> includes every other header".
> 
> As far as I can see, LogRegExpSource only has a single call site in
> RegExpCompileEvent, and only uses the logger to get the isolate. So you could
> move it out of the Logger class and into an anonymous namespace, and
> additionally pass in the isolate, to avoid the additional include (log.cc
> includes log-utils.h anyway).
> 
> > So, I used pointer.
> 
> OK.

Done.

[Jakob Kummerow, 2015-06-29 13:10:06]

LGTM, thanks!

[Djordje.Pesic, 2015-06-29 13:26:39]

The CQ bit was checked by djordje.pesic@imgtec.com

[Djordje.Pesic, 2015-06-29 13:26:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
dusan.milosavljevic@imgtec.com
Link to the patchset: https://codereview.chromium.org/1207433002/#ps40001
(title: "Addressed comments")

[commit-bot: I haz the power, 2015-06-29 13:26:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1207433002/40001

[commit-bot: I haz the power, 2015-06-29 13:54:04]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-06-29 13:54:18]

Patchset 3 (id:??) landed as
https://crrev.com/7be96aa2e7995a19ec96dc04da38216837f1eb6e
Cr-Commit-Position: refs/heads/master@{#29347}