Fixed macro redefinition warnings when compiling with clang-cl.

[mwinterb]

Both clang-cl and Clang/C2 #define _MSC_VER but also have support for __builtin_clz and __builtin_clzll, leading to duplicate macro definition warnings. This change suppresses emulation of clz using _BitScanReverse if the __clang__ macro is defined.

[mwinterb]

It seemed to "heavy" to mention it in the commit, but clang-cl refers to this http://www.llvm.org/builds/, and clang/c2 refers to this: http://blogs.msdn.com/b/vcblog/archive/2015/12/04/introducing-clang-with-microsoft-codegen-in-vs-2015-update-1.aspx.

It's possibly also reasonable to add undef's of FMT_BUILTIN_CLZLL and FMT_BUILTIN_CLZ before defining them in the FMT_GCC_VERSION / FMT_HAS_BUILTIN case, but both of the "Microsoft/Clang" compilers also generate warnings about an unknown pragma intrinsic for _BSR and _BSR64, so that can't be the only change.

[vitaut]

Great catch, thanks!

It's possibly also reasonable to add undef's of FMT_BUILTIN_CLZLL and FMT_BUILTIN_CLZ before defining them in the FMT_GCC_VERSION / FMT_HAS_BUILTIN case, but both of the "Microsoft/Clang" compilers also generate warnings about an unknown pragma intrinsic for _BSR and _BSR64, so that can't be the only change.

Maybe check

#if defined(_MSC_VER) && !defined(FMT_BUILTIN_CLZ)

instead of

#if defined(_MSC_VER) && !defined(__clang__)

in case some other compiler decides to do the same (however unlikely it is =)). What do you think?

Also it would be nice to have a comment why this is necessary, because it's not obvious that other compilers can define _MSC_VER.

[mwinterb]

Both sound good, I'll get to that soon.

There's another "issue" with the emulation, and that's that _BitScanReverse is annotated as only returning success if the return value is non-zero, so VC++'s static analyzer complains about "r" being uninitialized since there's no check for success. The only way that could happen is if 0 is passed in to the clz functions. Since cppformat is careful to never pass in zero I was going to just assert that 'x' isn't zero and suppress the warning, likely with just a pragma. Would you like that to be a separate PR? Do you have any stylistic concerns about it? So clz would switch from this:

inline uint32_t clz(uint32_t x) {
  unsigned long r = 0;
  _BitScanReverse(&r, x);
  return 31 - r;
}

to this:

inline uint32_t clz(uint32_t x) {
  unsigned long r = 0;
  _BitScanReverse(&r, x);

  assert(x != 0);
#pragma warning(suppress: 6102)
  return 31 - r;
}

[vitaut]

Same PR is OK, but from https://msdn.microsoft.com/en-us/library/fbxyd7zd.aspx I don't see how _BitScanReverse can return an error. Could you post the exact warning that you get?

[mwinterb]

This ended up being more words than you wanted, sorry.

The lines may be off from current code, but:
format.h(90): warning C6102: Using 'r' from failed function call at line '88'.
format.h(111): warning C6102: Using 'r' from failed function call at line '108'.

86: inline uint32_t clz(uint32_t x) {
87:  unsigned long r = 0;
88:  _BitScanReverse(&r, x);

90:  return 31 - r;
}

Line 111 is in clzll's definition.

The winnt.h header (but not in intrin.h, so this depends on which files are included before format.h), defines the function prototype as:

_Success_(return!=0)
BOOLEAN
_BitScanReverse (
    _Out_ DWORD *Index,
    _In_ DWORD Mask
    );

Which tells the static analyzer "*Index is valid only if the return value is non-zero". Since there's no check of the result from _BitScanReverse or _BitScanReverse64, the static analyzer assumes that the function failed and then warns that there is an undefined value usage.

From those docs for _BitScanReverse,
"Return Value
Nonzero if Index was set, or 0 if no set bits were found." which matches Intel's docs on BSR since that's just what _BitScanReverse compiles down to, "If the content source operand is 0, the content of the destination operand is undefined." and "The ZF flag is set to 1 if all the source operand is 0; otherwise, the ZF flag is cleared."

A similar workaround is seen here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/include/core/SkMath.h&l=81 (though I don't know why the return value isn't just checked instead of checking "mask" before calling BSR, but like in cppformat, there's an enforcement that the incoming value isn't zero before calling BSR). An alternate way of fixing the warning it would be:

inline uint32_t clz(uint32_t x)
{
    unsigned long r = 0;
    if (_BitScanReverse(&r, x))
    {
        return 31 - r;
    }

    return 32;
}

but the codegen retains the branch, even with known values, so the assert that undefined behavior cannot happen and suppress seems like the best for the resulting code even if it's slightly uglier in the source code.

[vitaut]

Thanks for a detailed explanation. My only concern regarding the #pragma warning workaround is that it applies to all the code below and can suppress other, potentially useful, warnings. The effect of this pragma can be localized by wrapping the code in

#pragma warning(push)
...
#pragma warning(pop) 

but it's a bit messy.

Since this warning is triggered by a SAL annotation, can it be fixed using an annotation too, e.g.

inline uint32_t clz(_In_range_(1, UINT32_MAX) uint32_t x)

or something like that?

[vitaut]

If not, then #pragma warning is fine.

[mwinterb]

I'm uncertain, in doing some research for that last comment, I came across this bug report, which tries to come up with several ways of annotating the function correctly, but since _BitScanReverse is not annotated with its own preconditions correctly, the analyzer doesn't know the the _In_range_ on "x" for clz would make _BitScanReverse always succeed. Some functions the analyzer has intrinsic knowledge of, but that doesn't seem to be one of them.
https://connect.microsoft.com/VisualStudio/feedback/details/1114418/bitscanreverse-as-defined-in-winnt-h-line-5065-causes-analyze-to-generate-false-positives-with-proposed-solution

I'll work on trying to annotate clz and clzll to generate a warning if there's ever a call that is not known to be non-zero but my attempts so far haven't been fruitful.

#pragma warning suppress is a shorthand for the push / disable / pop syntax, but only for the line that follows the suppress, so it shouldn't be an issue, unless you feel the push / pop is clearer. From here: https://msdn.microsoft.com/en-us/library/2c8f766e.aspx "Pushes the current state of the pragma on the stack, disables the specified warning for the next line, and then pops the warning stack so that the pragma state is reset."

[vitaut]

#pragma warning suppress is a shorthand for the push / disable / pop syntax

Didn't know that. Then let's go with the pragma and don't worry about annotations.

[vitaut]

Merged, thanks!

[mwinterb]

I missed one other problem. I don't know how it happened, I thought I committed the fix, but obviously didn't. The issue is this:
format.h(83,10): warning : unknown pragma ignored [-Wunknown-pragmas]
1> # pragma intrinsic(_BitScanReverse)

I'll get another PR soon, sorry about that. It is a bit more conditional code, though.

[vitaut]

No problems, thanks for working on this.