Store mipmap levels in deferred texture image

Store mipmap levels in deferred texture image

This is a follow-up to https://codereview.chromium.org/2115023002/ and
https://codereview.chromium.org/2034933003/ which were reverted due to
an access violation and a memory leak, respectively.

When creating the deferred texture image, detect if using medium / high
quality. If so, generate and store mipmaps in the deferred texture
image.

When creating a texture from that be sure to read it back out.

BUG=578304
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2242883004

Committed: https://skia.googlesource.com/skia/+/33e0cb5e7f9926b96db209c825f1eeca7c15bb16

[cblume, 2016-08-12 18:39:16]

Description was changed from

==========
Store mipmap levels in deferred texture image

This is a follow-up to https://codereview.chromium.org/2115023002/ and
https://codereview.chromium.org/2034933003/ which were reverted due to
an access violation and a memory leak, respectively.

When creating the deferred texture image, detect if using medium / high
quality. If so, generate and store mipmaps in the deferred texture
image.

When creating a texture from that be sure to read it back out.

BUG=578304
==========

to

==========
Store mipmap levels in deferred texture image

This is a follow-up to https://codereview.chromium.org/2115023002/ and
https://codereview.chromium.org/2034933003/ which were reverted due to
an access violation and a memory leak, respectively.

When creating the deferred texture image, detect if using medium / high
quality. If so, generate and store mipmaps in the deferred texture
image.

When creating a texture from that be sure to read it back out.

BUG=578304
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2242883004
==========

[cblume, 2016-08-12 22:25:34]

cblume@chromium.org changed reviewers:
+ brianosman@google.com, bsalomon@google.com, ericrk@chromium.org

[cblume, 2016-08-12 22:25:36]

Pointer aliasing.
BTW, we have it in other spots as well. When testing locally I was able to get
other spots to trigger.

https://codereview.chromium.org/2242883004/diff/20001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2242883004/diff/20001/BUILD.gn#newcode546
BUILD.gn:546: cflags = [ "-fno-strict-aliasing" ]
I believe the problem as pointer aliasing.
This solves it, but in a way I do not like. I'm only using it to point out that
this seems to be the issue.

This makes GCC's optimization less aggressive.

https://codereview.chromium.org/2242883004/diff/20001/src/image/SkImage_Gpu.cpp
File src/image/SkImage_Gpu.cpp (right):

https://codereview.chromium.org/2242883004/diff/20001/src/image/SkImage_Gpu.c...
src/image/SkImage_Gpu.cpp:532: intptr_t bufferAsInt =
reinterpret_cast<intptr_t>(buffer);
The problem is we are remapping buffer to several different aliased pointers.
Here is one.

https://codereview.chromium.org/2242883004/diff/20001/src/image/SkImage_Gpu.c...
src/image/SkImage_Gpu.cpp:536: ct = reinterpret_cast<SkPMColor*>(bufferAsInt +
ctOffset);
Here is another.

https://codereview.chromium.org/2242883004/diff/20001/src/image/SkImage_Gpu.c...
src/image/SkImage_Gpu.cpp:546: DeferredTextureImage* dti = new (buffer)
DeferredTextureImage();
Here is a third.

https://codereview.chromium.org/2242883004/diff/20001/src/image/SkImage_Gpu.c...
src/image/SkImage_Gpu.cpp:554: dti->fColorTableData = ct;
At this point, 'ct' may not have been written because aliasing allowed the
assignment to be reordered. This could be setting nullptr even when a ct
existed.

https://codereview.chromium.org/2242883004/diff/20001/src/image/SkImage_Gpu.c...
src/image/SkImage_Gpu.cpp:559: dti->fColorSpace =
reinterpret_cast<void*>(bufferAsInt + colorSpaceOffset);
dti and bufferAsInt may be rearranged here. I'm not sure. We're casting it back
to void*.

