Fix template bugs that prevent rolling gtest DEPS in Chrome.

Fix template bugs that prevent rolling gtest DEPS in Chrome.

BUG=none

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179278

[Nick Bray (chromium), 2014-07-30 22:10:56]

First blink-side change ever, choosing arbitrary reviewers from OWNERS.

[Nick Bray (chromium), 2014-07-30 22:16:55]

I forgot: it looks like some of the oilpan code suffers from the same bug, but I
didn't fix it because it wasn't strictly necessary for my purposes and I didn't
know how to test the #ifdefed code.

[Ken Russell (switch to Gerrit), 2014-07-30 22:21:29]

This looks fine, but ager@ or haraken@ should review the change to
platform/heap/.

https://codereview.chromium.org/429793003/diff/1/Source/platform/heap/Visitor.h
File Source/platform/heap/Visitor.h (right):

https://codereview.chromium.org/429793003/diff/1/Source/platform/heap/Visitor...
Source/platform/heap/Visitor.h:142: template <typename T> const bool
NeedsAdjustAndMark<T, true>::value;
Why is this out-of-line definition needed? Will this cause the variable to be
instantiated into every translation unit which includes this header?

[Nico, 2014-07-30 22:26:02]

https://codereview.chromium.org/429793003/diff/1/Source/platform/heap/Visitor.h
File Source/platform/heap/Visitor.h (right):

https://codereview.chromium.org/429793003/diff/1/Source/platform/heap/Visitor...
Source/platform/heap/Visitor.h:142: template <typename T> const bool
NeedsAdjustAndMark<T, true>::value;
On 2014/07/30 22:21:29, Ken Russell wrote:
> Why is this out-of-line definition needed? Will this cause the variable to be
> instantiated into every translation unit which includes this header?

Yes, that seems wrong.

[haraken, 2014-07-30 22:29:56]

+oilpan-reviews, +zerny

[haraken, 2014-07-30 22:31:18]

On 2014/07/30 22:16:55, Nick Bray (chromium) wrote:
> I forgot: it looks like some of the oilpan code suffers from the same bug, but
I
> didn't fix it because it wasn't strictly necessary for my purposes and I
didn't
> know how to test the #ifdefed code.

You can set GYP_DEFINES='enable_oilpan=1 blink_gc_plugin=1' and build it.

[Nick Bray (chromium), 2014-07-30 22:33:45]

https://codereview.chromium.org/429793003/diff/1/Source/platform/heap/Visitor.h
File Source/platform/heap/Visitor.h (right):

https://codereview.chromium.org/429793003/diff/1/Source/platform/heap/Visitor...
Source/platform/heap/Visitor.h:142: template <typename T> const bool
NeedsAdjustAndMark<T, true>::value;
On 2014/07/30 22:21:29, Ken Russell wrote:
> Why is this out-of-line definition needed? Will this cause the variable to be
> instantiated into every translation unit which includes this header?

It isn't technically "out-of-line" because the inline definition is incomplete
on its own.  static variables in classes are essentially an "extern" declaration
for storage that is declared elsewhere.  Normally that "elsewhere" is in a .cc
file, but for templates you don't want to declare the storage for each concrete
instantiation.  Declaring it in a header file would normally cause a multiple
definition problem because each source file that included the header file would
create its own version, but C++ cheats and has a "one definition rule" that
covers this case.  The templated instantiations of this variable are stored in
the COMDAT section and deduplicated, similar to inline method definitions in a
header file.

http://stackoverflow.com/questions/1834597/what-is-the-comdat-section-used-for

Without this declaration, compilers _may_ be able to const fold accesses to this
field out of existence before the link step, so the missing storage declaration
is not noticed.  No such luck with gtest, however, there was a link error.

[Nico, 2014-07-30 22:35:03]

On Wed, Jul 30, 2014 at 3:33 PM, <ncbray@chromium.org> wrote:

