Avoid a strict aliasing issue in LazyInstance.

Avoid a strict aliasing issue in LazyInstance.

The key change here is to use the return value from placement
new rather than casting buf_. Avoiding the cast avoids the
strict aliasing issue.

BUG=28749
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=41434

[Craig, 2010-01-12 19:15:05]

This is the alternative to my other attempt at fixing this (which was at
http://codereview.chromium.org/519045).

I'm happy to code up a slightly less cryptic alternative e.g.:

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

but I'll probably only get to looking at any of this again tomorrow.

Please review.

Thank you!

[darin (slow to review), 2010-01-12 20:32:14]

LGTM

http://codereview.chromium.org/548011/diff/1/3
File base/lazy_instance.h (right):

http://codereview.chromium.org/548011/diff/1/3#newcode74
base/lazy_instance.h:74: bool NeedInstance();
nit: in the CL description, you call this NeedsInstance.

[Dean McNamee, 2010-01-13 00:59:31]

This code is tricky, so I'll try to review it when I get a chance.

It is a bit sad to add an instance_, since you always know it will point to
&buf_, so it's growing the runtime memory size to work around a compile time
warning...

[Craig, 2010-01-13 14:58:42]

I've done s/NeedInstance/NeedsInstance as per Darin
and aligned the buffer as suggested by jamesr. I used
16 byte alignment which should be good enough for pretty
much anything.

I tried to use __BIGGEST_ALIGNMENT__ btw. but that
seems not to work on older gcc versions.

Please take another look if you can. Thank you.

[darin (slow to review), 2010-01-13 18:04:03]

http://codereview.chromium.org/548011/diff/2007/2009
File base/lazy_instance.h (right):

http://codereview.chromium.org/548011/diff/2007/2009#newcode124
base/lazy_instance.h:124: int8 buf_[sizeof(Type)] __attribute__((aligned(16)));
i'm confused about the need to align this member.  won't the first member of a
struct already be sufficiently aligned?

[Craig, 2010-01-13 18:56:35]

On 2010/01/13 18:04:03, darin wrote:
> http://codereview.chromium.org/548011/diff/2007/2009
> File base/lazy_instance.h (right):
> 
> http://codereview.chromium.org/548011/diff/2007/2009#newcode124
> base/lazy_instance.h:124: int8 buf_[sizeof(Type)]
__attribute__((aligned(16)));
> i'm confused about the need to align this member.  won't the first member of a
> struct already be sufficiently aligned?

It's "normally" ok as far as I can tell for gcc but I recall issues in v8 that
needed an align(16) when the compiler cranked out some vector instructions
somewhere to copy something.

The best info I can find is:

http://gcc.gnu.org/onlinedocs/gcc/Variable-Attributes.html

[quote]
The default alignment is sufficient for all scalar types, but may not be enough
for all vector types on a target which supports vector operations.
[/quote]

I'm unsure of windows .. will dig a bit and report back.

[Peter Kasting, 2010-01-13 19:25:02]

http://codereview.chromium.org/548011/diff/2007/2009
File base/lazy_instance.h (right):

http://codereview.chromium.org/548011/diff/2007/2009#newcode124
base/lazy_instance.h:124: int8 buf_[sizeof(Type)] __attribute__((aligned(16)));
On 2010/01/13 18:04:03, darin wrote:
> i'm confused about the need to align this member.  won't the first member of a
> struct already be sufficiently aligned?

The first member of a struct will have the same alignment as the struct itself. 
The struct is guaranteed to have an alignment at least as large as the maximum
alignment required by any one member.  In this case the struct is naturally
aligned to the max of (Atomic32, int8, Type*), which looks like 4 bytes.  So the
16-byte alignment requested here does something.

(In practice, the struct will be overaligned in many cases for a perf benefit,
but nothing guarantees that.)

[jamesr, 2010-01-13 21:05:35]

http://codereview.chromium.org/548011/diff/2007/2009
File base/lazy_instance.h (right):

http://codereview.chromium.org/548011/diff/2007/2009#newcode124
base/lazy_instance.h:124: int8 buf_[sizeof(Type)] __attribute__((aligned(16)));
On 2010/01/13 18:04:03, darin wrote:
> i'm confused about the need to align this member.  won't the first member of a
> struct already be sufficiently aligned?

'Type' might have a larger alignment requirement than this type (say, for
example, it contains a member of type 'double' or '__m128i').

[Craig, 2010-01-14 06:48:13]

Here are the msvc alignment rules btw.

http://msdn.microsoft.com/en-us/library/83ythb65.aspx

[Craig, 2010-02-10 15:24:10]

So I haven't forgotten about this :)

I have sifted through the current users of LazyInstance and I can't see any at
the moment that will not be happy with the default alignment of 4 bytes. I still
think the revised alignment of 16 bytes for the buffer is a good idea to cater
for possible future users of this code with different alignment requirements.

Dean: have you had a chance to look at this yet btw.?

thank you.

[Craig, 2010-03-11 18:53:48]

I'm going to land the first patch set here, probably tomorrow, with the
(trivial) nits addressed. That patch has a LGTM from Darin. If anyone has any
objections, yell soon please to stop me. I've sent it to the try servers again
of course to confirm it's still happy and have been running it locally for ages
without issues.