Page MenuHomePhabricator

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

Authored by steplong on Jun 13 2022, 7:45 AM.

Details

Reviewers
rnk
hans
thakis
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

Diff Detail

Event Timeline

steplong created this revision.Jun 13 2022, 7:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2022, 7:45 AM
steplong requested review of this revision.Jun 13 2022, 7:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2022, 7:45 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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 updated this revision to Diff 436592.Jun 13 2022, 3:58 PM
  • Ignore /Zc:wchar_t
hans added inline comments.Jun 14 2022, 4:59 AM
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 updated this revision to Diff 436860.Jun 14 2022, 11:08 AM
  • Only enable /Zc:alignedNew and /Zc:sizedDealloc
steplong retitled this revision from [clang-cl][MSVC] Add default /Zc conformance arguments to [clang-cl][MSVC] Enable /Zc:alignedNew for C++17 and /Zc:sizedDealloc by default.Jun 14 2022, 11:09 AM
steplong edited the summary of this revision. (Show Details)
steplong added inline comments.Jun 14 2022, 11:11 AM
clang/lib/Driver/ToolChains/Clang.cpp
6641

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

clang/lib/Driver/ToolChains/Clang.cpp
6501

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

6641

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 updated this revision to Diff 436985.Jun 14 2022, 5:38 PM
  • Change logic for /Zc:alignedNew and /Zc:sizedDealloc
  • Add tests for different /std:c++
hans added inline comments.Jun 15 2022, 7:56 AM
clang/lib/Driver/ToolChains/Clang.cpp
6629–6637

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

steplong updated this revision to Diff 437243.EditedJun 15 2022, 10:46 AM

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 accepted this revision.Jun 20 2022, 9:25 AM

lgtm

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

This revision is now accepted and ready to land.Jun 20 2022, 9:25 AM
steplong updated this revision to Diff 438437.Jun 20 2022, 10:32 AM
  • Remove DEFAULT from test
steplong added a comment.EditedJun 20 2022, 2:05 PM

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 added a comment.Jun 21 2022, 3:35 AM

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 updated this revision to Diff 438682.Jun 21 2022, 6:46 AM
  • Add -target x86_64-unknown-linux to misc-new-delete-overloads.cpp
Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2022, 6:46 AM
steplong updated this revision to Diff 438778.Jun 21 2022, 11:26 AM

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

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 updated this revision to Diff 449140.Mon, Aug 1, 2:59 PM
  • Change unittest to match Num Args 1 or 2

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

steplong updated this revision to Diff 449303.Tue, Aug 2, 9:11 AM
  • Add necessary includes to use EXPECT_THAT and MatchesRegex
steplong updated this revision to Diff 452062.Thu, Aug 11, 8:12 PM
  • Change (1|2) to [12]. Test is failing because () isn't supported