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

Standard Library Modules: Fix warning C4365 (signed/unsigned mismatch) with /ZI #4487

Merged

Conversation

StephanTLavavej
Copy link
Member

This fixes DevCom-10610863 "Importing module std warns on signed/unsigned mismatch" (internal VSO-1992367 / AB#1992367 ).

Problem

The problem was that the /ZI compiler option (debug info for Edit and Continue) messes with __LINE__, making it a variable of type long instead of an integer literal. This variable can emit sign conversion warnings when passed to functions taking unsigned types (whereas integer literals don't, regardless of their type, because the compiler can directly see that the value is unaffected).

Fix

The fix is to static_cast<unsigned int>, matching what _invalid_parameter() takes.

I'm also slightly updating the previous comment, since // sentence fragment is fine by itself, but multiple sentences need punctuation for clarity.

Extra fix/cleanup

There was one more potential occurrence of signed/unsigned mismatch warnings involving __LINE__, when we call _invoke_watson() for _HAS_EXCEPTIONS=0. I couldn't actually repro a warning here, but we can eliminate any risk - it turns out that _invoke_watson() always ignores its parameters. See C:\Program Files (x86)\Windows Kits\10\Source\10.0.22621.0\ucrt\misc\invalid_parameter.cpp.

Unchanged __LINE__s

The rest of our __LINE__ usage is with _malloc_dbg and _calloc_dbg:

return _malloc_dbg(_Size > 0 ? _Size : 1, _CRT_BLOCK, __FILE__, __LINE__);

They take int linenumber, so a long will be fine.

No test coverage

I've been testing with /w14365 since the very beginning:

* PM_CL="/w14365 /D_ENFORCE_FACET_SPECIALIZATIONS=1 /D_STL_CALL_ABORT_INSTEAD_OF_INVALID_PARAMETER /Zc:preprocessor"

However, we missed this for two reasons: (1) no /ZI coverage, and (2) we use _STL_CALL_ABORT_INSTEAD_OF_INVALID_PARAMETER in our tests, which inherently bypasses the affected code. So, adding /ZI wouldn't actually accomplish anything useful.

Since we're highly centralized (all of _STL_ASSERT, _STL_VERIFY, _STL_REPORT_ERROR, and _INVALID_MEMORY_ORDER ultimately lead to _STL_CRT_SECURE_INVALID_PARAMETER), I believe that manual testing of this fix is sufficient as we're unlikely to regress.

C:\Program Files (x86)\Windows Kits\10\Source\10.0.22621.0\ucrt\misc\invalid_parameter.cpp

This was the only other potential occurrence of signed/unsigned mismatch warnings involving __LINE__.
@StephanTLavavej StephanTLavavej added bug Something isn't working modules C++23 modules, C++20 header units labels Mar 17, 2024
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner March 17, 2024 03:04
@cpplearner
Copy link
Contributor

cpplearner commented Mar 17, 2024

it turns out that _invoke_watson() always ignores its parameters.

Could a debugger see the parameters by inspecting the stack? Because apparently _invoke_watson() is intended to activate the debugger (or Dr. Watson) when one is present.

(But maybe it doesn't matter what _RAISE does, since it's used only if !_HAS_EXCEPTIONS, which is officially unsupported.)

@AlexGuteniev
Copy link
Contributor

AlexGuteniev commented Mar 17, 2024

The debugger should be able to inspect the call stack on its own, but it may be more convenient in variables for some cases

@StephanTLavavej
Copy link
Member Author

Yeah, if you can see the callstack, you already have all of the info that the arguments would have provided.

@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 StephanTLavavej self-assigned this Mar 19, 2024
@StephanTLavavej StephanTLavavej merged commit 2175098 into microsoft:main Mar 19, 2024
35 checks passed
@StephanTLavavej StephanTLavavej deleted the standard-library-meowdules branch March 19, 2024 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working modules C++23 modules, C++20 header units
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants