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

[clang] Enable sized deallocation by default in C++14 onwards #83774

Merged
merged 1 commit into from
Apr 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion clang-tools-extra/clangd/unittests/FindTargetTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -839,7 +839,9 @@ TEST_F(TargetDeclTest, OverloadExpr) {
[[delete]] x;
}
)cpp";
EXPECT_DECLS("CXXDeleteExpr", "void operator delete(void *) noexcept");
// Sized deallocation is enabled by default in C++14 onwards.
EXPECT_DECLS("CXXDeleteExpr",
"void operator delete(void *, unsigned long) noexcept");
}

TEST_F(TargetDeclTest, DependentExprs) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,6 @@ struct S {
// CHECK-MESSAGES: :[[@LINE+1]]:7: warning: declaration of 'operator new' has no matching declaration of 'operator delete' at the same scope
void *operator new(size_t size) noexcept(false);

struct T {
// Sized deallocations are not enabled by default, and so this new/delete pair
// does not match. However, we expect only one warning, for the new, because
// the operator delete is a placement delete and we do not warn on mismatching
// placement operations.
// CHECK-MESSAGES: :[[@LINE+1]]:9: warning: declaration of 'operator new' has no matching declaration of 'operator delete' at the same scope
void *operator new(size_t size) noexcept;
void operator delete(void *ptr, size_t) noexcept; // ok only if sized deallocation is enabled
};

struct U {
void *operator new(size_t size) noexcept;
void operator delete(void *ptr) noexcept;
Expand Down
5 changes: 5 additions & 0 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,11 @@ C++ Language Changes
--------------------
- Implemented ``_BitInt`` literal suffixes ``__wb`` or ``__WB`` as a Clang extension with ``unsigned`` modifiers also allowed. (#GH85223).

C++14 Feature Support
^^^^^^^^^^^^^^^^^^^^^
- Sized deallocation is enabled by default in C++14 onwards. The user may specify
``-fno-sized-deallocation`` to disable it if there are some regressions.

C++20 Feature Support
^^^^^^^^^^^^^^^^^^^^^

Expand Down
8 changes: 4 additions & 4 deletions clang/include/clang/Driver/Options.td
Original file line number Diff line number Diff line change
Expand Up @@ -603,6 +603,7 @@ class MarshallingInfoVisibility<KeyPathAndMacro kpm, code default>
// Key paths that are constant during parsing of options with the same key path prefix.
defvar cplusplus = LangOpts<"CPlusPlus">;
defvar cpp11 = LangOpts<"CPlusPlus11">;
defvar cpp14 = LangOpts<"CPlusPlus14">;
defvar cpp17 = LangOpts<"CPlusPlus17">;
defvar cpp20 = LangOpts<"CPlusPlus20">;
defvar c99 = LangOpts<"C99">;
Expand Down Expand Up @@ -3365,10 +3366,9 @@ defm relaxed_template_template_args : BoolFOption<"relaxed-template-template-arg
"Enable C++17 relaxed template template argument matching">,
NegFlag<SetFalse>>;
defm sized_deallocation : BoolFOption<"sized-deallocation",
LangOpts<"SizedDeallocation">, DefaultFalse,
PosFlag<SetTrue, [], [ClangOption, CC1Option],
"Enable C++14 sized global deallocation functions">,
NegFlag<SetFalse>>;
LangOpts<"SizedDeallocation">, Default<cpp14.KeyPath>,
PosFlag<SetTrue, [], [], "Enable C++14 sized global deallocation functions">,
NegFlag<SetFalse>, BothFlags<[], [ClangOption, CC1Option]>>;
defm aligned_allocation : BoolFOption<"aligned-allocation",
LangOpts<"AlignedAllocation">, Default<cpp17.KeyPath>,
PosFlag<SetTrue, [], [ClangOption], "Enable C++17 aligned allocation functions">,
Expand Down
13 changes: 9 additions & 4 deletions clang/lib/Driver/ToolChains/Clang.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7294,10 +7294,15 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
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.
Args.addOptInFlag(CmdArgs, options::OPT_fsized_deallocation,
options::OPT_fno_sized_deallocation);
// -fsized-deallocation is on by default in C++14 onwards and otherwise off
// by default.
if (Arg *A = Args.getLastArg(options::OPT_fsized_deallocation,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only need -fno-sized-deallocation for CC1Option. When neither option is specified, assume implicit -fsized-deallocation.

The code change in Darwin.cpp should be changed to a function that decides whether the default is yes or no.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I may not understand what you mean, can I leave it unchanged as this code is just like -faligned-allocation. We can change it later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my other comment. I think this should be fixed.

options::OPT_fno_sized_deallocation)) {
if (A->getOption().matches(options::OPT_fno_sized_deallocation))
CmdArgs.push_back("-fno-sized-deallocation");
Copy link
Member

@MaskRay MaskRay Apr 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not pass -fsized-deallocation to cc1, as -fno-sized-deallocation is not a cc1 option and will cause a failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if I understand correctly, but I think we need both clang and cc1 options for sized_deallocation. Please see aligned_allocation case, which is the same as sized_deallocation:

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

And there are a lot of tests assumed we have cc1 options -fno-sized-deallocation and -fsized-deallocation like clang/test/SemaCXX/builtin-operator-new-delete.cpp.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for bothering you, but I'd like to know if it makes sense to you. @MaskRay

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. There are many -fsize-deallocation in existing tests. I can clean up them this patch lands.

else
CmdArgs.push_back("-fsized-deallocation");
}

// -faligned-allocation is on by default in C++17 onwards and otherwise off
// by default.
Expand Down
58 changes: 55 additions & 3 deletions clang/lib/Driver/ToolChains/Darwin.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2912,16 +2912,68 @@ static bool sdkSupportsBuiltinModules(const Darwin::DarwinPlatformKind &TargetPl
}
}

void Darwin::addClangTargetOptions(const llvm::opt::ArgList &DriverArgs,
llvm::opt::ArgStringList &CC1Args,
Action::OffloadKind DeviceOffloadKind) const {
static inline llvm::VersionTuple
sizedDeallocMinVersion(llvm::Triple::OSType OS) {
switch (OS) {
default:
break;
case llvm::Triple::Darwin:
case llvm::Triple::MacOSX: // Earliest supporting version is 10.12.
wangpc-pp marked this conversation as resolved.
Show resolved Hide resolved
return llvm::VersionTuple(10U, 12U);
case llvm::Triple::IOS:
case llvm::Triple::TvOS: // Earliest supporting version is 10.0.0.
return llvm::VersionTuple(10U);
case llvm::Triple::WatchOS: // Earliest supporting version is 3.0.0.
return llvm::VersionTuple(3U);
}

llvm_unreachable("Unexpected OS");
}

bool Darwin::isSizedDeallocationUnavailable() const {
llvm::Triple::OSType OS;

if (isTargetMacCatalyst())
return TargetVersion < sizedDeallocMinVersion(llvm::Triple::MacOSX);
switch (TargetPlatform) {
case MacOS: // Earlier than 10.12.
OS = llvm::Triple::MacOSX;
break;
case IPhoneOS:
OS = llvm::Triple::IOS;
break;
case TvOS: // Earlier than 10.0.
OS = llvm::Triple::TvOS;
break;
case WatchOS: // Earlier than 3.0.
OS = llvm::Triple::WatchOS;
break;
case DriverKit:
case XROS:
// Always available.
return false;
}

return TargetVersion < sizedDeallocMinVersion(OS);
}

void Darwin::addClangTargetOptions(
const llvm::opt::ArgList &DriverArgs, llvm::opt::ArgStringList &CC1Args,
Action::OffloadKind DeviceOffloadKind) const {
// Pass "-faligned-alloc-unavailable" only when the user hasn't manually
// enabled or disabled aligned allocations.
if (!DriverArgs.hasArgNoClaim(options::OPT_faligned_allocation,
options::OPT_fno_aligned_allocation) &&
isAlignedAllocationUnavailable())
CC1Args.push_back("-faligned-alloc-unavailable");

// Pass "-fno-sized-deallocation" only when the user hasn't manually enabled
// or disabled sized deallocations.
if (!DriverArgs.hasArgNoClaim(options::OPT_fsized_deallocation,
options::OPT_fno_sized_deallocation) &&
isSizedDeallocationUnavailable())
CC1Args.push_back("-fno-sized-deallocation");

addClangCC1ASTargetOptions(DriverArgs, CC1Args);

// Enable compatibility mode for NSItemProviderCompletionHandler in
Expand Down
4 changes: 4 additions & 0 deletions clang/lib/Driver/ToolChains/Darwin.h
Original file line number Diff line number Diff line change
Expand Up @@ -511,6 +511,10 @@ class LLVM_LIBRARY_VISIBILITY Darwin : public MachO {
/// targeting.
bool isAlignedAllocationUnavailable() const;

/// Return true if c++14 sized deallocation functions are not implemented in
/// the c++ standard library of the deployment target we are targeting.
bool isSizedDeallocationUnavailable() const;

void addClangTargetOptions(const llvm::opt::ArgList &DriverArgs,
llvm::opt::ArgStringList &CC1Args,
Action::OffloadKind DeviceOffloadKind) const override;
Expand Down
6 changes: 6 additions & 0 deletions clang/lib/Driver/ToolChains/ZOS.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,12 @@ void ZOS::addClangTargetOptions(const ArgList &DriverArgs,
if (!DriverArgs.hasArgNoClaim(options::OPT_faligned_allocation,
options::OPT_fno_aligned_allocation))
CC1Args.push_back("-faligned-alloc-unavailable");

// Pass "-fno-sized-deallocation" only when the user hasn't manually enabled
// or disabled sized deallocations.
if (!DriverArgs.hasArgNoClaim(options::OPT_fsized_deallocation,
options::OPT_fno_sized_deallocation))
CC1Args.push_back("-fno-sized-deallocation");
}

void zos::Assembler::ConstructJob(Compilation &C, const JobAction &JA,
Expand Down
2 changes: 1 addition & 1 deletion clang/test/AST/ast-dump-expr-json.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2333,7 +2333,7 @@ void TestNonADLCall3() {
// CHECK-NEXT: "kind": "FunctionDecl",
// CHECK-NEXT: "name": "operator delete",
// CHECK-NEXT: "type": {
// CHECK-NEXT: "qualType": "void (void *) noexcept"
// CHECK-NEXT: "qualType": "void (void *, unsigned long) noexcept"
// CHECK-NEXT: }
// CHECK-NEXT: },
// CHECK-NEXT: "inner": [
Expand Down
2 changes: 1 addition & 1 deletion clang/test/AST/ast-dump-expr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ void UnaryExpressions(int *p) {
// CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}} <col:8> 'int *' lvalue ParmVar 0x{{[^ ]*}} 'p' 'int *'

::delete p;
// CHECK: CXXDeleteExpr 0x{{[^ ]*}} <line:[[@LINE-1]]:3, col:12> 'void' global Function 0x{{[^ ]*}} 'operator delete' 'void (void *) noexcept'
// CHECK: CXXDeleteExpr 0x{{[^ ]*}} <line:[[@LINE-1]]:3, col:12> 'void' global Function 0x{{[^ ]*}} 'operator delete' 'void (void *, unsigned long) noexcept'
// CHECK-NEXT: ImplicitCastExpr
// CHECK-NEXT: DeclRefExpr 0x{{[^ ]*}} <col:12> 'int *' lvalue ParmVar 0x{{[^ ]*}} 'p' 'int *'

Expand Down
Loading
Loading