ubsan shift fixes

ubsan shift fixes

Use an inline function that does a normal shift. When built for the sanitizer, add casts so that the shift is unsigned.

Also make a few fixes to do unsigned shifts or avoid the shift altogether; and add an argument spec to some macros.

R=reed@google.com,mtklein@google.com
BUG=skia:4633
Committed: https://skia.googlesource.com/skia/+/3127c99986dc932343aae5ccc575237d99c3aaec

[_cary, 2015-12-08 18:22:50]

caryclark@chromium.org changed reviewers:
+ caryclark@chromium.org

[_cary, 2015-12-08 18:22:51]

shield your eyes

[reed1, 2015-12-08 18:29:04]

https://codereview.chromium.org/1503423003/diff/1/include/core/SkFixed.h
File include/core/SkFixed.h (right):

https://codereview.chromium.org/1503423003/diff/1/include/core/SkFixed.h#newc...
include/core/SkFixed.h:91: SkTPin<int32_t>(((int64_t)numer * (1 << 16)) / denom,
SK_MinS32, SK_MaxS32)
nit: I think we should comment why we're multiplying instead of just shifting

https://codereview.chromium.org/1503423003/diff/1/include/core/SkFixed.h#newc...
include/core/SkFixed.h:148: #define SkIntToFixed3232(x)       ((SkFixed3232)(x)
* (1L << 32))
same nit about commenting

[_cary, 2015-12-09 14:33:40]

PTAL. This approach should introduce no new overhead. In a few places, slightly
different but correct logic was used instead.

https://codereview.chromium.org/1503423003/diff/1/include/core/SkFixed.h
File include/core/SkFixed.h (right):

https://codereview.chromium.org/1503423003/diff/1/include/core/SkFixed.h#newc...
include/core/SkFixed.h:91: SkTPin<int32_t>(((int64_t)numer * (1 << 16)) / denom,
SK_MinS32, SK_MaxS32)
On 2015/12/08 18:29:03, reed1 wrote:
> nit: I think we should comment why we're multiplying instead of just shifting

Done (Reworked so there's no more multiply workarounds)

https://codereview.chromium.org/1503423003/diff/1/include/core/SkFixed.h#newc...
include/core/SkFixed.h:148: #define SkIntToFixed3232(x)       ((SkFixed3232)(x)
* (1L << 32))
On 2015/12/08 18:29:03, reed1 wrote:
> same nit about commenting

Done (Uses inline shift function now instead)

[_cary, 2015-12-09 14:35:22]

Description was changed from

==========
ubsan shift fixes

R=reed@google.com,mtklein@google.com
BUG=skia:4633
==========

to

==========
ubsan shift fixes

Use an inline function that does a normal shift. When built for the sanitizer,
add casts so that the shift is unsigned.

Also make a few fixes to do unsigned shifts or avoid the shift altogether; and
add an argument spec to some macros.

R=reed@google.com,mtklein@google.com
BUG=skia:4633
==========

[reed1, 2015-12-09 15:54:27]

nice clean fix -- given the absurd situation we find ourselves in w/ 2s
compliment not being std.

lgtm

https://codereview.chromium.org/1503423003/diff/80001/include/core/SkTypes.h
File include/core/SkTypes.h (right):

https://codereview.chromium.org/1503423003/diff/80001/include/core/SkTypes.h#...
include/core/SkTypes.h:308: return (int32_t) ((uint32_t) value << shift);
do we think we can't always use the version here (line 308)? Wonder if we need
the if/else

https://codereview.chromium.org/1503423003/diff/80001/src/effects/gradients/S...
File src/effects/gradients/SkClampRange.h (right):

https://codereview.chromium.org/1503423003/diff/80001/src/effects/gradients/S...
src/effects/gradients/SkClampRange.h:15: #define SkScalarToGradFixed(x) 
SkScalarToFixed3232(x)
nice catch

[mtklein, 2015-12-09 16:48:11]

https://codereview.chromium.org/1503423003/diff/80001/gyp/libjpeg-turbo.gyp
File gyp/libjpeg-turbo.gyp (right):

https://codereview.chromium.org/1503423003/diff/80001/gyp/libjpeg-turbo.gyp#n...
gyp/libjpeg-turbo.gyp:45: 'ldflags!': [ '-fsanitize=<(skia_sanitizer)' ],
Should be we can revert this file.  libjpeg-turbo's covered by
tools/xsan.blacklist.

https://codereview.chromium.org/1503423003/diff/80001/include/core/SkTypes.h
File include/core/SkTypes.h (right):

https://codereview.chromium.org/1503423003/diff/80001/include/core/SkTypes.h#...
include/core/SkTypes.h:308: return (int32_t) ((uint32_t) value << shift);
On 2015/12/09 at 15:54:26, reed1 wrote:
> do we think we can't always use the version here (line 308)? Wonder if we need
the if/else

agree... both'll generate the same code.

https://codereview.chromium.org/1503423003/diff/80001/src/effects/gradients/S...
File src/effects/gradients/SkClampRange.h (right):

https://codereview.chromium.org/1503423003/diff/80001/src/effects/gradients/S...
src/effects/gradients/SkClampRange.h:15: #define SkScalarToGradFixed(x) 
SkScalarToFixed3232(x)
On 2015/12/09 at 15:54:27, reed1 wrote:
> nice catch

has this actually changed anything?  just hygiene?

https://codereview.chromium.org/1503423003/diff/80001/src/utils/SkDashPath.cpp
File src/utils/SkDashPath.cpp (right):

https://codereview.chromium.org/1503423003/diff/80001/src/utils/SkDashPath.cp...
src/utils/SkDashPath.cpp:13: return !(x & 1);
i'll guarantee even (x % 2 == 0) generates good code

[caryclark, 2015-12-09 19:13:12]

https://codereview.chromium.org/1503423003/diff/80001/gyp/libjpeg-turbo.gyp
File gyp/libjpeg-turbo.gyp (right):

https://codereview.chromium.org/1503423003/diff/80001/gyp/libjpeg-turbo.gyp#n...
gyp/libjpeg-turbo.gyp:45: 'ldflags!': [ '-fsanitize=<(skia_sanitizer)' ],
On 2015/12/09 16:48:11, mtklein wrote:
> Should be we can revert this file.  libjpeg-turbo's covered by
> tools/xsan.blacklist.

Done.

https://codereview.chromium.org/1503423003/diff/80001/include/core/SkTypes.h
File include/core/SkTypes.h (right):

https://codereview.chromium.org/1503423003/diff/80001/include/core/SkTypes.h#...
include/core/SkTypes.h:308: return (int32_t) ((uint32_t) value << shift);
On 2015/12/09 15:54:26, reed1 wrote:
> do we think we can't always use the version here (line 308)? Wonder if we need
> the if/else

Done.

https://codereview.chromium.org/1503423003/diff/80001/src/effects/gradients/S...
File src/effects/gradients/SkClampRange.h (right):

https://codereview.chromium.org/1503423003/diff/80001/src/effects/gradients/S...
src/effects/gradients/SkClampRange.h:15: #define SkScalarToGradFixed(x) 
SkScalarToFixed3232(x)
On 2015/12/09 16:48:11, mtklein wrote:
> On 2015/12/09 at 15:54:27, reed1 wrote:
> > nice catch
> 
> has this actually changed anything?  just hygiene?

No change -- made it while tracking down a different bug -- but thought it was
worth having.

https://codereview.chromium.org/1503423003/diff/80001/src/utils/SkDashPath.cpp
File src/utils/SkDashPath.cpp (right):

https://codereview.chromium.org/1503423003/diff/80001/src/utils/SkDashPath.cp...
src/utils/SkDashPath.cpp:13: return !(x & 1);
On 2015/12/09 16:48:11, mtklein wrote:
> i'll guarantee even (x % 2 == 0) generates good code

Acknowledged.

[mtklein, 2015-12-09 19:15:26]

lgtm

[caryclark, 2015-12-09 19:15:57]

The CQ bit was checked by caryclark@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-09 19:16:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1503423003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1503423003/100001

[commit-bot: I haz the power, 2015-12-09 19:19:30]

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

[commit-bot: I haz the power, 2015-12-09 19:19:31]

Dry run: Try jobs failed on following builders:
  Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)
  Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[caryclark, 2015-12-09 19:45:28]

The CQ bit was checked by caryclark@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-09 19:45:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1503423003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1503423003/120001

[commit-bot: I haz the power, 2015-12-09 19:58:43]

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

[commit-bot: I haz the power, 2015-12-09 19:58:44]

Dry run: This issue passed the CQ dry run.

[mtklein, 2015-12-09 20:01:32]

The CQ bit was checked by mtklein@google.com

[mtklein, 2015-12-09 20:01:32]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com,
mtklein@google.com
Link to the patchset: https://codereview.chromium.org/1503423003/#ps120001
(title: "add cast to work around win compiler")

[commit-bot: I haz the power, 2015-12-09 20:01:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1503423003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1503423003/120001

[commit-bot: I haz the power, 2015-12-09 20:02:32]

Description was changed from

==========
ubsan shift fixes

Use an inline function that does a normal shift. When built for the sanitizer,
add casts so that the shift is unsigned.

Also make a few fixes to do unsigned shifts or avoid the shift altogether; and
add an argument spec to some macros.

R=reed@google.com,mtklein@google.com
BUG=skia:4633
==========

to

==========
ubsan shift fixes

Use an inline function that does a normal shift. When built for the sanitizer,
add casts so that the shift is unsigned.

Also make a few fixes to do unsigned shifts or avoid the shift altogether; and
add an argument spec to some macros.

R=reed@google.com,mtklein@google.com
BUG=skia:4633

Committed:
https://skia.googlesource.com/skia/+/3127c99986dc932343aae5ccc575237d99c3aaec
==========

[commit-bot: I haz the power, 2015-12-09 20:02:33]

Committed patchset #7 (id:120001) as
https://skia.googlesource.com/skia/+/3127c99986dc932343aae5ccc575237d99c3aaec