Disable the noisiest /analyze warning in Chrome. ~3,700/12,000

Disable the noisiest /analyze warning in Chrome. ~3,700/12,000

Decades ago Intel decided that the bsr (Bit Scan Reverse) instruction
should have undefined results if its argument is zero. This probably
makes the instruction harder to implement and it definitely makes it
more difficult to use.

In SkCLZ_portable it requires a check for a zero argument, but despite
that check /analyze still warns that _BitScanReverse might fail
(because it doesn't know what can cause failures). Because this warning
occurs in a frequently included header file it ends up being very noisy,
accounting for ~30% of all warnings (before deduplication).

Suppressing this useless warning will make the raw results easier to
look through.

Committed: https://skia.googlesource.com/skia/+/aea85dc3d332c963f5b85460036951ae3386683a

[brucedawson, 2015-01-29 22:25:44]

brucedawson@chromium.org changed reviewers:
+ reed@google.com

[brucedawson, 2015-01-29 22:25:45]

It would be nice to suppress this warning.

The noisiness of this warning can be seen by searching for "skmath.h(82)" in:

http://build.chromium.org/p/chromium.fyi/builders/Chromium%20Windows%20Analyz...

It's actually a higher number/percentage in build 60 which built all of Chrome
(including tests).

[reed1, 2015-01-30 14:03:51]

reed@google.com changed reviewers:
+ bungeman@google.com, mtklein@google.com

[reed1, 2015-01-30 14:03:52]

seems fine to me. adding mike/ben

[bungeman-skia, 2015-01-30 15:49:22]

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

https://codereview.chromium.org/872673007/diff/1/include/core/SkMath.h#newcode85
include/core/SkMath.h:85: #pragma warning(disable: 6102) // Using 'index' from
failed function call
Can we push and pop this? I feel uncomfortable making this look like it's scoped
here, when it isn't. Since this is in a header, I think this covers the rest of
the translation unit as well.

[mtklein, 2015-01-30 16:07:55]

I guess there's no way to communicate up to /analyze that it's not zero? 
__assume() ?

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

https://codereview.chromium.org/872673007/diff/1/include/core/SkMath.h#newcode84
include/core/SkMath.h:84: #ifdef _PREFAST_
Can't hurt to add '// /analyze enabled'

[brucedawson, 2015-01-30 17:38:06]

Thanks for the comments. I've addressed them.

Regarding telling /analyze that the input is definitely non-zero, that won't
help because /analyze isn't smart enough to know under what conditions
_BitScanReverse might fail, it just knows that it might fail. Lame.

Ultimately it's Intel's fault. Whose genius idea was it to have an instruction
that has undefined behavior?

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

https://codereview.chromium.org/872673007/diff/1/include/core/SkMath.h#newcode84
include/core/SkMath.h:84: #ifdef _PREFAST_
On 2015/01/30 16:07:54, mtklein wrote:
> Can't hurt to add '// /analyze enabled'

Done.

https://codereview.chromium.org/872673007/diff/1/include/core/SkMath.h#newcode85
include/core/SkMath.h:85: #pragma warning(disable: 6102) // Using 'index' from
failed function call
Good catch. I meant to use #pragma warning(suppress which only affects the
following line, but I forgot. I've made that change, and confirmed that the
warning remains enabled for the rest of the translation unit.

[mtklein, 2015-01-30 18:09:37]

lgtm

[bungeman-skia, 2015-01-30 18:10:54]

https://codereview.chromium.org/872673007/diff/40001/include/core/SkMath.h
File include/core/SkMath.h (right):

https://codereview.chromium.org/872673007/diff/40001/include/core/SkMath.h#ne...
include/core/SkMath.h:84: #ifdef _PREFAST_  // /analyze enabled
I took a look, and it looks like warning 6102 is specifically only an analyzer
warning, so it seems a bit redundant to only disable this warning only when
analyze is enabled (since we're already inside an _MSC_VER ifdef). Can we remove
this ifdef and just always suppress this warning on this line?

[brucedawson, 2015-01-30 18:11:18]

The CQ bit was checked by brucedawson@chromium.org

[commit-bot: I haz the power, 2015-01-30 18:11:57]

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

[commit-bot: I haz the power, 2015-01-30 18:11:59]

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

[commit-bot: I haz the power, 2015-01-30 18:12:00]

Presubmit check for 872673007-40001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Since the CL is editing public API, you must have an LGTM from one of:
('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org',
'bsalomon@google.com', 'djsollen@chromium.org', 'djsollen@google.com')

[reed1, 2015-01-30 18:22:21]

Do you think we can eliminate the check for PREFAST?

[brucedawson, 2015-01-30 18:24:22]

That depends on what compilers you support.

VC++ 2010 and below do not support /analyze and therefore, I believe, are not
familiar with warning 6102, so they might give a warning on attempting to
suppress it. The same may apply with express editions of more recent compilers.
So, *maybe*, but probably not.

[bungeman-skia, 2015-01-30 19:36:39]

On 2015/01/30 18:24:22, brucedawson wrote:
> That depends on what compilers you support.
> 
> VC++ 2010 and below do not support /analyze and therefore, I believe, are not
> familiar with warning 6102, so they might give a warning on attempting to
> suppress it. The same may apply with express editions of more recent
compilers.
> So, *maybe*, but probably not.

As an aside, at this point we should stop checking for VS2005 and later
(VC1400), I can't imagine anything we do would work on an older version anyway.
I can't imagine we even compile on anything before VS2010 (VC1600), so we should
remove these now unneeded version checks.

I'm fairly certain that 6102 is in the 'normal' warning range and won't be
warned about anyway. (So is the 28000 series for that matter.) I fired up my old
XP with 2010 VM, and 2010 express has no issue with "#pragma warning(suppress :
6102)". If you wanted to be doubly sure you can write

#pragma warning(suppress : 4616)
#pragma warning(suppress : 6102)

and the first line should suppress any warning in the second (if any).  I've
tested that this suppresses the warning in VC2010 when trying to suppress
warning 100000 (which normally gives a warning). I would prefer not to reference
_PREFAST_ here if we don't need to.

[brucedawson, 2015-01-30 19:49:43]

Thanks for doing that test. I had no easy way to do that.

With those results I feel much safer removing the _PREFAST_ check.

PTAL.

[reed1, 2015-01-30 20:20:01]

lgtm

[bungeman-skia, 2015-01-30 20:46:00]

The CQ bit was checked by bungeman@google.com

[bungeman-skia, 2015-01-30 20:46:00]

lgtm

[commit-bot: I haz the power, 2015-01-30 20:46:48]

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

[commit-bot: I haz the power, 2015-01-30 20:57:53]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/aea85dc3d332c963f5b85460036951ae3386683a