This is an archive of the discontinued LLVM Phabricator instance.

[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
6694

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
6554

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

6694

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
6682–6690

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.Aug 1 2022, 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.Aug 2 2022, 9:11 AM
  • Add necessary includes to use EXPECT_THAT and MatchesRegex
steplong updated this revision to Diff 452062.Aug 11 2022, 8:12 PM
  • Change (1|2) to [12]. Test is failing because () isn't supported
steplong updated this revision to Diff 452655.Aug 15 2022, 6:56 AM
  • Update unit test to see if this regex works
thakis added inline comments.Aug 15 2022, 8:12 AM
clang/lib/Driver/ToolChains/Clang.cpp
6702

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 added inline comments.Aug 15 2022, 10:34 AM
clang/lib/Driver/ToolChains/Clang.cpp
6702

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

steplong updated this revision to Diff 452757.Aug 15 2022, 11:46 AM
  • 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"

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