https://codereview.chromium.org/2242883004/diff/20001/src/image/SkImage_Gpu.c...
src/image/SkImage_Gpu.cpp:568: intptr_t mipLevelPtr = bufferAsInt + pixelOffset
+ SkAlign8(SkAutoPixmapStorage::AllocSize(
This feeds off bufferAsInt and isn't introducing a new type to the pointer
aliasing. However, it feels the effects of the pointer aliasing.

https://codereview.chromium.org/2242883004/diff/20001/src/image/SkImage_Gpu.c...
src/image/SkImage_Gpu.cpp:584: SkASSERT(mipLevelPtr + 
mipLevel.fPixmap.getSafeSize() <= bufferAsInt + pixelOffset + pixelSize);
(I added some asserts to show the memcpy is sane.)

https://codereview.chromium.org/2242883004/diff/20001/src/image/SkImage_Gpu.c...
src/image/SkImage_Gpu.cpp:586: memcpy(reinterpret_cast<void*>(mipLevelPtr),
mipLevel.fPixmap.addr(),
Here, we cast mipLevelPtr back to void*.
The compiler only needs to worry about the intptr_t type's reads and writes. So
that means anything that was dealing with bufferAsInt will be correctly
read/written without reordering.

It is odd that we saw a crash on this line. We are only accessing one alias on
this line. I am unsure how to explain it other than "any aliasing = undefined
behavior and don't expect sane results".

https://codereview.chromium.org/2242883004/diff/20001/src/image/SkImage_Gpu.c...
src/image/SkImage_Gpu.cpp:589: reinterpret_cast<void*>(mipLevelPtr);
dti's and mipLevelPtr's reads and writes can be rearranged by the compiler. This
could be doing (this_type)(nullptr)-> or this->fPixelData = nullptr;

https://codereview.chromium.org/2242883004/diff/20001/src/image/SkImage_Gpu.c...
src/image/SkImage_Gpu.cpp:592: mipLevelPtr +=
SkAlign8(mipLevel.fPixmap.getSafeSize());
Or this line could be reordered above the dti->blah = mipLevelPtr;

[cblume, 2016-08-12 23:01:04]

Oh also, adding to the pointer aliasing evidence: the compiles which failed were
all GCC, which is more aggressive with pointer aliasing.

[bsalomon, 2016-08-13 00:12:42]

bsalomon@google.com changed reviewers:
+ mtklein@google.com

[bsalomon, 2016-08-13 00:12:42]

+mtklein. I believe our policy is to compile without --no-strict-aliasing and
use SkTCast (which AFAIK defeats strict aliasing on all the compilers we
support).

[cblume, 2016-08-13 00:14:32]

On 2016/08/13 00:12:42, bsalomon wrote:
> +mtklein. I believe our policy is to compile without --no-strict-aliasing and
> use SkTCast (which AFAIK defeats strict aliasing on all the compilers we
> support).

I like the policy of not using -fno-strict-aliasing. It allows for more
optimization. :)
I wanted to point this step out before the next step which would remove the
aliasing.
I'll look into SkTCast.

[mtklein, 2016-08-14 14:06:24]

https://codereview.chromium.org/2242883004/diff/20001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2242883004/diff/20001/BUILD.gn#newcode546
BUILD.gn:546: cflags = [ "-fno-strict-aliasing" ]
On 2016/08/12 22:25:35, cblume wrote:
> I believe the problem as pointer aliasing.
> This solves it, but in a way I do not like. I'm only using it to point out
that
> this seems to be the issue.
> 
> This makes GCC's optimization less aggressive.

Right, we turn on -fstrict-aliasing in our test bots even more often than GCC
does naturally, to be compatible with clients who may use it.  It's the same
reason we turn off RTTI and exceptions.  To my knowledge, though, the clients we
pay most attention to disable strict aliasing because it's too hard or subtle a
constraint for developers to manage.  I tend to agree, except that's pretty
clear you want to be able to assume nothing aliases 'this'.  If the compilers
were better about warning about when a strict-aliasing related optimization
fired, it'd also be less of an issue... mostly the problem is it's a silent
killer.

Are you sure this here fixes your problem?  This flag will apply only to those 3
files listed above, right?  And only if you're using GN... most people are using
GYP.

SkTCast looks well-intentioned but, I think, incorrect.  That union is a fine
implementation-defined way of aliasing its elements (non-standard, but supported
by the compilers we use).  However, it doesn't guarantee anything about the
aliasing of the pointed-to elements.  It's OK to use a union to pun a float to
an int (e.g. SkFloatIntUnion), but it doesn't help you to pun a float* to and
int*.

memcpy() is always a safe choice, but again it's got to be done on the data
itself, not as a roundabout reinterpret_cast on the pointer.  It's not helpful
if you want to pun the data in-place.

The only other way I know to fix this is to introduce an aliased variant of the
type you're working with, using __attribute__((may_alias)).  It bestows on that
type the same special property that char has, which lets memcpy() actually be
implemented.  And this is how GCC manages its own aliased builtin types, like
__m128i.  You can take a look at SkChecksum_opts.h:91 to see may_alias in
action.  To my knowledge, only GCC does dirty things with aliasing... Clang will
accept the attribute but already wasn't optimizing in any unexpected way, and
MSVC doesn't seem to try to exploit strict aliasing in this way either.

When you say,
  "BTW, we have it in other spots as well. When testing locally I was able to
get other spots to trigger."
can you list those other spots, or show us how you found them?  I'd like to get
bots up to guard against this.

[mtklein, 2016-08-14 14:10:42]

> The only other way I know to fix this is to introduce an aliased variant of
the
> type you're working with, using __attribute__((may_alias)).  It bestows on
that
> type the same special property that char has, which lets memcpy() actually be
> implemented.  And this is how GCC manages its own aliased builtin types, like
> __m128i.  You can take a look at SkChecksum_opts.h:91 to see may_alias in
> action.  To my knowledge, only GCC does dirty things with aliasing... Clang
will
> accept the attribute but already wasn't optimizing in any unexpected way, and
> MSVC doesn't seem to try to exploit strict aliasing in this way either.

Just thought of another complication!

This may_alias-implementation of hash_fn() dates from a time when hash_fn() was
often inlined.
This aliasing paranoia may be moot now that this code is accessed through a
call... I don't
think the problem was so much aliasing uint32_t and uint8_t (that's supposed to
be explicitly
legal) but rather aliasing T and uint32_t, for whatever T's address got passed
in as data.

[cblume, 2016-08-15 23:39:48]

https://codereview.chromium.org/2242883004/diff/20001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2242883004/diff/20001/BUILD.gn#newcode546
BUILD.gn:546: cflags = [ "-fno-strict-aliasing" ]
On 2016/08/14 14:06:24, mtklein wrote:
> Are you sure this here fixes your problem?  This flag will apply only to those
3
> files listed above, right?  And only if you're using GN... most people are
using
> GYP.

Oh huh. Maybe the bot runs passed just by chance then. If this only effects
those 3 files then this shouldn't fix anything.

> SkTCast looks well-intentioned but, I think, incorrect.  That union is a fine
> implementation-defined way of aliasing its elements (non-standard, but
supported
> by the compilers we use).  However, it doesn't guarantee anything about the
> aliasing of the pointed-to elements.  It's OK to use a union to pun a float to
> an int (e.g. SkFloatIntUnion), but it doesn't help you to pun a float* to and
> int*.

I agree. I just took a look at SkTCast and I'm uncomfortable with it. When you
write one type of a union, you can only legally read from that type. Anything
else is undefined behavior. But SkTCast uses the C behavior (not C++ behavior)
where you can use a union in this way.

As far as I know, all the major compilers don't take advantage of this undefined
behavior in any way and they behave like C does. But I'm still not a huge fan. I
guess it would at least put the undefined behavior in one place so if it becomes
a problem you can update all call sites.

But all that aside, you are right that it would still be aliasing when used with
pointers.


> To my knowledge, only GCC does dirty things with aliasing... Clang will
> accept the attribute but already wasn't optimizing in any unexpected way, and
> MSVC doesn't seem to try to exploit strict aliasing in this way either.

I agree. I still feel confident the problem is aliasing. It is only showing up
on GCC builds. Even if my -fno-strict-aliasing didn't apply.

> When you say,
>   "BTW, we have it in other spots as well. When testing locally I was able to
> get other spots to trigger."
> can you list those other spots, or show us how you found them?  I'd like to
get
> bots up to guard against this.

Oh, I thought this was an access violation but it might actually be an unaligned
pointer plus an instruction that requires aligned data.

In the stack below you'll see dst was 0xb399ab14 (not 16-byte aligned) and it
looks like dst1 = dst.
https://cs.chromium.org/chromium/src/third_party/skia/src/opts/SkTextureCompr...

Get a fresh checkout of Skia (no changes)
These instructions are for Debug but it happens in Release as well.

$ adb devices
List of devices attached
(my nexus 6 serial here)      device
(Just to sanity check that I'm not targeting the wrong device)

$ ./platform_tools/android/bin/android_ninja -d nexus_6 -t Debug --gcc

$ ./platform_tools/android/bin/android_gdb_native --debug dm --resourcePath
/data/local/tmp/skia/resources

Thread 2 "skia_launcher" received signal SIGBUS, Bus error.
[Switching to Thread 6837.6931]
0xb5cdc950 in vst1q_u64 (__b=..., __a=0xb399ab14)
    at
/chromium-dev/skia/platform_tools/android/toolchains/arm-r12b-14/lib/gcc/arm-linux-androideabi/4.9.x/include/arm_neon.h:9161
9161      __builtin_neon_vst1v2di ((__builtin_neon_di *) __a, (int64x2_t) __b);
(gdb) info stack
#0  0xb5cdc950 in vst1q_u64 (__b=..., __a=0xb399ab14)
    at
/chromium-dev/skia/platform_tools/android/toolchains/arm-r12b-14/lib/gcc/arm-linux-androideabi/4.9.x/include/arm_neon.h:9161
#1  neon::compress_r11eac_blocks (dst=0xb399ab14, src=0xb394b800 "",
rowBytes=48)
    at ../../../../src/opts/SkTextureCompressor_opts.h:208
#2  0xb5cdca0e in neon::compress_a8_r11eac (dst=0xb399ab14 "\377",
src=0xb394b800 "", 
    width=48, height=48, rowBytes=48)
    at ../../../../src/opts/SkTextureCompressor_opts.h:231
#3  0xb5eaf2b2 in SkTextureCompressor::CompressBufferToFormat (dst=0xb399ab14
"\377", 
    src=0xb394b800 "", srcColorType=kAlpha_8_SkColorType, width=48, height=48, 
    rowBytes=48, format=SkTextureCompressor::kR11_EAC_Format)
    at ../../../../src/utils/SkTextureCompressor.cpp:125
#4  0xb5eaf3ec in SkTextureCompressor::CompressBitmapToFormat (pixmap=..., 
    format=SkTextureCompressor::kR11_EAC_Format)
    at ../../../../src/utils/SkTextureCompressor.cpp:158
#5  0xb4c60c58 in test_CompressCheckerboard (reporter=0xb6d78d04)
    at ../../../../tests/TextureCompressionTest.cpp:157
#6  0xb4946176 in run_test (test=...) at ../../../../dm/DM.cpp:1248
#7  0xb49462e6 in <lambda()>::operator()(void) const (__closure=0xb67ff070)
    at ../../../../dm/DM.cpp:1347
#8  0xb494751a in std::_Function_handler<void(), dm_main()::<lambda()>
>::_M_invoke(const std::_Any_data &) (__functor=...)
    at
/chromium-dev/skia/platform_tools/android/toolchains/arm-r12b-14/include/c++/4.9.x/functional:2039
#9  0xb5d697b0 in std::function<void ()>::operator()() const (this=0xb6d78d54)
    at
/chromium-dev/skia/platform_tools/android/toolchains/arm-r12b-14/include/c++/4.9.x/functional:2439
#10 0xb5d68e30 in (anonymous namespace)::ThreadPool::Loop (arg=0xb646f070)
    at ../../../../src/core/SkTaskGroup.cpp:167
#11 0xb5eb8c5c in thread_start (arg=0xb647e040)
    at ../../../../src/utils/SkThreadUtils_pthread.cpp:66
#12 0xb6f69bb0 in __pthread_start(void*) ()
   from /chromium-dev/skia/out/config/android-nexus_6/android_gdb_tmp/libc.so
#13 0xb6f67af4 in __start_thread ()
   from /chromium-dev/skia/out/config/android-nexus_6/android_gdb_tmp/libc.so
#14 0x00000000 in ?? ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)

