Skip to content

Turn on /analyze and fix resulting warnings - #1009

Merged
Stephan T. Lavavej (StephanTLavavej) merged 21 commits into
microsoft:masterfrom
BillyONeal:analyze
Aug 26, 2020
Merged

Turn on /analyze and fix resulting warnings#1009
Stephan T. Lavavej (StephanTLavavej) merged 21 commits into
microsoft:masterfrom
BillyONeal:analyze

Conversation

@BillyONeal

@BillyONeal Billy O'Neal (BillyONeal) commented Jul 7, 2020 •
edited by StephanTLavavej
Loading

Copy link
Copy Markdown
Member

This is to resolve Microsoft-internal:

(which all boil down to 'getting yelled at for not turning on /analyze')

Functional changes:

  • Added annotations for consistency to definitions
  • Added what I hope are 'obvious' _Analysis_assume_s
  • When a function sometimes filled in a value and sometimes did not, and the parameter was annotated _Out_, I removed the annotation as the annotations to tell PREfast that the parameter is conditionally _Out_ exceed any value they might give us.
  • Filed multprec.cpp: _MP_Rem has apparently nonsense memory read #1008 for a 'probably a bug but I'm not sure' case in _MP_Rem
  • Blindly suppressed additional warnings in ConcRT and Boost as there is nothing we can do about them here
  • Changed __crtInitializeCriticalSectionEx to return the value of InitializeCriticalSectionAndSpinCount rather than always return TRUE; in Vista or later this is no functional change, and on XP it will pass through the result correctly. Most callers are still ignoring the return value (as they probably should, now) but this avoids a warning here.
  • Changed _Wcsxfrm to use RAII to record when bbuffer is allocated rather than extra null checks.
  • Changed Xp_setw to hoist constexpr comparisons into an if constexpr block.
  • Added _PThrow null check before calling RtlPcToFileHeader in excptptr.cpp.

@BillyONeal
Billy O'Neal (BillyONeal) requested a review from a team as a code owner July 7, 2020 09:30
@CaseyCarter Casey Carter (CaseyCarter) added the bug Something isn't working label Jul 7, 2020
StephanTLavavej
Stephan T. Lavavej (StephanTLavavej) previously requested changes Jul 9, 2020
View reviewed changes

@StephanTLavavej Stephan T. Lavavej (StephanTLavavej) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The arm/arm64 builds are failing. From the error log:

C:\agent\_work\1\s\stl\src\winapisupp.cpp(493,1): error C2220: the following warning is treated as an error
C:\agent\_work\1\s\stl\src\winapisupp.cpp(490) : warning C28251: Inconsistent annotation for '__crtQueueUserWorkItem': this instance has no annotations. See c:\agent\_work\1\s\stl\src\awint.hpp(232).

@StephanTLavavej

This comment has been minimized.

Sign in to view
@BillyONeal

This comment has been minimized.

Sign in to view
@BillyONeal

Billy O'Neal (BillyONeal) commented Jul 15, 2020

Copy link
Copy Markdown
Member Author

(In fact I'd rather this land after the atomic wait stuff since that adds new APIs that are likely to make PREfast unhappy)

@StephanTLavavej
Define __crtQueueUserWorkItem with SAL.
0ab6312 
And follow the style in threadpoollegacyapiset.h.
@StephanTLavavej
Update SAL.
f6ef56e 
The functional changes are:

* `__crtFlsAlloc`: Change `lpCallback`
  from `__in` to `_In_opt_`
  as declared in `fibersapi.h`
* `__crtCreateEventExW`: Change `dwFlags`
  from `__reserved` to `_In_`
  as declared in `synchapi.h`
* `__crtGetFileInformationByHandleEx`: Change `lpFileInformation`
  from `_Out_` to `_Out_writes_bytes_(dwBufferSize)`
  as declared in `WinBase.h`
* `__crtSetFileInformationByHandle`: Change `lpFileInformation`
  from `_In_` to `_In_reads_bytes_(dwBufferSize)`
  as declared in `fileapi.h`
* `__crtGetLocaleInfoEx`: Change `lpLCData`
  from `_Out_opt_` to `_Out_writes_opt_(cchData)`
  as declared in `WinNls.h`
@StephanTLavavej

Stephan T. Lavavej (StephanTLavavej) commented Jul 15, 2020

Copy link
Copy Markdown
Member

Building with /analyze is noticeably slower. Are there other possibilities, like making this off-by-default for developers, but with coverage in the CI, assuming that /analyze build breaks will be relatively rare?

@BillyONeal

Billy O'Neal (BillyONeal) commented Jul 15, 2020

Copy link
Copy Markdown
Member Author

Building with /analyze is noticeably slower. Are there other possibilities, like making this off-by-default for developers, but with coverage in the CI, assuming that /analyze build breaks will be relatively rare?

Sure, we can do that; I didn't bother before because I figure our test pass takes ~an hour and our build takes ~30 seconds, and turning this on makes it take ~45 seconds; but the test pass still dominates. Making it a CMake configurable option is probably a good idea.

@StephanTLavavej

Stephan T. Lavavej (StephanTLavavej) commented Jul 15, 2020

Copy link
Copy Markdown
Member

Pushed a fix for the build break, which was simple - define __crtQueueUserWorkItem with SAL, like the rest of the definitions. But then I noticed that awint.hpp had different SAL annotations from the WinSDK, some stylistic but some significant. I updated the SAL, changing deprecated __meow to modern _Meow_ (note: not a mechanical in-place conversion; each occurrence was taken from the WinSDK) and fixing the differences. (I'm not sure if they were taken from an older WinSDK, or what. I used the WinSDK that's currently checked into the MSVC-internal repo.)

I ran full clean builds of x86/x64/arm/arm64 and they succeeded.

@BillyONeal

Billy O'Neal (BillyONeal) commented Jul 15, 2020

Copy link
Copy Markdown
Member Author

Thanks for running that down Stephan :D

@StephanTLavavej
Stephan T. Lavavej (StephanTLavavej) dismissed their stale review July 16, 2020 01:10

Fixed build break, need to actually review changes now

@BillyONeal
Merge remote-tracking branch 'origin/master' into analyze
91a7d7c 
# Conflicts:
#	stl/inc/xfilesystem_abi.h
#	stl/src/filesystem.cpp
@StephanTLavavej

This comment has been minimized.

Sign in to view
@StephanTLavavej

Stephan T. Lavavej (StephanTLavavej) commented Aug 3, 2020

Copy link
Copy Markdown
Member

I resolved the merge conflict; the code in parallel_algorithms.cpp was deleted, and I verified that this PR's addition of _Analysis_assume_(_Kernel32); didn't need to be moved anywhere else.

@BillyONeal

Billy O'Neal (BillyONeal) commented Aug 4, 2020

Copy link
Copy Markdown
Member Author

Thanks STL

StephanTLavavej
Stephan T. Lavavej (StephanTLavavej) requested changes Aug 20, 2020
View reviewed changes

@StephanTLavavej Stephan T. Lavavej (StephanTLavavej) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only change requested is to make the /analyze build off by default, and enabled for PR/CI builds.

Comment thread stl/src/filesystem.cpp
Comment thread stl/src/filesystem.cpp
@cbezault Curtis J Bezault (cbezault) added the build Related to the build system label Aug 24, 2020
@BillyONeal

Billy O'Neal (BillyONeal) commented Aug 24, 2020

Copy link
Copy Markdown
Member Author

Thanks Curtis!

@cbezault

Curtis J Bezault (cbezault) commented Aug 24, 2020

Copy link
Copy Markdown
Contributor

I'm not sure how to verify that the changes I made make /analyze run in the CI since the output isn't any different...

@CaseyCarter

Casey Carter (CaseyCarter) commented Aug 24, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

I'm not sure how to verify that the changes I made make /analyze run in the CI since the output isn't any different...

Temporarily Inject a code pattern that /analyze warns about and see if the CI breaks? (Possibly as a test PR from a different branch so as not to break this one?)

@StephanTLavavej

Stephan T. Lavavej (StephanTLavavej) commented Aug 26, 2020

Copy link
Copy Markdown
Member

The build break was that /clr:pure is /std:c++14, so using if constexpr in xxxprec.hpp triggered the Future Technology warning. The simplest way to silence this was to use the STL's header warning suppression machinery, which determines whether that warning needs to be suppressed (in addition to silencing other noise that we never care about). I didn't use the whole public header guard, just the MSVC warning fragment.

@StephanTLavavej
Stephan T. Lavavej (StephanTLavavej) merged commit 5bf2826 into microsoft:master Aug 26, 2020
@StephanTLavavej

Stephan T. Lavavej (StephanTLavavej) commented Aug 26, 2020

Copy link
Copy Markdown
Member

Analyzing change... 🤖 Beep! Thanks for this improvement, human! 🚀

Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@StephanTLavavej Stephan T. Lavavej (StephanTLavavej) StephanTLavavej approved these changes
+1 more reviewer
@mnatsuhara Miya Natsuhara (mnatsuhara) mnatsuhara approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@StephanTLavavej Stephan T. Lavavej (StephanTLavavej)

Labels

bug Something isn't working build Related to the build system

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@BillyONeal @StephanTLavavej @cbezault @CaseyCarter @mnatsuhara