>
> https://codereview.chromium.org/429793003/diff/1/Source/
> platform/heap/Visitor.h
> File Source/platform/heap/Visitor.h (right):
>
> https://codereview.chromium.org/429793003/diff/1/Source/
> platform/heap/Visitor.h#newcode142
> Source/platform/heap/Visitor.h:142: template <typename T> const bool
> NeedsAdjustAndMark<T, true>::value;
> On 2014/07/30 22:21:29, Ken Russell wrote:
>
>> Why is this out-of-line definition needed? Will this cause the
>>
> variable to be
>
>> instantiated into every translation unit which includes this header?
>>
>
> It isn't technically "out-of-line" because the inline definition is
> incomplete on its own.  static variables in classes are essentially an
> "extern" declaration for storage that is declared elsewhere.  Normally
> that "elsewhere" is in a .cc file, but for templates you don't want to
> declare the storage for each concrete instantiation.  Declaring it in a
> header file would normally cause a multiple definition problem because
> each source file that included the header file would create its own
> version, but C++ cheats and has a "one definition rule" that covers this
> case.  The templated instantiations of this variable are stored in the
> COMDAT section and deduplicated, similar to inline method definitions in
> a header file.
>

COMDAT is a windows-only thing, right?


>
> http://stackoverflow.com/questions/1834597/what-is-the-
> comdat-section-used-for
>
> Without this declaration, compilers _may_ be able to const fold accesses
> to this field out of existence before the link step, so the missing
> storage declaration is not noticed.  No such luck with gtest, however,
> there was a link error.
>
> https://codereview.chromium.org/429793003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Nick Bray (chromium), 2014-07-30 22:41:37]

> COMDAT is a windows-only thing, right?

Nope.
https://refspecs.linuxbase.org/elf/gabi4+/ch4.sheader.html
http://en.wikipedia.org/wiki/One_Definition_Rule

Also, if I was wrong and the variable wasn't de-duped, this change would cause a
link error.

[Ken Russell (switch to Gerrit), 2014-07-30 22:42:27]

On 2014/07/30 22:41:37, Nick Bray (chromium) wrote:
> > COMDAT is a windows-only thing, right?
> 
> Nope.
> https://refspecs.linuxbase.org/elf/gabi4+/ch4.sheader.html
> http://en.wikipedia.org/wiki/One_Definition_Rule
> 
> Also, if I was wrong and the variable wasn't de-duped, this change would cause
a
> link error.

Good and thorough analysis.

LGTM from my standpoint, but please wait for ager or haraken's review.

[haraken, 2014-07-30 22:44:34]

I'm not much familiar with compiler details and I'll delegate the review to kbr
and Nico.

rubberstamp LGTM as an OWNER.

[haraken, 2014-07-30 22:44:48]

FYI, ager is on vacation.

[Nick Bray (chromium), 2014-07-30 22:47:12]

I forgot to cross-link, here are similar changes Chrome side:

https://codereview.chromium.org/431533003/

[Nico, 2014-07-30 22:56:58]

On Wed, Jul 30, 2014 at 3:42 PM, <kbr@chromium.org> wrote:

> On 2014/07/30 22:41:37, Nick Bray (chromium) wrote:
>
>> > COMDAT is a windows-only thing, right?
>>
>
>  Nope.
>> https://refspecs.linuxbase.org/elf/gabi4+/ch4.sheader.html
>> http://en.wikipedia.org/wiki/One_Definition_Rulen
>>
>
>  Also, if I was wrong and the variable wasn't de-duped, this change would
>> cause
>>
> a
>
>> link error.
>>
>
I believe your explanation (lgtm), but I hadn't heard the name "comdat" on
non-windows before. You learn something new every day :-) (
http://www.airs.com/blog/archives/52 too )


>
> Good and thorough analysis.
>
> LGTM from my standpoint, but please wait for ager or haraken's review.
>
>
> https://codereview.chromium.org/429793003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Ken Russell (switch to Gerrit), 2014-07-31 00:24:20]

The CQ bit was checked by kbr@chromium.org

[commit-bot: I haz the power, 2014-07-31 00:24:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ncbray@chromium.org/429793003/1

[Ken Russell (switch to Gerrit), 2014-07-31 01:13:28]

The CQ bit was unchecked by kbr@chromium.org

[Nick Bray (chromium), 2014-07-31 01:30:43]

The CQ bit was checked by ncbray@chromium.org

[commit-bot: I haz the power, 2014-07-31 01:31:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ncbray@chromium.org/429793003/1

[commit-bot: I haz the power, 2014-07-31 02:42:15]

Change committed as 179278