https://codereview.chromium.org/2242883004/diff/20001/src/image/SkImage_Gpu.cpp
File src/image/SkImage_Gpu.cpp (right):

https://codereview.chromium.org/2242883004/diff/20001/src/image/SkImage_Gpu.c...
src/image/SkImage_Gpu.cpp:532: intptr_t bufferAsInt =
reinterpret_cast<intptr_t>(buffer);
On 2016/08/12 22:25:35, cblume wrote:
> The problem is we are remapping buffer to several different aliased pointers.
> Here is one.

Sorry, I said this wrong.
This is not an instance of aliasing.
intptr_t is type integer, large enough to hold a pointer. It can be read
similarly to int32_t / int(size)_t.
It is a cast to an int, not a pointer.

https://codereview.chromium.org/2242883004/diff/20001/src/image/SkImage_Gpu.c...
src/image/SkImage_Gpu.cpp:554: dti->fColorTableData = ct;
On 2016/08/12 22:25:35, cblume wrote:
> At this point, 'ct' may not have been written because aliasing allowed the
> assignment to be reordered. This could be setting nullptr even when a ct
> existed.

I think this might be the bigger issue.
At this point, I think dti's and ct's assignments might possibly be rearranged.

[mtklein, 2016-08-16 02:22:23]

https://codereview.chromium.org/2242883004/diff/20001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2242883004/diff/20001/BUILD.gn#newcode546
BUILD.gn:546: cflags = [ "-fno-strict-aliasing" ]
> I agree. I just took a look at SkTCast and I'm uncomfortable with it. When you
> write one type of a union, you can only legally read from that type. Anything
> else is undefined behavior. But SkTCast uses the C behavior (not C++ behavior)
> where you can use a union in this way.
> 
> As far as I know, all the major compilers don't take advantage of this
undefined
> behavior in any way and they behave like C does. But I'm still not a huge fan.
I
> guess it would at least put the undefined behavior in one place so if it
becomes
> a problem you can update all call sites.
> 
> But all that aside, you are right that it would still be aliasing when used
with
> pointers.

So glad to have you on board, but I don't think this is quite the right way to
think about this.

Using a a field of a union that's not the most recently assigned is undefined
behavior in C and C++.  GCC and MSVC and Clang all break with the standard and
define this behavior as what you think it'd do.  The C++11 standard has chilled
out a little about what can go in unions, but is still totally uptight about
mixed field access, man.

MSVC and Clang seem to take their union trick further an extend this to
reinterpret_casts and C-style casts.  As far as I've seen, they do what you'd
think they do there.  GCC seems willing to apply strict aliasing rules here even
when it can clearly see you're asking to violate them.

All these compilers will happily let you pun float to int with a union.  GCC
will let you pun float* to int*, but now strict aliasing (of pointers) gets
involved and GCC invokes the "that's undefined, therefore didn't happen" rule if
you dereference that int*.  False implies true, pigs fly, the compiler does
whatever it wants.

https://codereview.chromium.org/2242883004/diff/20001/BUILD.gn#newcode546
BUILD.gn:546: cflags = [ "-fno-strict-aliasing" ]
On 2016/08/15 23:39:47, cblume wrote:
> On 2016/08/14 14:06:24, mtklein wrote:
> > Are you sure this here fixes your problem?  This flag will apply only to
those
> 3
> > files listed above, right?  And only if you're using GN... most people are
> using
> > GYP.
> 
> Oh huh. Maybe the bot runs passed just by chance then. If this only effects
> those 3 files then this shouldn't fix anything.
> 
> > SkTCast looks well-intentioned but, I think, incorrect.  That union is a
fine
> > implementation-defined way of aliasing its elements (non-standard, but
> supported
> > by the compilers we use).  However, it doesn't guarantee anything about the
> > aliasing of the pointed-to elements.  It's OK to use a union to pun a float
to
> > an int (e.g. SkFloatIntUnion), but it doesn't help you to pun a float* to
and
> > int*.
> 
> I agree. I just took a look at SkTCast and I'm uncomfortable with it. When you
> write one type of a union, you can only legally read from that type. Anything
> else is undefined behavior. But SkTCast uses the C behavior (not C++ behavior)
> where you can use a union in this way.
> 
> As far as I know, all the major compilers don't take advantage of this
undefined
> behavior in any way and they behave like C does. But I'm still not a huge fan.
I
> guess it would at least put the undefined behavior in one place so if it
becomes
> a problem you can update all call sites.
> 
> But all that aside, you are right that it would still be aliasing when used
with
> pointers.
> 
> 
> > To my knowledge, only GCC does dirty things with aliasing... Clang will
> > accept the attribute but already wasn't optimizing in any unexpected way,
and
> > MSVC doesn't seem to try to exploit strict aliasing in this way either.
> 
> I agree. I still feel confident the problem is aliasing. It is only showing up
> on GCC builds. Even if my -fno-strict-aliasing didn't apply.
> 
> > When you say,
> >   "BTW, we have it in other spots as well. When testing locally I was able
to
> > get other spots to trigger."
> > can you list those other spots, or show us how you found them?  I'd like to
> get
> > bots up to guard against this.
> 
> Oh, I thought this was an access violation but it might actually be an
unaligned
> pointer plus an instruction that requires aligned data.
> 
> In the stack below you'll see dst was 0xb399ab14 (not 16-byte aligned) and it
> looks like dst1 = dst.
>
https://cs.chromium.org/chromium/src/third_party/skia/src/opts/SkTextureCompr...
> 
> Get a fresh checkout of Skia (no changes)
> These instructions are for Debug but it happens in Release as well.
> 
> $ adb devices
> List of devices attached
> (my nexus 6 serial here)      device
> (Just to sanity check that I'm not targeting the wrong device)
> 
> $ ./platform_tools/android/bin/android_ninja -d nexus_6 -t Debug --gcc
> 
> $ ./platform_tools/android/bin/android_gdb_native --debug dm --resourcePath
> /data/local/tmp/skia/resources
> 
> Thread 2 "skia_launcher" received signal SIGBUS, Bus error.
> [Switching to Thread 6837.6931]
> 0xb5cdc950 in vst1q_u64 (__b=..., __a=0xb399ab14)
>     at
>
/chromium-dev/skia/platform_tools/android/toolchains/arm-r12b-14/lib/gcc/arm-linux-androideabi/4.9.x/include/arm_neon.h:9161
> 9161      __builtin_neon_vst1v2di ((__builtin_neon_di *) __a, (int64x2_t)
__b);
> (gdb) info stack
> #0  0xb5cdc950 in vst1q_u64 (__b=..., __a=0xb399ab14)
>     at
>
/chromium-dev/skia/platform_tools/android/toolchains/arm-r12b-14/lib/gcc/arm-linux-androideabi/4.9.x/include/arm_neon.h:9161
> #1  neon::compress_r11eac_blocks (dst=0xb399ab14, src=0xb394b800 "",
> rowBytes=48)
>     at ../../../../src/opts/SkTextureCompressor_opts.h:208
> #2  0xb5cdca0e in neon::compress_a8_r11eac (dst=0xb399ab14 "\377",
> src=0xb394b800 "", 
>     width=48, height=48, rowBytes=48)
>     at ../../../../src/opts/SkTextureCompressor_opts.h:231
> #3  0xb5eaf2b2 in SkTextureCompressor::CompressBufferToFormat (dst=0xb399ab14
> "\377", 
>     src=0xb394b800 "", srcColorType=kAlpha_8_SkColorType, width=48, height=48,

