Refactor lazy_instance to avoid a strict aliasing problem with gcc 4.4.

Refactor lazy_instance to avoid a strict aliasing problem with gcc 4.4.

The key change here is to use the Type* returned from placement new rather
than casting the buffer to a Type*.

This change also rolls the lazy instance helper into lazy instance as the lazy
instance helper would have to have been templatized to deal with the aforementioned
change and so would have to have moved into the header file anyway.

BUG=28749

[Craig, 2010-01-10 14:59:42]

Dean I was hoping you could review this for me or recommend someone for the task
please.

Other than the comments in the description, the other interesting part of the
change is that I added a MemoryBarrier call to EnsureInstance - rationale:
ensure Type* instance_ is visible to other threads before updating state_ but
I'm not 100% sure if that is needed as the update to state using Release_Store
does a ATOMICOPS_COMPILER_BARRIER() ...

Thank you!

[Craig, 2010-01-10 16:01:35]

On 2010/01/10 14:59:42, Craig wrote:
> Dean I was hoping you could review this for me or recommend someone for the
task
> please.
> 
> Other than the comments in the description, the other interesting part of the
> change is that I added a MemoryBarrier call to EnsureInstance - rationale:
> ensure Type* instance_ is visible to other threads before updating state_ but
> I'm not 100% sure if that is needed as the update to state using Release_Store
> does a ATOMICOPS_COMPILER_BARRIER() ...

I've nuked the extra MemoryBarrier in patch set 4. I don't think it's needed if
I understand Release_Store correctly.

[Dean McNamee, 2010-01-10 19:05:03]

When this was written, Darin specifically wanted the helper non-template
superclass to reduce the footprint of the generated code, and to have less code
in the header...

Darin?

[darin (slow to review), 2010-01-12 08:19:04]

Yeah, it's usually good practice to factor template classes like this if they
are meaty enough.  This one seems meaty.

It seems like you could also fix the aliasing issue by wrapping Traits::New with
code that assigns to the Type* member but still looks the same to the
LazyInstanceHelper.

[Craig, 2010-01-12 12:06:32]

On 2010/01/12 08:19:04, darin wrote:
> Yeah, it's usually good practice to factor template classes like this if they
> are meaty enough.  This one seems meaty.

OK.

> It seems like you could also fix the aliasing issue by wrapping Traits::New
with
> code that assigns to the Type* member but still looks the same to the
> LazyInstanceHelper.

Perhaps I'm being denser than usual but despite that sounding conceptually good,
I'm not seeing how to implement that immediately. I'll mull it over a bit with
some coffee for inspiration.

What I did think of doing (but it's ugly so I probably shouldn't even suggest
it) is to split EnsureInstance into "NeedInstance" and "CompleteInstance" and
then do something like this in the lazyinstance caller:

if (..not created yet... && NeedInstance()) {
  // we're in CREATING state now
  instance_ = Traits::New(buf_);
  CompleteInstance(instance_, Traits::Delete); // hooks up destruction  stuff
and moved to CREATED
}

That adds an extra line or two to the header but keeps most of the complexity in
the helper...

[darin (slow to review), 2010-01-12 18:09:09]

On Tue, Jan 12, 2010 at 4:06 AM, <craig.schlenter@chromium.org> wrote:

> On 2010/01/12 08:19:04, darin wrote:
>
>> Yeah, it's usually good practice to factor template classes like this if
>> they
>> are meaty enough.  This one seems meaty.
>>
>
> OK.
>
>
>  It seems like you could also fix the aliasing issue by wrapping
>> Traits::New
>>
> with
>
>> code that assigns to the Type* member but still looks the same to the
>> LazyInstanceHelper.
>>
>
> Perhaps I'm being denser than usual but despite that sounding conceptually
> good,
> I'm not seeing how to implement that immediately. I'll mull it over a bit
> with
> some coffee for inspiration.
>
> What I did think of doing (but it's ugly so I probably shouldn't even
> suggest
> it) is to split EnsureInstance into "NeedInstance" and "CompleteInstance"
> and
> then do something like this in the lazyinstance caller:
>
> if (..not created yet... && NeedInstance()) {
>  // we're in CREATING state now
>  instance_ = Traits::New(buf_);
>  CompleteInstance(instance_, Traits::Delete); // hooks up destruction
>  stuff
> and moved to CREATED
> }
>
> That adds an extra line or two to the header but keeps most of the
> complexity in
> the helper...
>
>
Something like that seems reasonable to me, but I think I'm confused about
the underlying issue.  I don't have convenient access to GCC 4.4, so I can't
easily prototype solutions.  Can you share the error output from GCC 4.4?

-Darin




>
> http://codereview.chromium.org/519045
>

[Craig, 2010-01-12 18:24:15]

On 2010/01/12 18:09:09, darin wrote:
[snip]
> Something like that seems reasonable to me, but I think I'm confused about
> the underlying issue.  I don't have convenient access to GCC 4.4, so I can't
> easily prototype solutions.  Can you share the error output from GCC 4.4?

The compiler isn't actually cranking out any error messages normally despite the
use of -Wall which is why it took a while to track it down to a strict aliasing
issue.

I have a piece of test code that shows the issue under valgrind here:
http://code.google.com/p/chromium/issues/detail?id=28749#c22
 and the error message (when using -Wstrict-aliasing=2 which is prone to more
false positives) is:

In file included from test.cc:10:
base/lazy_instance.h: In member function ‘Type* base::LazyInstance<Type, 
Traits>::Pointer() [with Type = std::stack<myscoped_refptr<PluginChannelBase>, 
std::deque<myscoped_refptr<PluginChannelBase>, 
std::allocator<myscoped_refptr<PluginChannelBase> > > >, Traits = 
base::DefaultLazyInstanceTraits<std::stack<myscoped_refptr<PluginChannelBase>, 
std::deque<myscoped_refptr<PluginChannelBase>, 
std::allocator<myscoped_refptr<PluginChannelBase> > > > >]’:
test.cc:130:   instantiated from here
base/lazy_instance.h:98: warning: dereferencing type-punned pointer will break 
strict-aliasing rules

[Craig, 2010-01-12 18:38:59]

I've put a rough version of the "NeedsInstance" version of the patch here btw.:

http://codereview.chromium.org/548011

I can't think of an appropriately cool name for "NeedsInstance" given that it's
doing two things ... we could also arguably do something like this instead and
introduce another helper WaitForInstance:

if (state not CREATED) {
  if (NeedsInstance()) {
     instance_ = placement new
     CompleteInstance(...)
  } else {
     WaitForInstance()
  }
}

[darin (slow to review), 2010-01-12 18:52:35]

On Tue, Jan 12, 2010 at 10:38 AM, <craig.schlenter@chromium.org> wrote:

> I've put a rough version of the "NeedsInstance" version of the patch here
> btw.:
>
> http://codereview.chromium.org/548011
>
> I can't think of an appropriately cool name for "NeedsInstance" given that
> it's
> doing two things ... we could also arguably do something like this instead
> and
> introduce another helper WaitForInstance:
>
> if (state not CREATED) {
>  if (NeedsInstance()) {
>     instance_ = placement new
>     CompleteInstance(...)
>  } else {
>     WaitForInstance()
>
>  }
> }
>
>
^^^ that seems a bit more clear to me, but the way you have it now is
reasonable too.

-darin



> http://codereview.chromium.org/519045
>

[jamesr, 2010-01-12 19:13:07]

Naive suggestion:

Just heap allocate the space for Type.  Our allocator should be fast enough to
handle that - it's certainly cheaper to do a small allocation than to do a
CompareAndSwap.  Then you don't have to worry about aliasing the space for Type.

[Craig, 2010-01-12 19:20:09]

On 2010/01/12 19:13:07, jamesr wrote:
> Naive suggestion:
> 
> Just heap allocate the space for Type.  Our allocator should be fast enough to
> handle that - it's certainly cheaper to do a small allocation than to do a
> CompareAndSwap.  Then you don't have to worry about aliasing the space for
Type.

Wouldn't you still need something along the lines of CompareAndSwap to make it
thread safe so you don't end up allocating multiple objects when two threads
race calling Pointer()?

[jamesr, 2010-01-12 19:21:45]

On 2010/01/12 19:20:09, Craig wrote:
> On 2010/01/12 19:13:07, jamesr wrote:
> > Naive suggestion:
> > 
> > Just heap allocate the space for Type.  Our allocator should be fast enough
to
> > handle that - it's certainly cheaper to do a small allocation than to do a
> > CompareAndSwap.  Then you don't have to worry about aliasing the space for
> Type.
> 
> Wouldn't you still need something along the lines of CompareAndSwap to make it
> thread safe so you don't end up allocating multiple objects when two threads
> race calling Pointer()?

Yes, you'd still need all the same synchronization.  I don't think the cost of
heap allocating is significant compared to that, however.

[jamesr, 2010-01-13 06:39:45]

How does this code assure that buf_ is the proper alignment to have contain a
Type object?

[Craig, 2010-01-13 07:11:28]

On 2010/01/13 06:39:45, jamesr wrote:
> How does this code assure that buf_ is the proper alignment to have contain a
> Type object?

I actually fiddled with some alignment stuff when I was first trying to work out
what was wrong (before I realised it was aliasing) but forgot to add it in
permanently. We could feasibly add the gcc alignment attribute to it in an
appropriate #ifdef. I'm not sure what alignment data members have by default
btw.

[Craig, 2010-01-13 21:01:18]

I'm going to close this btw. since it has been replaced by
http://codereview.chromium.org/548011