MIPS32r2: Fix Chromium runtime crash

MIPS32r2: Fix Chromium runtime crash

Crash is caused by ldxc1 instruction, which traps when double values are
not aligned on 8-byte boundaries. Problem was tracked to SkChunkAlloc which
produces pointers aligned on 4-byte boundaries leading to misalignment.

This change makes sure that SkChunkAlloc will produce pointers that are 
aligned to 8 bytes.

Appropriate tests are added to tests/MemsetTest.cpp

TEST=Build Chromium with Clang and run on MIPS32r2 platform
TEST=./out/Debug/dm --match Memset
BUG=130022
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1849183004

Committed: https://skia.googlesource.com/skia/+/401e77cfe126eea1a12bd81410871fdb80739283

[lmilko, 2016-04-01 12:42:53]

Description was changed from

==========
MIPS32r2: Fix Chromium runtime crash

Crash is caused by ldxc1 instruction, which traps when double values are
not aligned on 8-byte boundaries. Problem was tracked to SkChunkAlloc which
produces pointers aligned on 4-byte boundaries leading to misalignment.

This change makes sure that SkChunkAlloc will produce pointers that are
aligned to 8 bytes.

TEST=Build Chromium with Clang and run on MIPS32r2 platform
BUG=130022
==========

to

==========
MIPS32r2: Fix Chromium runtime crash

Crash is caused by ldxc1 instruction, which traps when double values are
not aligned on 8-byte boundaries. Problem was tracked to SkChunkAlloc which
produces pointers aligned on 4-byte boundaries leading to misalignment.

This change makes sure that SkChunkAlloc will produce pointers that are
aligned to 8 bytes.

TEST=Build Chromium with Clang and run on MIPS32r2 platform
BUG=130022
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[lmilko, 2016-04-01 12:45:34]

milko.leporis@imgtec.com changed reviewers:
+ reed@google.com

[lmilko, 2016-04-01 12:45:35]

Please take a look

[lmilko, 2016-04-08 11:57:07]

On 2016/04/01 12:45:35, lmilko wrote:
> Please take a look

ping

[lmilko, 2016-04-14 13:19:07]

milko.leporis@imgtec.com changed reviewers:
+ bsalomon@google.com

[lmilko, 2016-04-14 13:19:08]

+bsalomon

Please take a look.

[bsalomon, 2016-04-19 14:43:40]

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

[bsalomon, 2016-04-19 14:43:41]

I'm not very familiar with this code, defer to reed@ or mtklein@

[lmilko, 2016-05-06 15:54:55]

Ping.

[reed1, 2016-05-06 18:25:36]

Seems fine.

1. Can you also augment some of the testing in tests/MemsetTest.cpp to try
various allocations, and check that they are always 8-byte aligned?

2. Can you add dox in the header to make this new contract clear?

[lmilko, 2016-05-10 11:49:45]

Description was changed from

==========
MIPS32r2: Fix Chromium runtime crash

Crash is caused by ldxc1 instruction, which traps when double values are
not aligned on 8-byte boundaries. Problem was tracked to SkChunkAlloc which
produces pointers aligned on 4-byte boundaries leading to misalignment.

This change makes sure that SkChunkAlloc will produce pointers that are
aligned to 8 bytes.

TEST=Build Chromium with Clang and run on MIPS32r2 platform
BUG=130022
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
MIPS32r2: Fix Chromium runtime crash

Crash is caused by ldxc1 instruction, which traps when double values are
not aligned on 8-byte boundaries. Problem was tracked to SkChunkAlloc which
produces pointers aligned on 4-byte boundaries leading to misalignment.

This change makes sure that SkChunkAlloc will produce pointers that are 
aligned to 8 bytes.

Appropriate tests are added to tests/MemsetTest.cpp

TEST=Build Chromium with Clang and run on MIPS32r2 platform
TEST=./out/Debug/dm --match Memset
BUG=130022
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[lmilko, 2016-05-10 11:51:01]

Comments addressed, please take a look.

[lmilko, 2016-05-18 09:44:54]

On 2016/05/10 11:51:01, lmilko wrote:
> Comments addressed, please take a look.

PING

[lmilko, 2016-05-25 12:54:08]

On 2016/05/18 09:44:54, lmilko wrote:
> On 2016/05/10 11:51:01, lmilko wrote:
> > Comments addressed, please take a look.
> 
> PING

@reed1 Can you please take a look.

[reed1, 2016-05-25 14:31:52]

Not sure the unittest for chunkalloc needs to pull in headers from pathops.
Shouldn't it just try allocating various sizes (in various orders) and assert
that the address returned is always 8-byte aligned?

[lmilko, 2016-05-26 09:40:03]

PTAL

[lmilko, 2016-06-01 07:53:51]

On 2016/05/26 09:40:03, lmilko wrote:
> PTAL

@reed Can you please take a look.

[reed1, 2016-06-01 13:45:27]

https://codereview.chromium.org/1849183004/diff/60001/src/core/SkChunkAlloc.cpp
File src/core/SkChunkAlloc.cpp (right):

https://codereview.chromium.org/1849183004/diff/60001/src/core/SkChunkAlloc.c...
src/core/SkChunkAlloc.cpp:176: return ptr;
I think this is the single point where we return the ptr to the user. Lets add
an assert here that the value is 8-byte aligned.

