Bug 163255 – HasOwnPropertyCache needs to ref the UniquedStringImpls it sees

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

...

[Saam Barati]

Created attachment 291295 [details]
patch

[Saam Barati]

Comment on attachment 291295 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291295&action=review

> Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:62
> +        bitwise_cast<HasOwnPropertyCache*>(cache)->clear();

I'll make this a static_cast

[WebKit Commit Bot]

Attachment 291295 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:116:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

Comment on attachment 291295 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291295&action=review

> Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:133
>          memset(this, 0, sizeof(Entry) * size);

Could you just placement new instead?

[Geoffrey Garen]

Comment on attachment 291295 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291295&action=review

> Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:123
> +            bitwise_cast<Entry*>(this)[i].Entry::~Entry();

Pointer casting can use reinterpret_cast.

[Saam Barati]

(In reply to comment #5)
> Comment on attachment 291295 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=291295&action=review
> 
> > Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:123
> > +            bitwise_cast<Entry*>(this)[i].Entry::~Entry();
> 
> Pointer casting can use reinterpret_cast.

I prefer bitwise_cast for this still because it does the sizeof check. Which prevents  e from making mistakes like this:
unsigned bar = ...;
Foo* ptr = bitwise_cast<Foo*>(bar);

[Saam Barati]

Created attachment 291315 [details]
patch for landing

[WebKit Commit Bot]

Attachment 291315 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/HasOwnPropertyCache.h:118:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 291315 [details]
patch for landing

Clearing flags on attachment: 291315

Committed r207186: <http://trac.webkit.org/changeset/207186>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Garcia Campos]

This broke the build in several linux bots (GTK+, JSCOnly). Maybe something not supported by GCC 4.9?

https://build.webkit.org/builders/JSCOnly%20Linux%20ARMv7%20Thumb2%20Release/builds/2318/steps/compile-webkit/logs/stdio

[Csaba Osztrogonác]

(In reply to comment #11)
> This broke the build in several linux bots (GTK+, JSCOnly). Maybe something
> not supported by GCC 4.9?
> 
> https://build.webkit.org/builders/JSCOnly%20Linux%20ARMv7%20Thumb2%20Release/
> builds/2318/steps/compile-webkit/logs/stdio

I also noticed this error, adding Entry::Entry(RefPtr<UniquedStringImpl>, StructureID, bool) constructor fixed the build for me. I'm goning to land
it immediately.

[Csaba Osztrogonác]

Fix landed in http://trac.webkit.org/changeset/207213.


(In reply to comment #11)
> This broke the build in several linux bots (GTK+, JSCOnly). Maybe something
> not supported by GCC 4.9?
> 
> https://build.webkit.org/builders/JSCOnly%20Linux%20ARMv7%20Thumb2%20Release/
> builds/2318/steps/compile-webkit/logs/stdio

But the question is why the GTK EWS was green? Do you have different
compiler on the EWS and buildbots? It would be great to keep them in sync.

[Csaba Osztrogonác]

Additionally it was broken on Apple Windows and WinCairo bots too.

Is it a compiler bug in MSVC and GCC? Is C++11/14/17 allows the compiler to
generate Entry::Entry(RefPtr<UniquedStringImpl>, StructureID, bool) constructor?

[Csaba Osztrogonác]

(In reply to comment #14)
> Additionally it was broken on Apple Windows and WinCairo bots too.
> 
> Is it a compiler bug in MSVC and GCC? Is C++11/14/17 allows the compiler to
> generate Entry::Entry(RefPtr<UniquedStringImpl>, StructureID, bool)
> constructor?

I found the proper answer for this problem. In C++11 Entry isn't an aggregate
because of brace initializers of its members, but it is in C++14.

http://stackoverflow.com/questions/35891233/initializing-a-struct-with-aggregate-initialization-and-member-initializers

The problem here is that GCC 4.9 and VS 2015 
doesn't support NSDMIs for aggregates.

[Carlos Garcia Campos]

(In reply to comment #13)
> Fix landed in http://trac.webkit.org/changeset/207213.
> 
> 
> (In reply to comment #11)
> > This broke the build in several linux bots (GTK+, JSCOnly). Maybe something
> > not supported by GCC 4.9?
> > 
> > https://build.webkit.org/builders/JSCOnly%20Linux%20ARMv7%20Thumb2%20Release/
> > builds/2318/steps/compile-webkit/logs/stdio
> 
> But the question is why the GTK EWS was green? Do you have different
> compiler on the EWS and buildbots?

Yes, unfortunately.

> It would be great to keep them in sync.

Agree

[Darin Adler]

I believe the build fix added reference count churn:

+        Entry(RefPtr<UniquedStringImpl> impl, StructureID structureID, bool result)
+            : impl(impl)

This is not the same thing as the automatically generated behavior, which can do a move if passed a RefPtr&&.

[Saam Barati]

(In reply to comment #17)
> I believe the build fix added reference count churn:
> 
> +        Entry(RefPtr<UniquedStringImpl> impl, StructureID structureID, bool
> result)
> +            : impl(impl)
> 
> This is not the same thing as the automatically generated behavior, which
> can do a move if passed a RefPtr&&.

Yeah I believe it should always be passed a RefPtr&&. This should be an easy fix.

Does the `StructName { ... }` syntax call the constructor?

[Darin Adler]

(In reply to comment #18)
> Does the `StructName { ... }` syntax call the constructor?

Yes.

[Saam Barati]

(In reply to comment #19)
> (In reply to comment #18)
> > Does the `StructName { ... }` syntax call the constructor?
> 
> Yes.

Interesting. I had thought the "{ ... }" just individually constructs each field.
Do you know what the rules are for it calling a constructor? Must there be a constructor that matches the types? Like
StructName{ T1,...,TN }
Calls constructor of type
ConstructorTye(T1, ..., TN)
?
(Ignoring type conversions, etc)

[Darin Adler]

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > Does the `StructName { ... }` syntax call the constructor?
> > 
> > Yes.
> 
> Interesting. I had thought the "{ ... }" just individually constructs each
> field.

Only if there is no constructor.

> Do you know what the rules are for it calling a constructor?

An oversimplification: direct initialization of the data members with that syntax works only if there are no constructors. For the full story, C++ reference material recommended. I normally use cppreference.com to look things up.

 Must there be a
> constructor that matches the types? Like
> StructName{ T1,...,TN }
> Calls constructor of type
> ConstructorTye(T1, ..., TN)
> ?
> (Ignoring type conversions, etc)

Yes. In fact, the ClassName { } syntax does not allow many type conversions that the ClassName ( ) syntax performs, which is another benefit of using it.

[Saam Barati]

I've opened this bug to go back to move semantics:
https://bugs.webkit.org/show_bug.cgi?id=163490

[Saam Barati]

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > (In reply to comment #18)
> > > > Does the `StructName { ... }` syntax call the constructor?
> > > 
> > > Yes.
> > 
> > Interesting. I had thought the "{ ... }" just individually constructs each
> > field.
> 
> Only if there is no constructor.
> 
> > Do you know what the rules are for it calling a constructor?
> 
> An oversimplification: direct initialization of the data members with that
> syntax works only if there are no constructors. For the full story, C++
> reference material recommended. I normally use cppreference.com to look
> things up.
Interesting. I'll read about this. It's surprising to me because I had just assumed the syntax was more or less shorthand for initializing each field. That was a bad assumption on my part. 
> 
>  Must there be a
> > constructor that matches the types? Like
> > StructName{ T1,...,TN }
> > Calls constructor of type
> > ConstructorTye(T1, ..., TN)
> > ?
> > (Ignoring type conversions, etc)
> 
> Yes. In fact, the ClassName { } syntax does not allow many type conversions
> that the ClassName ( ) syntax performs, which is another benefit of using it.