Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix static analysis warnings #3734

Merged
merged 9 commits into from
May 31, 2023

Conversation

StephanTLavavej
Copy link
Member

@StephanTLavavej StephanTLavavej commented May 26, 2023

The MSVC-internal repo is making another push to enable and clean up static analysis warnings in the build. The STL was previously unaffected as we've built our tests with plain /analyze for 3000 years, but now additional rulesets are being enabled and the STL is emitting warnings.

  • Add empty braces to fix warning C26495 "Variable 'MEOW' is uninitialized. Always initialize a member variable (type.6)."
    • This is safe because Zero Is A Strict Subset Of Garbage.
  • Permanently silence warning C26495 (uninitialized member variable) for _Big_integer_flt::_Mydata.
    • As the big comment explains, leaving this uninitialized is important for performance.
  • Fix warning C28252 "Inconsistent annotation for 'RtlCaptureStackBackTrace': return/function has 'SAL_success(return!=0)' on the prior instance. See [...]\winnt.h(20721)."
    • The return annotation was indeed missing.
  • Fix warning C26439 "This kind of function should not throw. Declare it 'noexcept' (f.6)."
    • Add noexcept(false) because these move ctors call basic_ios::init() => ios_base::_Init() => new locale.
    • list dynamically allocates a sentinel node.
  • Fix warning C26450 "Arithmetic overflow: '-' operation causes overflow at compile time. Use a wider type to store the operands (io.1)." and warning C26454 "Arithmetic overflow: '-' operation produces a negative unsigned result at compile time (io.5)."
    • We can avoid these warnings by directly forming the values, which is also easier to read.
      C:\Temp>type meow.cpp
      
      #include <climits>
      #include <iostream>
      using namespace std;
      
      static_assert((1ul << 31) == 0 - static_cast<unsigned long>(LONG_MIN));
      static_assert((1ull << 63) == 0 - static_cast<unsigned long long>(LLONG_MIN));
      
      int main() {
          cout << 0 - static_cast<unsigned long>(LONG_MIN) << endl;
          cout << (1ul << 31) << endl;
      
          cout << 0 - static_cast<unsigned long long>(LLONG_MIN) << endl;
          cout << (1ull << 63) << endl;
      }
      C:\Temp>cl /EHsc /nologo /W4 meow.cpp && meow
      meow.cpp
      2147483648
      2147483648
      9223372036854775808
      9223372036854775808
      
  • Silence warning C26110 "Caller failing to hold lock 'MEOW' before calling function 'ReleaseSRWLockExclusive'."
    • We're implementing lock machinery, so we have to assume that the lock is held here.
  • In <charconv>, work around VSO-1826196 "False positive C26454 in charconv". This has already been fixed in the latest compiler sources, we just need to wait for the MSVC-internal toolset to be updated. Thanks to @dmitrykobets-msft for investigating this.

…zed. Always initialize a member variable (type.6)."
…race': return/function has 'SAL_success(return!=0)' on the prior instance. See [...]\winnt.h(20721)."
…t 'noexcept' (f.6)."

Add `noexcept(false)` because these move ctors call `basic_ios::init()` => `ios_base::_Init()` => `new locale`.

`list` dynamically allocates a sentinel node.
…w at compile time. Use a wider type to store the operands (io.1)." and warning C26454 "Arithmetic overflow: '-' operation produces a negative unsigned result at compile time (io.5)."

We can avoid these warnings by directly forming the values, which is also easier to read.

    C:\Temp>type meow.cpp
    #include <climits>
    #include <iostream>
    using namespace std;

    static_assert((1ul << 31) == 0 - static_cast<unsigned long>(LONG_MIN));
    static_assert((1ull << 63) == 0 - static_cast<unsigned long long>(LLONG_MIN));

    int main() {
        cout << 0 - static_cast<unsigned long>(LONG_MIN) << endl;
        cout << (1ul << 31) << endl;

        cout << 0 - static_cast<unsigned long long>(LLONG_MIN) << endl;
        cout << (1ull << 63) << endl;
    }

    C:\Temp>cl /EHsc /nologo /W4 meow.cpp && meow
    meow.cpp
    2147483648
    2147483648
    9223372036854775808
    9223372036854775808
…ling function 'ReleaseSRWLockExclusive'."

We're implementing lock machinery, so we have to assume that the lock is held here.
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label May 26, 2023
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner May 26, 2023 05:56
@StephanTLavavej StephanTLavavej self-assigned this May 26, 2023
@StephanTLavavej
Copy link
Member Author

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

@StephanTLavavej
Copy link
Member Author

Added one more change before mirroring.

stl/inc/future Outdated Show resolved Hide resolved
stl/inc/ios Show resolved Hide resolved
stl/inc/list Show resolved Hide resolved
stl/inc/istream Show resolved Hide resolved
@StephanTLavavej StephanTLavavej merged commit c8594d8 into microsoft:main May 31, 2023
@StephanTLavavej StephanTLavavej deleted the static-analysis-2 branch May 31, 2023 00:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants