Fix CodeQL warnings, part 2

[StephanTLavavej]

Part 1: #3489

CodeQL is emitting more warnings in the STL's separately compiled sources, resulting in the automatic creation of high-priority bugs in the internal database.

This PR introduces no behavioral changes, nor does it affect the STL's dllexports.

xtime_get

• Fix xtime_get CodeQL warnings.
  • Bugs: VSO-1773907, VSO-1773908, VSO-1773909, VSO-1773910
  • This is the [cpp/conditionallyuninitializedvariable] warning:

    The status of this call to xtime_get is not checked, potentially leaving now uninitialized.

This is the most interesting one. We never documented or supported the non-Standard xtime API (and we really should get rid of these non-Standard identifiers someday). All of our calls to xtime_get are in src, and all of them are exactly xtime_get(&now, TIME_UTC). In this case, it always succeeds and calls the static helper sys_get_time. So, we can clean up this code and eradicate all occurrences of this warning.

• In inc/xtimec.h, we're going to drop the declaration of xtime_get. This is paired with marking the definition in src/xtime.cpp as preserved for bincompat.
  • As recently seen in Cleanups: Simplify thread and condition_variable, identify unused dllexports #3532, this does not affect the dllexport surface, because the declaration and the definition match (specifically _CRTIMP2_PURE and __cdecl).
  • This prevents both users and other STL TUs from seeing this non-Standard API that's likely to make CodeQL unhappy.
• Then, inc/xtimec.h declares _Xtime_get2, but only when building the STL. (This is because we don't have any convenient header in src that's dragged in by the relevant TUs.)
  • As the comment explains, this declaration lacks _CRTIMP2_PURE and __cdecl because it is not dllexported - it allows STL TUs to call a function defined in another STL TU, but it isn't for users to interact with. (The _Ugly name will have external linkage when static linking, but is unobservable to users.)
• All usage is mechanically converted to the simpler function.
• Finally, src/xtime.cpp changes the static helper sys_get_time (unobservable by all other TUs) into the _Xtime_get2 helper (for STL TUs), with the same comment.

<xbit_ops.h>

• Silence <xbit_ops.h> CodeQL warnings.
  • Bug: VSO-1772889
  • This is the [cpp/conditionallyuninitializedvariable] warning:

    The status of this call to externally defined (SAL) _BitScanReverse is not checked, potentially leaving _Result uninitialized.

  • This code is safe because _Floor_of_log_2() begins with:

    _Value |= size_t{1}; // avoid undefined answer from _BitScanReverse for 0

vector_algorithms.cpp

• Cleanup: Perform compound assignments before if-statements.
  • In modern code, we conventionally avoid mixing mutation and branching. This separation makes if (_Bingo != 0) easier to see in the following change:
• Silence vector_algorithms.cpp CodeQL warnings.
  • Bugs: VSO-1772915, VSO-1772918, VSO-1772919
  • This is the [cpp/conditionallyuninitializedvariable] warning:

    The status of this call to externally defined (SAL) _BitScanForward is not checked, potentially leaving _Offset uninitialized.

  • This code is safe because each callsite has tested if (_Bingo != 0).
• Silence more vector_algorithms.cpp CodeQL warnings.
  • Bugs: VSO-1772995, VSO-1772900, VSO-1772896
  • These [cpp/conditionallyuninitializedvariable] warnings are of the form:

    The status of this call to externally defined (SAL) _BitScanReverse is not checked, potentially leaving _H_pos uninitialized.

  • We're looking for a known element in these vector registers, so we have a guarantee that _Mask is non-zero and therefore _H_pos is always initialized.

[frederick-vs-ja]

Great!
It seems we can kill the name xtime without breaking ABI, since all non-inline functions involving xtime in STL are extern "C", which means that changing the name of a parameter type doesn't affect the mangled name. I'll try this in a later PR.

[AlexGuteniev]

Have you filled a bug against CodeQL about incorrect handling of _BitScanForward / _BitScanReverse, so that stupid "lgtm" comments could be removed in future?

[StephanTLavavej]

We've looked into reporting bugs to CodeQL but AFAIK we don't know how yet 😿

[barcharcraz]

stray newline (possibly above as well)

[StephanTLavavej]

These newlines were intentional. Because the LGTM comment (apparently) has to appear on the same line, I had to move the existing comment above. Then I introduced newlines above and below, to make it clear that the above comment applies only to the bitscan intrinsic, and not to following code.

[barcharcraz]

ditto stray newline

[StephanTLavavej]

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.