>     rowBytes=48, format=SkTextureCompressor::kR11_EAC_Format)
>     at ../../../../src/utils/SkTextureCompressor.cpp:125
> #4  0xb5eaf3ec in SkTextureCompressor::CompressBitmapToFormat (pixmap=..., 
>     format=SkTextureCompressor::kR11_EAC_Format)
>     at ../../../../src/utils/SkTextureCompressor.cpp:158
> #5  0xb4c60c58 in test_CompressCheckerboard (reporter=0xb6d78d04)
>     at ../../../../tests/TextureCompressionTest.cpp:157
> #6  0xb4946176 in run_test (test=...) at ../../../../dm/DM.cpp:1248
> #7  0xb49462e6 in <lambda()>::operator()(void) const (__closure=0xb67ff070)
>     at ../../../../dm/DM.cpp:1347
> #8  0xb494751a in std::_Function_handler<void(), dm_main()::<lambda()>
> >::_M_invoke(const std::_Any_data &) (__functor=...)
>     at
>
/chromium-dev/skia/platform_tools/android/toolchains/arm-r12b-14/include/c++/4.9.x/functional:2039
> #9  0xb5d697b0 in std::function<void ()>::operator()() const (this=0xb6d78d54)
>     at
>
/chromium-dev/skia/platform_tools/android/toolchains/arm-r12b-14/include/c++/4.9.x/functional:2439
> #10 0xb5d68e30 in (anonymous namespace)::ThreadPool::Loop (arg=0xb646f070)
>     at ../../../../src/core/SkTaskGroup.cpp:167
> #11 0xb5eb8c5c in thread_start (arg=0xb647e040)
>     at ../../../../src/utils/SkThreadUtils_pthread.cpp:66
> #12 0xb6f69bb0 in __pthread_start(void*) ()
>    from /chromium-dev/skia/out/config/android-nexus_6/android_gdb_tmp/libc.so
> #13 0xb6f67af4 in __start_thread ()
>    from /chromium-dev/skia/out/config/android-nexus_6/android_gdb_tmp/libc.so
> #14 0x00000000 in ?? ()
> Backtrace stopped: previous frame identical to this frame (corrupt stack?)

Yes, I agree the proximal problem here is that vst1q_u64() requires 8-byte
alignment, and 0xb399ab14 only has 4.  I bet you'll find 50%+ of runs pass
without changing anything.  :)

A vreinterpretq_uxx_u64() and using vst1q_u32() or vst1q_u8() will probably fix
this.

I just had to disable some unit tests of this texture compression code today. 
Is it used?

[cblume, 2016-08-16 03:47:19]

https://codereview.chromium.org/2242883004/diff/20001/BUILD.gn
File BUILD.gn (right):

https://codereview.chromium.org/2242883004/diff/20001/BUILD.gn#newcode546
BUILD.gn:546: cflags = [ "-fno-strict-aliasing" ]
On 2016/08/16 02:22:22, mtklein wrote:
> > I agree. I just took a look at SkTCast and I'm uncomfortable with it. When
you
> > write one type of a union, you can only legally read from that type.
Anything
> > else is undefined behavior. But SkTCast uses the C behavior (not C++
behavior)
> > where you can use a union in this way.
> > 
> > As far as I know, all the major compilers don't take advantage of this
> undefined
> > behavior in any way and they behave like C does. But I'm still not a huge
fan.
> I
> > guess it would at least put the undefined behavior in one place so if it
> becomes
> > a problem you can update all call sites.
> > 
> > But all that aside, you are right that it would still be aliasing when used
> with
> > pointers.
> 
> So glad to have you on board, but I don't think this is quite the right way to
> think about this.
> 
> Using a a field of a union that's not the most recently assigned is undefined
> behavior in C and C++.  GCC and MSVC and Clang all break with the standard and
> define this behavior as what you think it'd do.  The C++11 standard has
chilled
> out a little about what can go in unions, but is still totally uptight about
> mixed field access, man.
> 
> MSVC and Clang seem to take their union trick further an extend this to
> reinterpret_casts and C-style casts.  As far as I've seen, they do what you'd
> think they do there.  GCC seems willing to apply strict aliasing rules here
even
> when it can clearly see you're asking to violate them.
> 
> All these compilers will happily let you pun float to int with a union.  GCC
> will let you pun float* to int*, but now strict aliasing (of pointers) gets
> involved and GCC invokes the "that's undefined, therefore didn't happen" rule
if
> you dereference that int*.  False implies true, pigs fly, the compiler does
> whatever it wants.

