[clang-cl][MSVC] Enable /Zc:alignedNew for C++17 and /Zc:sizedDealloc by default

[steplong]

Summary

MSVC turns on /Zc:alignedNew for C++17 by default and /Zc:sizedDealloc by default

https://docs.microsoft.com/en-us/cpp/build/reference/zc-conformance?view=msvc-170

[steplong]

I'm not sure how to check for /std: at this stage to turn on arguments like /Zc:alignedNew

clang/test/Driver/cl-zc.cpp

108

This line will fail, but I can't find any documentation on thread safe statics being off for versions < 19. https://docs.microsoft.com/en-us/cpp/build/reference/zc-threadsafeinit-thread-safe-local-static-initialization?view=msvc-170 says Visual Studio 2015 implements this by default.

[steplong]

• Ignore /Zc:wchar_t

[hans]

clang/test/Driver/cl-zc.cpp

108

VS 2015 has _MSC_VER 1900.
The test was added here: https://github.com/llvm/llvm-project/commit/c371ff048df8731052976f4e628ed1861cf61cfd and I assume David verified locally that 19 is the real cutoff.
I just tested locally with VS 2013, and it is indeed off there

C:\src\tmp>type a.cc && cl -c a.cc && dumpbin /disasm a.obj
int g();

int f() {
    static int x = g();
    return x;
}
Microsoft (R) C/C++ Optimizing Compiler Version 18.00.31101 for x86
Copyright (C) Microsoft Corporation.  All rights reserved.

a.cc
Microsoft (R) COFF/PE Dumper Version 12.00.31101.0
Copyright (C) Microsoft Corporation.  All rights reserved.


Dump of file a.obj

File Type: COFF OBJECT

?f@@YAHXZ (int __cdecl f(void)):
  00000000: 55                 push        ebp
  00000001: 8B EC              mov         ebp,esp
  00000003: A1 00 00 00 00     mov         eax,dword ptr [?$S1@?1??f@@YAHXZ@4IA]
  00000008: 83 E0 01           and         eax,1
  0000000B: 75 19              jne         00000026
  0000000D: 8B 0D 00 00 00 00  mov         ecx,dword ptr [?$S1@?1??f@@YAHXZ@4IA]
  00000013: 83 C9 01           or          ecx,1
  00000016: 89 0D 00 00 00 00  mov         dword ptr [?$S1@?1??f@@YAHXZ@4IA],ecx
  0000001C: E8 00 00 00 00     call        ?g@@YAHXZ
  00000021: A3 00 00 00 00     mov         dword ptr [?x@?1??f@@YAHXZ@4HA],eax
  00000026: A1 00 00 00 00     mov         eax,dword ptr [?x@?1??f@@YAHXZ@4HA]
  0000002B: 5D                 pop         ebp
  0000002C: C3                 ret

  Summary

           8 .bss
          64 .debug$S
          2F .drectve
          2D .text$mn

clang/tools/driver/driver.cpp

237 ↗