https://codereview.chromium.org/1849183004/diff/60001/tests/MemsetTest.cpp
File tests/MemsetTest.cpp (right):

https://codereview.chromium.org/1849183004/diff/60001/tests/MemsetTest.cpp#ne...
tests/MemsetTest.cpp:96: for (size_t i = 0; i < SK_ARRAY_COUNT(gAllocSizes);
i++)
Is there something significant in the sizes you've chosen for the gAllocSizes
array? If not, would just a loop and SkRandom serve the same purpose? I'm
thinking mostly of making the test clear to the next reader.

[lmilko, 2016-06-01 14:48:06]

Please take a look.

https://codereview.chromium.org/1849183004/diff/60001/src/core/SkChunkAlloc.cpp
File src/core/SkChunkAlloc.cpp (right):

https://codereview.chromium.org/1849183004/diff/60001/src/core/SkChunkAlloc.c...
src/core/SkChunkAlloc.cpp:176: return ptr;
On 2016/06/01 13:45:27, reed1 wrote:
> I think this is the single point where we return the ptr to the user. Lets add
> an assert here that the value is 8-byte aligned.

Done.

https://codereview.chromium.org/1849183004/diff/60001/tests/MemsetTest.cpp
File tests/MemsetTest.cpp (right):

https://codereview.chromium.org/1849183004/diff/60001/tests/MemsetTest.cpp#ne...
tests/MemsetTest.cpp:96: for (size_t i = 0; i < SK_ARRAY_COUNT(gAllocSizes);
i++)
On 2016/06/01 13:45:27, reed1 wrote:
> Is there something significant in the sizes you've chosen for the gAllocSizes
> array? If not, would just a loop and SkRandom serve the same purpose? I'm
> thinking mostly of making the test clear to the next reader.

Done.

[reed1, 2016-06-01 14:56:12]

https://codereview.chromium.org/1849183004/diff/80001/tests/MemsetTest.cpp
File tests/MemsetTest.cpp (right):

https://codereview.chromium.org/1849183004/diff/80001/tests/MemsetTest.cpp#ne...
tests/MemsetTest.cpp:93: for (int i = 0; i < 16; i++)
final nit: :)

1. we have time. Lets run that rand loop more times... e.g. 1000?
2. skia sytle always uses { } for blocks, even single liners.

[lmilko, 2016-06-01 16:43:52]

https://codereview.chromium.org/1849183004/diff/80001/tests/MemsetTest.cpp
File tests/MemsetTest.cpp (right):

https://codereview.chromium.org/1849183004/diff/80001/tests/MemsetTest.cpp#ne...
tests/MemsetTest.cpp:93: for (int i = 0; i < 16; i++)
On 2016/06/01 14:56:11, reed1 wrote:
> final nit: :)
> 
> 1. we have time. Lets run that rand loop more times... e.g. 1000?
> 2. skia sytle always uses { } for blocks, even single liners.

Done.

[lmilko, 2016-06-03 08:25:49]

On 2016/06/01 16:43:52, lmilko wrote:
> https://codereview.chromium.org/1849183004/diff/80001/tests/MemsetTest.cpp
> File tests/MemsetTest.cpp (right):
> 
>
https://codereview.chromium.org/1849183004/diff/80001/tests/MemsetTest.cpp#ne...
> tests/MemsetTest.cpp:93: for (int i = 0; i < 16; i++)
> On 2016/06/01 14:56:11, reed1 wrote:
> > final nit: :)
> > 
> > 1. we have time. Lets run that rand loop more times... e.g. 1000?
> > 2. skia sytle always uses { } for blocks, even single liners.
> 
> Done.

@reed1 Nit addressed, please take a look

[reed1, 2016-06-03 19:38:02]

lgtm

[lmilko, 2016-06-05 20:00:08]

The CQ bit was checked by milko.leporis@imgtec.com

[commit-bot: I haz the power, 2016-06-05 20:00:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1849183004/100001

[commit-bot: I haz the power, 2016-06-05 20:14:25]

Description was changed from

==========
MIPS32r2: Fix Chromium runtime crash

Crash is caused by ldxc1 instruction, which traps when double values are
not aligned on 8-byte boundaries. Problem was tracked to SkChunkAlloc which
produces pointers aligned on 4-byte boundaries leading to misalignment.

This change makes sure that SkChunkAlloc will produce pointers that are 
aligned to 8 bytes.

Appropriate tests are added to tests/MemsetTest.cpp

TEST=Build Chromium with Clang and run on MIPS32r2 platform
TEST=./out/Debug/dm --match Memset
BUG=130022
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
MIPS32r2: Fix Chromium runtime crash

Crash is caused by ldxc1 instruction, which traps when double values are
not aligned on 8-byte boundaries. Problem was tracked to SkChunkAlloc which
produces pointers aligned on 4-byte boundaries leading to misalignment.

This change makes sure that SkChunkAlloc will produce pointers that are 
aligned to 8 bytes.

Appropriate tests are added to tests/MemsetTest.cpp

TEST=Build Chromium with Clang and run on MIPS32r2 platform
TEST=./out/Debug/dm --match Memset
BUG=130022
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/401e77cfe126eea1a12bd81410871fdb80739283
==========

[commit-bot: I haz the power, 2016-06-05 20:14:26]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/401e77cfe126eea1a12bd81410871fdb80739283