Mmmm I think C and C++ differ in their union behavior.
http://en.cppreference.com/w/c/language/union (notice, this is the C area of
cppreference)

"If the member used to access the contents of a union is not the same as the
member last used to store a value, the object representation of the value that
was stored is reinterpreted as an object representation of the new type (this is
known as type punning)."


But I didn't know that MSVC/GCC/Clang all formally stated they would define the
behavior. I thought it just acted as expected but we weren't promised that. It
is nice to know they clarified this. :)

https://codereview.chromium.org/2242883004/diff/20001/BUILD.gn#newcode546
BUILD.gn:546: cflags = [ "-fno-strict-aliasing" ]
On 2016/08/16 02:22:22, mtklein wrote:
> On 2016/08/15 23:39:47, cblume wrote:
> > On 2016/08/14 14:06:24, mtklein wrote:
> > > Are you sure this here fixes your problem?  This flag will apply only to
> those
> > 3
> > > files listed above, right?  And only if you're using GN... most people are
> > using
> > > GYP.
> > 
> > Oh huh. Maybe the bot runs passed just by chance then. If this only effects
> > those 3 files then this shouldn't fix anything.
> > 
> > > SkTCast looks well-intentioned but, I think, incorrect.  That union is a
> fine
> > > implementation-defined way of aliasing its elements (non-standard, but
> > supported
> > > by the compilers we use).  However, it doesn't guarantee anything about
the
> > > aliasing of the pointed-to elements.  It's OK to use a union to pun a
float
> to
> > > an int (e.g. SkFloatIntUnion), but it doesn't help you to pun a float* to
> and
> > > int*.
> > 
> > I agree. I just took a look at SkTCast and I'm uncomfortable with it. When
you
> > write one type of a union, you can only legally read from that type.
Anything
> > else is undefined behavior. But SkTCast uses the C behavior (not C++
behavior)
> > where you can use a union in this way.
> > 
> > As far as I know, all the major compilers don't take advantage of this
> undefined
> > behavior in any way and they behave like C does. But I'm still not a huge
fan.
> I
> > guess it would at least put the undefined behavior in one place so if it
> becomes
> > a problem you can update all call sites.
> > 
> > But all that aside, you are right that it would still be aliasing when used
> with
> > pointers.
> > 
> > 
> > > To my knowledge, only GCC does dirty things with aliasing... Clang will
> > > accept the attribute but already wasn't optimizing in any unexpected way,
> and
> > > MSVC doesn't seem to try to exploit strict aliasing in this way either.
> > 
> > I agree. I still feel confident the problem is aliasing. It is only showing
up
> > on GCC builds. Even if my -fno-strict-aliasing didn't apply.
> > 
> > > When you say,
> > >   "BTW, we have it in other spots as well. When testing locally I was able
> to
> > > get other spots to trigger."
> > > can you list those other spots, or show us how you found them?  I'd like
to
> > get
> > > bots up to guard against this.
> > 
> > Oh, I thought this was an access violation but it might actually be an
> unaligned
> > pointer plus an instruction that requires aligned data.
> > 
> > In the stack below you'll see dst was 0xb399ab14 (not 16-byte aligned) and
it
> > looks like dst1 = dst.
> >
>
https://cs.chromium.org/chromium/src/third_party/skia/src/opts/SkTextureCompr...
> > 
> > Get a fresh checkout of Skia (no changes)
> > These instructions are for Debug but it happens in Release as well.
> > 
> > $ adb devices
> > List of devices attached
> > (my nexus 6 serial here)      device
> > (Just to sanity check that I'm not targeting the wrong device)
> > 
> > $ ./platform_tools/android/bin/android_ninja -d nexus_6 -t Debug --gcc
> > 
> > $ ./platform_tools/android/bin/android_gdb_native --debug dm --resourcePath
> > /data/local/tmp/skia/resources
> > 
> > Thread 2 "skia_launcher" received signal SIGBUS, Bus error.
> > [Switching to Thread 6837.6931]
> > 0xb5cdc950 in vst1q_u64 (__b=..., __a=0xb399ab14)
> >     at
> >
>
/chromium-dev/skia/platform_tools/android/toolchains/arm-r12b-14/lib/gcc/arm-linux-androideabi/4.9.x/include/arm_neon.h:9161
> > 9161      __builtin_neon_vst1v2di ((__builtin_neon_di *) __a, (int64x2_t)
> __b);
> > (gdb) info stack
> > #0  0xb5cdc950 in vst1q_u64 (__b=..., __a=0xb399ab14)
> >     at
> >
>
/chromium-dev/skia/platform_tools/android/toolchains/arm-r12b-14/lib/gcc/arm-linux-androideabi/4.9.x/include/arm_neon.h:9161
> > #1  neon::compress_r11eac_blocks (dst=0xb399ab14, src=0xb394b800 "",
> > rowBytes=48)
> >     at ../../../../src/opts/SkTextureCompressor_opts.h:208
> > #2  0xb5cdca0e in neon::compress_a8_r11eac (dst=0xb399ab14 "\377",
> > src=0xb394b800 "", 
> >     width=48, height=48, rowBytes=48)
> >     at ../../../../src/opts/SkTextureCompressor_opts.h:231
> > #3  0xb5eaf2b2 in SkTextureCompressor::CompressBufferToFormat
(dst=0xb399ab14
> > "\377", 
> >     src=0xb394b800 "", srcColorType=kAlpha_8_SkColorType, width=48,
height=48,
> 
> >     rowBytes=48, format=SkTextureCompressor::kR11_EAC_Format)
> >     at ../../../../src/utils/SkTextureCompressor.cpp:125
> > #4  0xb5eaf3ec in SkTextureCompressor::CompressBitmapToFormat (pixmap=..., 
> >     format=SkTextureCompressor::kR11_EAC_Format)
> >     at ../../../../src/utils/SkTextureCompressor.cpp:158
> > #5  0xb4c60c58 in test_CompressCheckerboard (reporter=0xb6d78d04)
> >     at ../../../../tests/TextureCompressionTest.cpp:157
> > #6  0xb4946176 in run_test (test=...) at ../../../../dm/DM.cpp:1248
> > #7  0xb49462e6 in <lambda()>::operator()(void) const (__closure=0xb67ff070)
> >     at ../../../../dm/DM.cpp:1347
> > #8  0xb494751a in std::_Function_handler<void(), dm_main()::<lambda()>
> > >::_M_invoke(const std::_Any_data &) (__functor=...)
> >     at
> >
>
/chromium-dev/skia/platform_tools/android/toolchains/arm-r12b-14/include/c++/4.9.x/functional:2039
> > #9  0xb5d697b0 in std::function<void ()>::operator()() const
(this=0xb6d78d54)
> >     at
> >
>
/chromium-dev/skia/platform_tools/android/toolchains/arm-r12b-14/include/c++/4.9.x/functional:2439
> > #10 0xb5d68e30 in (anonymous namespace)::ThreadPool::Loop (arg=0xb646f070)
> >     at ../../../../src/core/SkTaskGroup.cpp:167
> > #11 0xb5eb8c5c in thread_start (arg=0xb647e040)
> >     at ../../../../src/utils/SkThreadUtils_pthread.cpp:66
> > #12 0xb6f69bb0 in __pthread_start(void*) ()
> >    from
/chromium-dev/skia/out/config/android-nexus_6/android_gdb_tmp/libc.so
> > #13 0xb6f67af4 in __start_thread ()
> >    from
/chromium-dev/skia/out/config/android-nexus_6/android_gdb_tmp/libc.so
> > #14 0x00000000 in ?? ()
> > Backtrace stopped: previous frame identical to this frame (corrupt stack?)
> 
> Yes, I agree the proximal problem here is that vst1q_u64() requires 8-byte
> alignment, and 0xb399ab14 only has 4.  I bet you'll find 50%+ of runs pass
> without changing anything.  :)
> 
> A vreinterpretq_uxx_u64() and using vst1q_u32() or vst1q_u8() will probably
fix
> this.
> 
> I just had to disable some unit tests of this texture compression code today. 
> Is it used?

