-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[Clang]Ensure correct handling of cleanup access control (#135668) #135686
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
Conversation
…#135668) The P2719 implementation refactored diagnostics for cleanup delete, and as part of that I attempted to fix handling of inaccessible cleanup operator delete. Alas the new branch was incorrect as it was performing an implicit bool conversion, which resulted in friend accessible cleanup operators incorrectly being considered erroneous and the allocation path errored out. This error however did not get diagnosed, so the result was and so we did not actually error out before codegen. Added both Sema and CodeGen tests to cover this.
c0549c4 to
62be33b
Compare
|
@llvm/pr-subscribers-clang Author: Oliver Hunt (ojhunt) ChangesThe P2719 implementation refactored diagnostics for cleanup delete, and as part of that I attempted to fix handling of inaccessible cleanup operator delete. Alas the new branch was incorrect as it was performing an implicit bool conversion, which resulted in friend accessible cleanup operators incorrectly being considered erroneous and the allocation path errored out. This error however did not get diagnosed, so the result was and so we did not actually error out before codegen. Added both Sema and CodeGen tests to cover this. Full diff: https://github.com/llvm/llvm-project/pull/135686.diff 3 Files Affected:
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index 16a39f8b5a4dd..dfb5824a1c3d7 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -1929,8 +1929,9 @@ static bool CheckDeleteOperator(Sema &S, SourceLocation StartLoc,
}
return true;
}
-
- return S.CheckAllocationAccess(StartLoc, Range, NamingClass, Decl, Diagnose);
+ Sema::AccessResult Accessible =
+ S.CheckAllocationAccess(StartLoc, Range, NamingClass, Decl, Diagnose);
+ return Accessible == Sema::AR_inaccessible;
}
/// Select the correct "usual" deallocation function to use from a selection of
diff --git a/clang/test/CodeGenCXX/bug135668.cpp b/clang/test/CodeGenCXX/bug135668.cpp
new file mode 100644
index 0000000000000..08743bd36bf5b
--- /dev/null
+++ b/clang/test/CodeGenCXX/bug135668.cpp
@@ -0,0 +1,38 @@
+// RUN: %clang_cc1 %s -triple arm64-apple-macosx -emit-llvm -fcxx-exceptions -fexceptions -std=c++23 -o - | FileCheck %s
+
+class TestClass {
+ public:
+ TestClass();
+ int field = 0;
+ friend class Foo;
+ static void * operator new(unsigned long size);
+ private:
+ static void operator delete(void *p);
+ };
+
+class Foo {
+public:
+ int test_method();
+};
+
+int Foo::test_method() {
+ TestClass *obj = new TestClass() ;
+ return obj->field;
+}
+
+// CHECK-LABEL: define noundef i32 @_ZN3Foo11test_methodEv
+// CHECK: [[THIS_ADDR:%.*]] = alloca ptr, align 8
+// CHECK: [[OBJ:%.*]] = alloca ptr, align 8
+// CHECK: store ptr %this, ptr [[THIS_ADDR]], align 8
+// CHECK: [[THIS1:%.*]] = load ptr, ptr [[THIS_ADDR]], align 8
+// CHECK: [[ALLOCATION:%.*]] = call noundef ptr @_ZN9TestClassnwEm(i64 noundef 4)
+// CHECK: [[INITIALIZEDOBJ:%.*]] = invoke noundef ptr @_ZN9TestClassC1Ev(ptr noundef nonnull align 4 dereferenceable(4) [[ALLOCATION]])
+// CHECK-NEXT: to label %[[INVOKE_CONT:.*]] unwind label %[[LPAD:.*]]
+// CHECK: [[INVOKE_CONT]]:
+// CHECK: store ptr [[ALLOCATION]], ptr [[OBJ]], align 8
+// CHECK: [[OBJPTR:%.*]] = load ptr, ptr [[OBJ]], align 8
+// CHECK: [[FIELDPTR:%.*]] = getelementptr inbounds nuw %class.TestClass, ptr [[OBJPTR]], i32 0, i32 0
+// CHECK: [[FIELD:%.*]] = load i32, ptr [[FIELDPTR]], align 4
+// CHECK: ret i32 [[FIELD]]
+// CHECK: [[LPAD]]:
+// CHECK: call void @_ZN9TestClassdlEPv(ptr noundef [[ALLOCATION]]) #3
diff --git a/clang/test/SemaCXX/bug135668.cpp b/clang/test/SemaCXX/bug135668.cpp
new file mode 100644
index 0000000000000..96d3d4abfd3ef
--- /dev/null
+++ b/clang/test/SemaCXX/bug135668.cpp
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -triple arm64-apple-macosx -Wall -fsyntax-only -verify %s -std=c++26 -fexceptions -fcxx-exceptions
+// expected-no-diagnostics
+
+// This test makes sure that we don't erroneously consider an accessible operator
+// delete to be inaccessible, and then discard the entire new expression.
+
+class TestClass {
+public:
+ TestClass();
+ int field = 0;
+ friend class Foo;
+ static void * operator new(unsigned long size);
+private:
+ static void operator delete(void *p);
+};
+
+class Foo {
+public:
+ int test_method();
+};
+
+int Foo::test_method() {
+ TestClass *obj = new TestClass() ;
+ return obj->field;
+}
|
cor3ntin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick fix
it's so very very stupid :O |
|
Thanks from this side too. Appreciate the speed. |
|
I'm going to go ahead and merge this as it is blocking me. I have checked it against my original case and it fixes it fine. Thanks again for the quick fix. |
|
@Sterling-Augustine sorry I was waiting for all the bots to pass |
|
No worries. The only bots left were marked "Experimental, ignore results" and they are super slow running. |
The P2719 implementation refactored diagnostics for cleanup delete, and as part of that I attempted to fix handling of inaccessible cleanup operator delete. Alas the new branch was incorrect as it was performing an implicit bool conversion, which resulted in friend accessible cleanup operators incorrectly being considered erroneous and the allocation path errored out. This error however did not get diagnosed, so the result was and so we did not actually error out before codegen.
Added both Sema and CodeGen tests to cover this.