(On Diff #436592)

Instead of injecting flags like this which seems a bit crude and won't work in non-clang-cl mode, can't we just enable these features keyed off the MSVC version like we already do for e.g. thread-safe statics?

[steplong]

• Only enable /Zc:alignedNew and /Zc:sizedDealloc

[steplong]

clang/lib/Driver/ToolChains/Clang.cpp

6654

There's a check here for faligned-allocation, but I wasn't sure how to get the c++ standard here. Also, I couldn't figure a clean way to compare if the standard was c++17 onwards, so I just enabled -faligned-allocation for C++17 above

[rnk]

+@luken-google

clang/lib/Driver/ToolChains/Clang.cpp

6514

Only for C++17, and not C++17 and above?

6654

What I would do is to compute a boolean above and use it as the default value here. The -cc1 line should be canonical, it shouldn't have duplicate, potentially contradictory flags.

[steplong]

• Change logic for /Zc:alignedNew and /Zc:sizedDealloc
• Add tests for different /std:c++

[hans]

clang/lib/Driver/ToolChains/Clang.cpp

6642–6650

I wonder at what version they did that though. Maybe we need to take that into consideration?

[steplong]

https://docs.microsoft.com/en-us/cpp/build/reference/zc-sizeddealloc-enable-global-sized-dealloc-functions?view=msvc-170 says it implements this by default starting on MSVC 2015. I don't have an older version of MSVC, so maybe someone can confirm for me

[hans]

lgtm

(If you like, consider dropping the "-DEFAULT" part from the new filecheck prefixes. I don't think they really add much value.)

[steplong]

• Remove DEFAULT from test

[steplong]

It looks like misc-new-delete-overloads.cpp is failing on line 20:

 1 // RUN: %check_clang_tidy %s misc-new-delete-overloads %t
 2
 3 typedef decltype(sizeof(int)) size_t;
 4
 5 struct S {
 6   // CHECK-MESSAGES: :[[@LINE+1]]:9: warning: declaration of 'operator new' has no matching declaration of 'operator delete' at the same scope [misc-new-delete-overloads]
 7   void *operator new(size_t size) noexcept;
 8   // CHECK-MESSAGES: :[[@LINE+1]]:9: warning: declaration of 'operator new[]' has no matching declaration of 'operator delete[]' at the same scope
 9   void *operator new[](size_t size) noexcept;
10 };
11
12 // CHECK-MESSAGES: :[[@LINE+1]]:7: warning: declaration of 'operator new' has no matching declaration of 'operator delete' at the same scope
13 void *operator new(size_t size) noexcept(false);
14
15 struct T {
16   // Sized deallocations are not enabled by default, and so this new/delete pair
17   // does not match. However, we expect only one warning, for the new, because
18   // the operator delete is a placement delete and we do not warn on mismatching
19   // placement operations.
20   // CHECK-MESSAGES: :[[@LINE+1]]:9: warning: declaration of 'operator new' has no matching declaration of 'operator delete' at the same scope
21   void *operator new(size_t size) noexcept;
22   void operator delete(void *ptr, size_t) noexcept; // ok only if sized deallocation is enabled
23 };

On Line 16, it says sized deallocations are not enabled by default, but this patch turns it on by default for MSVC. Should I unsupport this test for windows or is there a way to pass /Zc:sizedDealloc- only for windows?

[hans]

In D127641#3597360, @steplong wrote:

It looks like misc-new-delete-overloads.cpp is failing on line 20:
..
On Line 16, it says sized deallocations are not enabled by default, but this patch turns it on by default for MSVC. Should I unsupport this test for windows or is there a way to pass /Zc:sizedDealloc- only for windows?

You could add a target to the test so that it doesn't target windows-msvc, e.g. -target x86_64-unknown-linux.

[steplong]

• Add -target x86_64-unknown-linux to misc-new-delete-overloads.cpp

[steplong]

Whoops, this is a clang-tidy test, so -target won't work here.

[steplong]

Hmm, a StaticAnalyzer unit test is failing. It looks it is expecting "test.CXXDeallocator: NumArgs: 1\n", but getting `"test.CXXDeallocator: NumArgs: 2\n". I'm assuming it is because sized deallocation is enabled on default for MSVC now

[steplong]

• Change unittest to match Num Args 1 or 2

I won't merge this. Just want to see if this passes the unittest

[steplong]

• Add necessary includes to use EXPECT_THAT and MatchesRegex

[steplong]

• Change (1|2) to [12]. Test is failing because () isn't supported

[steplong]

• Update unit test to see if this regex works

[thakis]

clang/lib/Driver/ToolChains/Clang.cpp

6662

Do we want to do this in the driver? From what I understand, we already do this (MSVC or not) at the Driver->Frontend boundary. See Options.td:

defm aligned_allocation : BoolFOption<"aligned-allocation",
  LangOpts<"AlignedAllocation">, Default<cpp17.KeyPath>,
  PosFlag<SetTrue, [], "Enable C++17 aligned allocation functions">,
  NegFlag<SetFalse>, BothFlags<[CC1Option]>>;

It defaults to on when c++17 is set, and makes its way to LangOpts.AlignedAllocation.

Does this change here have any effect? From looking at the code, this should be the current behavior already (?)

[steplong]

clang/lib/Driver/ToolChains/Clang.cpp

6662

Hmmm, my test regarding checking for -faligned-allocation fails if I remove the code here.

[steplong]

• Clang-format test

The regex isn't optimal since it will still accept strings that aren't "Num Args: 1\n" or "Num Args: 2\n" such as "Num Args: \n2\n"

[steplong]

@thakis @hans What do you both think of the latest version? I would like to get this merged

clang/lib/Driver/ToolChains/Clang.cpp

if (const Arg *StdArg = Args.getLastArg(options::OPT__SLASH_std)) {
                              .Case("c17", "-std=c17")
                              .Default("");
       if (LanguageStandard.empty())
         D.Diag(clang::diag::warn_drv_unused_argument)
             << StdArg->getAsString(Args);
     }
     CmdArgs.push_back(LanguageStandard.data());
   }
+  bool IsCPP17Onwards = false;
   if (ImplyVCPPCXXVer) {
     StringRef LanguageStandard;
     if (const Arg *StdArg = Args.getLastArg(options::OPT__SLASH_std)) {
       Std = StdArg;
       LanguageStandard = llvm::StringSwitch<StringRef>(StdArg->getValue())
                              .Case("c++14", "-std=c++14")
                              .Case("c++17", "-std=c++17")
                              .Case("c++20", "-std=c++20")
                              .Case("c++latest", "-std=c++2b")
                              .Default("");
       if (LanguageStandard.empty())
         D.Diag(clang::diag::warn_drv_unused_argument)
             << StdArg->getAsString(Args);
     }
 
     if (LanguageStandard.empty()) {
       if (IsMSVC2015Compatible)
         LanguageStandard = "-std=c++14";
       else
         LanguageStandard = "-std=c++11";
     }
 
     CmdArgs.push_back(LanguageStandard.data());
+
+    IsCPP17Onwards =
+        LanguageStandard != "-std=c++11" && LanguageStandard != "-std=c++14";
   }
 
   Args.addOptInFlag(CmdArgs, options::OPT_fborland_extensions,
                     options::OPT_fno_borland_extensions);
 
   // -fno-declspec is default, except for PS4/PS5.
   if (Args.hasFlag(options::OPT_fdeclspec, options::OPT_fno_declspec,
                    RawTriple.isPS()))
     CmdArgs.push_back("-fdeclspec");
   else if (Args.hasArg(options::OPT_fno_declspec))

 
   // -frelaxed-template-template-args is off by default, as it is a severe
   // breaking change until a corresponding change to template partial ordering
   // is provided.
   Args.addOptInFlag(CmdArgs, options::OPT_frelaxed_template_template_args,
                     options::OPT_fno_relaxed_template_template_args);
 
   // -fsized-deallocation is off by default, as it is an ABI-breaking change for
-  // most platforms.
+  // most platforms. MSVC turns on /Zc:sizedDealloc by default, starting in
+  // MSVC 2015.
+  if (IsWindowsMSVC && IsMSVC2015Compatible &&
+      !Args.getLastArg(options::OPT_fsized_deallocation,
+                       options::OPT_fno_sized_deallocation))
+    CmdArgs.push_back("-fsized-deallocation");
+  else
     Args.addOptInFlag(CmdArgs, options::OPT_fsized_deallocation,
                       options::OPT_fno_sized_deallocation);
 
   // -faligned-allocation is on by default in C++17 onwards and otherwise off
   // by default.
   if (Arg *A = Args.getLastArg(options::OPT_faligned_allocation,
                                options::OPT_fno_aligned_allocation,
                                options::OPT_faligned_new_EQ)) {
     if (A->getOption().matches(options::OPT_fno_aligned_allocation))
       CmdArgs.push_back("-fno-aligned-allocation");
     else
       CmdArgs.push_back("-faligned-allocation");
+  } else if (IsCPP17Onwards) {
+    CmdArgs.push_back("-faligned-allocation");
   }
 
   // The default new alignment can be specified using a dedicated option or via
   // a GCC-compatible option that also turns on aligned allocation.
   if (Arg *A = Args.getLastArg(options::OPT_fnew_alignment_EQ,
                                options::OPT_faligned_new_EQ))
     CmdArgs.push_back(
         Args.MakeArgString(Twine("-fnew-alignment=") + A->getValue()));

clang/test/Driver/cl-zc.cpp

 // Note: %s must be preceded by --, otherwise it may be interpreted as a
 // command-line option, e.g. on Mac where %s is commonly under /Users.
 
+// RUN: %clang_cl /c -### -- %s 2>&1 | FileCheck -check-prefix=SIZED-DEALLOC-DEFAULT %s
+// RUN: %clang_cl /c -### -fms-compatibility-version=18 -- %s 2>&1 | FileCheck -check-prefix=SIZED-DEALLOC-OFF-DEFAULT %s
+// SIZED-DEALLOC-DEFAULT: "-fsized-deallocation"
+// SIZED-DEALLOC-OFF-DEFAULT-NOT: "-fsized-deallocation"
+
+// RUN: %clang_cl /c /std:c++11 -### -- %s 2>&1 | FileCheck -check-prefix=ALIGNED-NEW-BEFORE-CPP17-DEFAULT %s
+// RUN: %clang_cl /c /std:c++14 -### -- %s 2>&1 | FileCheck -check-prefix=ALIGNED-NEW-BEFORE-CPP17-DEFAULT %s
+// RUN: %clang_cl /c /std:c++17 -### -- %s 2>&1 | FileCheck -check-prefix=ALIGNED-NEW-CPP17ONWARDS-DEFAULT %s
+// RUN: %clang_cl /c /std:c++20 -### -- %s 2>&1 | FileCheck -check-prefix=ALIGNED-NEW-CPP17ONWARDS-DEFAULT %s
+// RUN: %clang_cl /c /std:c++latest -### -- %s 2>&1 | FileCheck -check-prefix=ALIGNED-NEW-CPP17ONWARDS-DEFAULT %s
+// ALIGNED-NEW-BEFORE-CPP17-DEFAULT-NOT: "-faligned-allocation"
+// ALIGNED-NEW-CPP17ONWARDS-DEFAULT: "-faligned-allocation"
+
 // RUN: %clang_cl /c -### -- %s 2>&1 | FileCheck -check-prefix=TRIGRAPHS-DEFAULT %s
 // cc1 will disable trigraphs for -fms-compatibility as long as -ftrigraphs
 // isn't explicitly passed.
 // TRIGRAPHS-DEFAULT-NOT: "-ftrigraphs"
 
 // RUN: %clang_cl /c -### /Zc:trigraphs -- %s 2>&1 | FileCheck -check-prefix=TRIGRAPHS-ON %s
 // TRIGRAPHS-ON: "-ftrigraphs"
 

 // INLINE-OFF: argument unused during compilation
 
 // RUN: %clang_cl /c -### /Zc:ternary -- %s 2>&1 | FileCheck -check-prefix=TERNARY-ON %s
 // TERNARY-ON-NOT: argument unused during compilation
 // RUN: %clang_cl /c -### /Zc:ternary- -- %s 2>&1 | FileCheck -check-prefix=TERNARY-OFF %s
 // TERNARY-OFF: argument unused during compilation
 
 // thread safe statics are off for versions < 19.
 // RUN: %clang_cl /c -### -fms-compatibility-version=18 -- %s 2>&1 | FileCheck -check-prefix=NoThreadSafeStatics %s
 // RUN: %clang_cl /Zc:threadSafeInit /Zc:threadSafeInit- /c -### -- %s 2>&1 | FileCheck -check-prefix=NoThreadSafeStatics %s
 // NoThreadSafeStatics: "-fno-threadsafe-statics"
 
 // RUN: %clang_cl /Zc:threadSafeInit /c -### -- %s 2>&1 | FileCheck -check-prefix=ThreadSafeStatics %s
 // ThreadSafeStatics-NOT: "-fno-threadsafe-statics"
 
 // RUN: %clang_cl /Zc:dllexportInlines- /c -### -- %s 2>&1 | FileCheck -check-prefix=NoDllExportInlines %s
 // NoDllExportInlines: "-fno-dllexport-inlines"