I have no idea if it is used. I was running into this (and also an illegal
instruction) via DM when trying to reproduce my error.


Speaking of, my error seemed to reproduce reliably every time on the bots
(making the revert clear). But I cannot reproduce it locally at all.

[mtklein, 2016-08-16 12:33:38]

Ah, neat, C99 defined union punning.  Thanks for pointing that out.

[cblume, 2016-08-17 16:05:33]

I can't seem to make it crash anymore.
But the only changes I made were adding some printfs.

I wonder if this causes the compiler to optimize less since printf has side
effects on the variables I'm printing?

[mtklein, 2016-08-17 16:16:22]

On 2016/08/17 16:05:33, cblume wrote:
> I can't seem to make it crash anymore.
> But the only changes I made were adding some printfs.
> 
> I wonder if this causes the compiler to optimize less since printf has side
> effects on the variables I'm printing?

Alignment issues are pretty whimsical.  It really depends where and how you've
allocated the memory that it's crashing on.

[cblume, 2016-08-17 16:18:04]

On 2016/08/17 16:05:33, cblume wrote:
> I can't seem to make it crash anymore.
> But the only changes I made were adding some printfs.
> 
> I wonder if this causes the compiler to optimize less since printf has side
> effects on the variables I'm printing?

I can't really make sense of it because PS5 errored and PS6 didn't. But the
difference between the two doesn't touch anything that I think the compiler
would have optimized into something unexpected. Is it possible we still have
some undefined behavior somewhere and the extra printfs simply caused the
compiler to produce better output?

[mtklein, 2016-08-17 16:19:22]

On 2016/08/17 16:18:04, cblume wrote:
> On 2016/08/17 16:05:33, cblume wrote:
> > I can't seem to make it crash anymore.
> > But the only changes I made were adding some printfs.
> > 
> > I wonder if this causes the compiler to optimize less since printf has side
> > effects on the variables I'm printing?
> 
> I can't really make sense of it because PS5 errored and PS6 didn't. But the
> difference between the two doesn't touch anything that I think the compiler
> would have optimized into something unexpected. Is it possible we still have
> some undefined behavior somewhere and the extra printfs simply caused the
> compiler to produce better output?

