Enable /permissive- and remaining /Zc flags #11816

lhecker · 2021-11-24T01:47:20Z

This commit enables /permissive- for all projects, as well as all other /Zc
flags not enabled by default by /permissive-. Some projects continue to be
built under /Zc:twoPhase- as JsonUtils.h fails to compile otherwise.

PR Checklist

Closes [MSVC][Build][Permissive-][std:c++latest] Terminal failed to build on MSVC #10703
I work here
Tests added/passed

src/host/init.cpp

lhecker · 2021-11-24T01:49:41Z

FYI JsonUtils requires extremely far reaching changes to compile with standard conform two phase lookup rules. It almost feels like half the code needs to be rewritten to be compliant with C++. 😄
I've done about a third of the work so far and will send a followup PR for that.
Additionally I'll send a PR to enable /await:strict, however that requires an upgrade of our cppwinrt dependency.

lhecker · 2021-11-24T02:34:34Z

src/host/alias.cpp

@@ -1216,7 +1216,7 @@ std::wstring Alias::s_MatchAndCopyAlias(const std::wstring& sourceText,
 // - LineCount - aliases can contain multiple commands. $T is the command separator
 // Return Value:
 // - None. It will just maintain the source as the target if we can't match an alias.
-void Alias::s_MatchAndCopyAliasLegacy(_In_reads_bytes_(cbSource) PWCHAR pwchSource,
+void Alias::s_MatchAndCopyAliasLegacy(_In_reads_bytes_(cbSource) PCWCH pwchSource,


/permissive- forbids you from passing string literals (const wchar_t*) in non-const arguments (wchar_t*).
This is a theme you'll see throughout this PR.

this explains.... years... of bugs

src/host/init.cpp

lhecker · 2021-11-24T02:36:07Z

src/host/ut_host/ReadWaitTests.cpp

@@ -118,7 +118,7 @@ class InputRecordConversionTests
 dbcsChars,
 INPUT_RECORD_COUNT * 2,
 nullptr,
- false);
+ FALSE);


Can't pass true bools as fake BOOLs anymore.

lhecker · 2021-11-24T02:38:16Z

src/host/ut_host/TextBufferIteratorTests.cpp

+template<typename T>
+T GetIterator()
+{
+}


Two phase lookup goes brrrrrrr... with our code ordering.

Just like regular functions, templates also need to be defined before they're used in C++. It's just MSVC that was treating templates as macros for the longest time and didn't do that. A lot of our code depends on this behavior.

ouch. okay this makes sense.

We could also leave them without bodies, correct?

template<typename T> T GetIterator();

and then provide specializations? Therefore, if we call it with a T that has not been specialized we get a linker error?

Or does this provide a compiler error?

Actually, how does this not provide an error in a compliant parser? There's no return, it should fail before we get to template generation.

Oh, this is code you moved. oops.

lhecker · 2021-11-24T02:39:10Z

src/inc/til/static_map.h

@@ -97,7 +97,7 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned"
 public:
 template<typename... Args>
 constexpr explicit presorted_static_map(const Args&... args) noexcept :
- static_map{ args... } {};
+ static_map<K, V, Compare, N, details::presorted_input_t>{ args... } {};


These things require template parameters now.

lhecker · 2021-11-24T02:40:20Z

src/inc/til/string.h

@@ -169,7 +169,7 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned"
 // and 5x or more for long strings (128 characters or more).
 // See: https:/microsoft/STL/issues/2289
 template<typename T, typename Traits>
- bool equals(const std::basic_string_view<T, Traits>& str1, const std::basic_string_view<T, Traits>& str2) noexcept
+ bool equals(const std::basic_string_view<T, Traits>& lhs, const std::basic_string_view<T, Traits>& rhs) noexcept


As mentioned before, with /permissive- and true two phase lookup, templates are actually parsed! This means such programming errors can't pass accidentally anymore, because the compiler will actually complain that the code is invalid even if the template isn't used anywhere.

...wow this template just was never compiled lol

lhecker · 2021-11-24T02:45:42Z

src/types/inc/viewport.hpp

+ constexpr bool operator==(const Viewport& other) const noexcept
+ {
+ return _sr == other._sr;
+ }
+
+ constexpr bool operator!=(const Viewport& other) const noexcept
+ {
+ return _sr != other._sr;
+ }


Don't ask me why, but without this change the code doesn't compile. 😐

how strange.

I've always wondered why we did out-of-body inline operators and didn't just put them in the classes

lhecker · 2021-11-24T21:59:04Z

src/interactivity/win32/ut_interactivity_win32/UiaTextRangeTests.cpp

@@ -1368,17 +1368,17 @@ class UiaTextRangeTests
 Log::Comment(NoThrowString().Format(L"Forward by %s", toString(textUnit)));

 // Create an UTR at EndExclusive
- const auto utrEnd{ atDocumentEnd ? documentEndExclusive : endExclusive };
+ const auto utrEnd{ atDocumentEnd ? documentEndExclusive : static_cast<COORD>(endExclusive) };


/permissive- implies /Zc:ternary, which requires both sides of a ternary operator to be of the same type, or implicitly convertible, which this isn't.

lhecker · 2021-11-24T22:00:51Z

src/terminal/parser/ft_fuzzer/fuzzing_logic.h

+
+ // Flips a random byte value within the buffer.
+ template<typename _Type>
+ static _Type* _fz_flipByte(__inout_ecount(rcelms) _Type* p, __inout size_t& rcelms)


These functions are used inside CFuzzLogic, but depend on CFuzzLogic themselves.
Since templates aren't macros anymore, we now need to forward declare CFuzzLogic, then define these and finally define CFuzzLogic.

zadjii-msft · 2021-12-01T11:57:46Z

src/host/alias.cpp

@@ -1216,7 +1216,7 @@ std::wstring Alias::s_MatchAndCopyAlias(const std::wstring& sourceText,
 // - LineCount - aliases can contain multiple commands. $T is the command separator
 // Return Value:
 // - None. It will just maintain the source as the target if we can't match an alias.
-void Alias::s_MatchAndCopyAliasLegacy(_In_reads_bytes_(cbSource) PWCHAR pwchSource,
+void Alias::s_MatchAndCopyAliasLegacy(_In_reads_bytes_(cbSource) PCWCH pwchSource,


this explains.... years... of bugs

miniksa · 2021-12-01T17:55:29Z

src/renderer/atlas/AtlasEngine.h

@@ -359,7 +359,9 @@ namespace Microsoft::Console::Render

 bool is_inline() const noexcept
 {
- return (__builtin_bit_cast(uintptr_t, allocated) & 1) != 0;
+ // VSO-1430353: __builtin_bitcast crashes the compiler under /permissive-.


Suggested change

// VSO-1430353: __builtin_bitcast crashes the compiler under /permissive-.

// VSO-1430353: __builtin_bitcast crashes the compiler under /permissive-. (BODGY)

working around compiler bugs is a BODGE

miniksa · 2021-12-01T17:57:46Z

src/server/ApiSorter.cpp

 {
- Status = STATUS_ILLEGAL_FUNCTION;
- goto Complete;
+ Message->SetReplyStatus(STATUS_ILLEGAL_FUNCTION);


Yeah that was annoying. Good fix.

miniksa · 2021-12-01T17:58:56Z

src/server/IoDispatchers.cpp

+ const auto cleanup = wil::scope_exit([&]() noexcept {
+ UnlockConsole();
+
+ if (!NT_SUCCESS(Status))
+ {
+ pReceiveMsg->SetReplyStatus(Status);
+ if (ProcessData != nullptr)
+ {
+ CommandHistory::s_Free(ProcessData);
+ gci.ProcessHandleList.FreeProcessData(ProcessData);
+ }
+ }
+ });
+


Oh it's so beautiful.

miniksa

Love it. Hope we can get closer to doing it in Razzle too.

DHowett

I haven't read the code but I am blocking until we clear or complete the TODOs from the PR body!

DHowett · 2021-12-02T00:12:55Z

src/cascadia/TerminalControl/pch.h

@@ -1,4 +1,4 @@
-// Copyright (c) Microsoft Corporation.
+// Copyright (c) Microsoft Corporation.


This should be a conflict, right? Adding a BOM is bad, removing one is a conflict?

This file currently has a BOM. So this should be a removal. Maybe git ignores it during merge?
I'll merge main into this PR tomorrow just to be sure.

DHowett · 2021-12-02T00:23:20Z

src/host/init.cpp

@@ -30,38 +28,35 @@ void InitSideBySide(_Out_writes_(ScratchBufferSize) PWSTR ScratchBuffer, __range
 // make references to DLLs in the system that are in the SxS cache (ex. a 3rd party IME is loaded and asks for
 // comctl32.dll. The load will fail if SxS wasn't initialized.) This was bug# WIN7:681280.

- // We look at the first few chars without being careful about a terminal nul, so init them.


we have lost the invariant that the scratch buffer is cleared on failure. is that okay?

if you could write a bit about this change, it would be easier to accept! :)

I checked the only caller of this function and the name of the argument is correct: It really is just a "scratch(pad) buffer" so that the callee doesn't need to allocate on the heap. It can be left uncleared on return. Additionally the caller calls this function last, right before returning and as such there's no one there to read from it anymore.

DHowett · 2021-12-02T00:26:25Z

src/host/init.cpp

 {
 NtToWin32PathOffset = 4;
 }

+ ACTCTXW actctx{};


WAIT a second.

Can ACTCTX not take an NT path here in 2021 on Windows 8.1+?

Can we just use wil::GetModuleFilenameW<std::wstring>, kill the scratch buffer, and let it allocate? Is that illegal?

DHowett · 2021-12-02T00:26:58Z

src/host/ut_host/ReadWaitTests.cpp

@@ -118,7 +118,7 @@ class InputRecordConversionTests
 dbcsChars,
 INPUT_RECORD_COUNT * 2,
 nullptr,
- false);
+ FALSE);


DHowett · 2021-12-02T00:28:24Z

src/host/ut_host/TextBufferIteratorTests.cpp

+template<typename T>
+T GetIterator()
+{
+}


ouch. okay this makes sense.

We could also leave them without bodies, correct?

template<typename T> T GetIterator();

and then provide specializations? Therefore, if we call it with a T that has not been specialized we get a linker error?

Or does this provide a compiler error?

Actually, how does this not provide an error in a compliant parser? There's no return, it should fail before we get to template generation.

DHowett · 2021-12-02T00:31:16Z

src/renderer/atlas/AtlasEngine.h

@@ -359,7 +359,9 @@ namespace Microsoft::Console::Render

 bool is_inline() const noexcept
 {
- return (__builtin_bit_cast(uintptr_t, allocated) & 1) != 0;
+ // VSO-1430353: __builtin_bitcast crashes the compiler under /permissive-.


Suggested change

// VSO-1430353: __builtin_bitcast crashes the compiler under /permissive-.

// VSO-1430353: __builtin_bitcast crashes the compiler under /permissive-. (BODGY)

working around compiler bugs is a BODGE

DHowett · 2021-12-02T00:36:11Z

src/server/IoDispatchers.cpp

+ const auto cleanup = wil::scope_exit([&]() noexcept {
+ UnlockConsole();
+
+ if (!NT_SUCCESS(Status))
+ {
+ pReceiveMsg->SetReplyStatus(Status);
+ if (ProcessData != nullptr)
+ {
+ CommandHistory::s_Free(ProcessData);
+ gci.ProcessHandleList.FreeProcessData(ProcessData);
+ }
+ }
+ });
+


DHowett · 2021-12-02T00:37:42Z

src/types/inc/viewport.hpp

+ constexpr bool operator==(const Viewport& other) const noexcept
+ {
+ return _sr == other._sr;
+ }
+
+ constexpr bool operator!=(const Viewport& other) const noexcept
+ {
+ return _sr != other._sr;
+ }


how strange.

DHowett · 2021-12-02T00:38:01Z

src/types/inc/viewport.hpp

+ constexpr bool operator==(const Viewport& other) const noexcept
+ {
+ return _sr == other._sr;
+ }
+
+ constexpr bool operator!=(const Viewport& other) const noexcept
+ {
+ return _sr != other._sr;
+ }


I've always wondered why we did out-of-body inline operators and didn't just put them in the classes

DHowett · 2021-12-02T00:38:36Z

src/winconpty/ft_pty/ConPtyTests.cpp

 {
 return _CreatePseudoConsole(INVALID_HANDLE_VALUE, size, hInput, hOutput, dwFlags, pPty);
 }

-HRESULT AttachPseudoConsole(HPCON hPC, LPPROC_THREAD_ATTRIBUTE_LIST lpAttributeList)
+static HRESULT AttachPseudoConsole(HPCON hPC, std::wstring& command, PROCESS_INFORMATION* ppi)


o_O why'd you refactor this out?

sorry, why'd you refactor this back in?

ah, code duplication. Ok, sure

IIRC the problem here was the "you can't pass string literals as mutable wchar_t* pointers" issue, but fixing it in the existing code was a hassle. So I just refactored the test real quick. As it turns out I was able to remove a lot of code duplication too.

…conformance

DHowett · 2021-12-08T20:00:24Z

Let's do it! EIM!

ghost · 2021-12-08T20:00:33Z

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost · 2022-02-03T18:23:15Z

🎉Windows Terminal Preview v1.13.10336.0 has been released which incorporates this pull request.:tada:

Handy links:

ghost added Area-Build Issues pertaining to the build system, CI, infrastructure, meta Issue-Task It's a feature request, but it doesn't really need a major design. Product-Meta The product is the management of the products. labels Nov 24, 2021

lhecker commented Nov 24, 2021

View reviewed changes

src/host/init.cpp Outdated Show resolved Hide resolved

lhecker added this to the Engineering Improvements 2021 milestone Nov 24, 2021

lhecker commented Nov 24, 2021

View reviewed changes

lhecker force-pushed the dev/lhecker/stricter-conformance branch from b58d0fc to 6192935 Compare November 24, 2021 22:28

Enable /permissive- and remaining /Zc flags

1d68188

lhecker force-pushed the dev/lhecker/stricter-conformance branch from 6192935 to 1d68188 Compare November 24, 2021 23:28

lhecker marked this pull request as ready for review November 24, 2021 23:57

zadjii-msft approved these changes Dec 1, 2021

View reviewed changes

miniksa reviewed Dec 1, 2021

View reviewed changes

miniksa approved these changes Dec 1, 2021

View reviewed changes

DHowett requested changes Dec 2, 2021

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 2, 2021

DHowett reviewed Dec 2, 2021

View reviewed changes

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Dec 2, 2021

zadjii-msft assigned DHowett Dec 2, 2021

lhecker added 4 commits December 3, 2021 16:37

Merge remote-tracking branch 'origin/main' into dev/lhecker/stricter-…

975a3aa

…conformance

Addressed feedback

afd60ca

Fix compilation error

b3466a3

Fix formatting

3e43585

DHowett approved these changes Dec 8, 2021

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 8, 2021

ghost merged commit 2b202ce into main Dec 8, 2021

ghost deleted the dev/lhecker/stricter-conformance branch December 8, 2021 20:00

ghost mentioned this pull request Feb 3, 2022

[MSVC][Build][Permissive-][std:c++latest] Terminal failed to build on MSVC #10703

Closed

zadjii-msft mentioned this pull request Jul 14, 2022

Bad code in src/terminal/parser/ft_fuzzer/fuzzing_directed.h #13501

Closed

This pull request was closed.

	// VSO-1430353: __builtin_bitcast crashes the compiler under /permissive-.
	// VSO-1430353: __builtin_bitcast crashes the compiler under /permissive-. (BODGY)

		@@ -1,4 +1,4 @@
		// Copyright (c) Microsoft Corporation.
		// Copyright (c) Microsoft Corporation.

Enable /permissive- and remaining /Zc flags #11816

Enable /permissive- and remaining /Zc flags #11816

Conversation

lhecker commented Nov 24, 2021 • edited Loading

PR Checklist

lhecker commented Nov 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Nov 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Nov 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Nov 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miniksa left a comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett commented Dec 8, 2021

ghost commented Dec 8, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Feb 3, 2022

lhecker commented Nov 24, 2021 •

edited

Loading

lhecker commented Nov 24, 2021 •

edited

Loading

lhecker Nov 24, 2021 •

edited

Loading

lhecker Nov 24, 2021 •

edited

Loading

lhecker Nov 24, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.