use memcpy to fix strict-aliasing warning

[reed1, 2012-01-05 17:03:07]

I was unable to capture this in an inline function and still suppress the
warning :( I welcome others to try to make this easier to read...

[caryclark1, 2012-01-05 18:15:26]

http://codereview.appspot.com/5515047/diff/1/include/core/SkDescriptor.h
File include/core/SkDescriptor.h (right):

http://codereview.appspot.com/5515047/diff/1/include/core/SkDescriptor.h#newc...
include/core/SkDescriptor.h:161: memcpy(&fDesc, &fStorage[0], sizeof(fDesc));
What about a macro?
#define SK_STRICT_ASSIGN(dest, src) memcpy(&dest, &src[0], sizeof(dest))
SK_STRICT_ASSIGN(fDesc, fStorage);
or something like that?

[TomH, 2012-01-05 19:39:13]

Tangent: other sources of strict-aliasing breakage are:

src/animator/SkAnimatorScript.cpp
src/animator/SkDisplayNumber.cpp
bench/MathBench.cpp

[TomH, 2012-01-05 20:13:57]

On 2012/01/05 17:03:07, reed1 wrote:
> I was unable to capture this in an inline function and still suppress the
> warning :( I welcome others to try to make this easier to read...

This change builds, but segfaults in bench?
In gdb, set fDesc = &fStorage / &fStorage[0] both work, but the compiler is
doing something unexpected in that memcpy.

[TomH, 2012-01-06 18:17:09]

Performance tests of strict-aliasing vs no-strict-aliasing are confusing. For
example, the verts benchmark runs 25-30% faster with strict-aliasing turned on,
but we can't see any difference in the generated assembly code to explain the
performance improvement.

[joth, 2012-01-12 20:46:19]

http://codereview.appspot.com/5515047/diff/1/include/core/SkDescriptor.h
File include/core/SkDescriptor.h (right):

http://codereview.appspot.com/5515047/diff/1/include/core/SkDescriptor.h#newc...
include/core/SkDescriptor.h:162: #endif
another one to try (at least for comparison) would be placement new:
  fDesc = new (fStorage) SkDescriptor;

this is effectively what chrome's LazyInstance does (see usage of the
|private_buf_| member.)

[jbates, 2012-01-12 21:28:37]

http://codereview.appspot.com/5515047/diff/1/include/core/SkDescriptor.h
File include/core/SkDescriptor.h (right):

http://codereview.appspot.com/5515047/diff/1/include/core/SkDescriptor.h#newc...
include/core/SkDescriptor.h:159: fDesc = (SkDescriptor*)fStorage;
If you change fStorage to a uint8_t array, it would be safe to alias and the
warning would probably go away. The uint32 array is dangerous because any access
to the data as uint32 could be reordered with respect to accesses as
SkDescriptor. If fStorage is uint8, the compiler assumes it could alias
anything.

[reed1, 2012-01-12 21:32:27]

if true, that's wacky, since uint32_t is closing to ensuring proper alignement
for SkDescriptor than uint8_t.

[jbates, 2012-01-12 21:36:36]

On 2012/01/12 21:32:27, reed1 wrote:
> if true, that's wacky, since uint32_t is closing to ensuring proper alignement
> for SkDescriptor than uint8_t.

Good point -- you would also need to specify the alignment of the array with
__attribute__((__aligned__(alignof(SkDescriptor)))) for GCC and whatever the
MSVC variant is.

[jbates, 2012-01-12 21:39:37]

On 2012/01/12 21:36:36, jbates wrote:
> On 2012/01/12 21:32:27, reed1 wrote:
> > if true, that's wacky, since uint32_t is closing to ensuring proper
alignement
> > for SkDescriptor than uint8_t.
> 
> Good point -- you would also need to specify the alignment of the array with
> __attribute__((__aligned__(alignof(SkDescriptor)))) for GCC and whatever the
> MSVC variant is.

Although, since the array follows a pointer (fDesc), it's already going to be
aligned at least 4 bytes (8 bytes for the 64-bit build). But without the
alignment attribute, you might get warnings when it's casted to SkDescriptor
about differing alignment.

[bsalomon, 2012-01-12 21:45:06]

On 2012/01/12 21:39:37, jbates wrote:
> On 2012/01/12 21:36:36, jbates wrote:
> > On 2012/01/12 21:32:27, reed1 wrote:
> > > if true, that's wacky, since uint32_t is closing to ensuring proper
> alignement
> > > for SkDescriptor than uint8_t.
> > 
> > Good point -- you would also need to specify the alignment of the array with
> > __attribute__((__aligned__(alignof(SkDescriptor)))) for GCC and whatever the
> > MSVC variant is.
> 
> Although, since the array follows a pointer (fDesc), it's already going to be
> aligned at least 4 bytes (8 bytes for the 64-bit build). But without the
> alignment attribute, you might get warnings when it's casted to SkDescriptor
> about differing alignment.

According to this article it is safe to cast _to_ a char* but not _from_ a
char*:
http://cellperformance.beyond3d.com/articles/2006/06/understanding-strict-ali...

[jbates, 2012-01-12 21:53:25]

On 2012/01/12 21:45:06, bsalomon wrote:
> On 2012/01/12 21:39:37, jbates wrote:
> > On 2012/01/12 21:36:36, jbates wrote:
> > > On 2012/01/12 21:32:27, reed1 wrote:
> > > > if true, that's wacky, since uint32_t is closing to ensuring proper
> > alignement
> > > > for SkDescriptor than uint8_t.
> > > 
> > > Good point -- you would also need to specify the alignment of the array
with
> > > __attribute__((__aligned__(alignof(SkDescriptor)))) for GCC and whatever
the
> > > MSVC variant is.
> > 
> > Although, since the array follows a pointer (fDesc), it's already going to
be
> > aligned at least 4 bytes (8 bytes for the 64-bit build). But without the
> > alignment attribute, you might get warnings when it's casted to SkDescriptor
> > about differing alignment.
> 
> According to this article it is safe to cast _to_ a char* but not _from_ a
> char*:
>
http://cellperformance.beyond3d.com/articles/2006/06/understanding-strict-ali...

If you don't know the history of the char pointer you are casting from, then it
is dangerous. But in this case, the aliasing of SkDescriptor is one of two
cases: as a char* or as a SkDescriptor*. It looks like the example shown in Mike
Acton's article is showing that you can't alias a uint32 as uint16 and expect it
to be safe by casting through a char*.

[jbates, 2012-01-12 23:24:52]

On 2012/01/12 21:53:25, jbates wrote:
> On 2012/01/12 21:45:06, bsalomon wrote:
> > On 2012/01/12 21:39:37, jbates wrote:
> > > On 2012/01/12 21:36:36, jbates wrote:
> > > > On 2012/01/12 21:32:27, reed1 wrote:
> > > > > if true, that's wacky, since uint32_t is closing to ensuring proper
> > > alignement
> > > > > for SkDescriptor than uint8_t.
> > > > 
> > > > Good point -- you would also need to specify the alignment of the array
> with
> > > > __attribute__((__aligned__(alignof(SkDescriptor)))) for GCC and whatever
> the
> > > > MSVC variant is.
> > > 
> > > Although, since the array follows a pointer (fDesc), it's already going to
> be
> > > aligned at least 4 bytes (8 bytes for the 64-bit build). But without the
> > > alignment attribute, you might get warnings when it's casted to
SkDescriptor
> > > about differing alignment.
> > 
> > According to this article it is safe to cast _to_ a char* but not _from_ a
> > char*:
> >
>
http://cellperformance.beyond3d.com/articles/2006/06/understanding-strict-ali...
> 
> If you don't know the history of the char pointer you are casting from, then
it
> is dangerous. But in this case, the aliasing of SkDescriptor is one of two
> cases: as a char* or as a SkDescriptor*. It looks like the example shown in
Mike
> Acton's article is showing that you can't alias a uint32 as uint16 and expect
it
> to be safe by casting through a char*.

In any case, since this code never actually accesses the fStorage array
directly, there's no actual danger here even if the type is left as uint32. I
would go with joth's idea of placement new and see if the compiler warning goes
away.