Yep.

[cblume, 2016-08-28 01:08:16]

The CQ bit was checked by cblume@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-28 01:08:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-28 01:17:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-28 01:17:11]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-GN-Trybot on
master.client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...)

[cblume, 2016-08-28 08:28:06]

The CQ bit was checked by cblume@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-28 08:28:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-28 08:57:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-28 08:57:29]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[cblume, 2016-08-28 09:28:14]

The CQ bit was checked by cblume@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-28 09:28:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[cblume, 2016-08-28 09:31:31]

ericrk: PTAL at https://codereview.chromium.org/2285013003
It remove's Chrome's usage of the default ctor for
DeferredTextureImageUsageParams. That must land before this patch (which removes
the default ctor).

bsalomon: PTAL at this patch. It should be good to go.


Summary of changes:
- There was a pointer aliasing issue which would reliably crash on release
builds. That has been fixed.
- Fixing it revealed a second problem: The default ctor of
DeferredTextureImageUsageParams was not setting all of the members. This would
cause an occasional crash on debug/release builds.
- I considered having the default ctor set all the members. But I would prefer
not to allow objects in invalid states. So I instead got rid of the default
ctor.
- Other than that, it does what it was originally designed to do: Store the
mipmap levels in a deferred texture image.

[cblume, 2016-08-28 09:32:23]

Oh, also, the mac chromium bot will fail because the chromium change hasn't
landed which removes chromium's usage of the default ctor.

Once we land that other patch the mac chromium bot will be green.

[commit-bot: I haz the power, 2016-08-28 10:10:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-28 10:10:30]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[bsalomon, 2016-08-29 15:49:31]

https://codereview.chromium.org/2242883004/diff/270001/src/image/SkImage_Gpu.cpp
File src/image/SkImage_Gpu.cpp (right):

https://codereview.chromium.org/2242883004/diff/270001/src/image/SkImage_Gpu....
src/image/SkImage_Gpu.cpp:547:
SkASSERT(std::is_standard_layout<DeferredTextureImage>::value);
Can this be a static assert?

https://codereview.chromium.org/2242883004/diff/270001/src/image/SkImage_Gpu....
src/image/SkImage_Gpu.cpp:548: memcpy(reinterpret_cast<void*>(bufferAsInt +
offsetof(DeferredTextureImage, fGammaTreatment)),
Is there some way to write this as a template based on the field? Or something
like that? Just trying to isolate the pattern and make this a little more
readable.

[cblume, 2016-08-29 21:52:58]

https://codereview.chromium.org/2242883004/diff/270001/src/image/SkImage_Gpu.cpp
File src/image/SkImage_Gpu.cpp (right):

https://codereview.chromium.org/2242883004/diff/270001/src/image/SkImage_Gpu....
src/image/SkImage_Gpu.cpp:547:
SkASSERT(std::is_standard_layout<DeferredTextureImage>::value);
On 2016/08/29 15:49:31, bsalomon wrote:
> Can this be a static assert?

Done.

https://codereview.chromium.org/2242883004/diff/270001/src/image/SkImage_Gpu....
src/image/SkImage_Gpu.cpp:548: memcpy(reinterpret_cast<void*>(bufferAsInt +
offsetof(DeferredTextureImage, fGammaTreatment)),
On 2016/08/29 15:49:31, bsalomon wrote:
> Is there some way to write this as a template based on the field? Or something
> like that? Just trying to isolate the pattern and make this a little more
> readable.

I simplified it where I could.

To do this, I introduced a class and a macro. I think it reads nicely though.

In some instances I wasn't able to use the class/macro combo nicely. So those
remain difficult to read. But the majority is simple and clear now.

[bsalomon, 2016-08-30 13:15:30]

lgtm, minor style nit for the new class: we don't give opening parens their own
lines.

[cblume, 2016-08-30 17:59:22]

The CQ bit was checked by cblume@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-30 17:59:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[cblume, 2016-08-30 17:59:30]

The CQ bit was unchecked by cblume@chromium.org

[cblume, 2016-08-30 18:49:08]

The CQ bit was checked by cblume@chromium.org

[cblume, 2016-08-30 18:49:08]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/2242883004/#ps310001
(title: "Fixing opening brace location.")

[commit-bot: I haz the power, 2016-08-30 18:49:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 19:09:25]

Description was changed from

==========
Store mipmap levels in deferred texture image

This is a follow-up to https://codereview.chromium.org/2115023002/ and
https://codereview.chromium.org/2034933003/ which were reverted due to
an access violation and a memory leak, respectively.

When creating the deferred texture image, detect if using medium / high
quality. If so, generate and store mipmaps in the deferred texture
image.

When creating a texture from that be sure to read it back out.

BUG=578304
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2242883004
==========

to

==========
Store mipmap levels in deferred texture image

This is a follow-up to https://codereview.chromium.org/2115023002/ and
https://codereview.chromium.org/2034933003/ which were reverted due to
an access violation and a memory leak, respectively.

When creating the deferred texture image, detect if using medium / high
quality. If so, generate and store mipmaps in the deferred texture
image.

When creating a texture from that be sure to read it back out.

BUG=578304
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2242883004

Committed:
https://skia.googlesource.com/skia/+/33e0cb5e7f9926b96db209c825f1eeca7c15bb16
==========

[commit-bot: I haz the power, 2016-08-30 19:09:26]

Committed patchset #17 (id:310001) as
https://skia.googlesource.com/skia/+/33e0cb5e7f9926b96db209c825f1eeca7c15bb16