[WIP][ObjC] objc_direct method visibility ABI #76608
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-codegen Author: Puyan Lotfi (plotfi) ChangesPosting this PR for posterity a bit earlier than I had intended because the old Phabricator is crashing (https://discourse.llvm.org/t/cant-access-https-reviews-llvm-org/75905): This patch adds the ability to expose direct ObjC methods as visible across link boundaries (dylibs). It is done so by using existing visibility attributes: This is a work in progress and there are a few pieces that should be landed with this within an LLVM release cycle so as not to cause any inconsistency in the ABI:
Much of this has been discussed ahead of time, and there is a more extensive document on this proposal at: https://docs.google.com/document/d/16CsNCA2OKWkUM_LCw_qabTcMtPiq9ODVkZHALlDvzR8 Full diff: https://github.com/llvm/llvm-project/pull/76608.diff 5 Files Affected:
diff --git a/clang/include/clang/AST/DeclObjC.h b/clang/include/clang/AST/DeclObjC.h
index f8f894b4b10d19..9a12b4007f09f6 100644
--- a/clang/include/clang/AST/DeclObjC.h
+++ b/clang/include/clang/AST/DeclObjC.h
@@ -482,6 +482,20 @@ class ObjCMethodDecl : public NamedDecl, public DeclContext {
/// True if the method is tagged as objc_direct
bool isDirectMethod() const;
+ /// True if method is meant to be visible.
+ ///
+ /// TODO: For now this is only true for a very narrow case where the method
+ /// must be direct and must also be explicitly marked as
+ /// __attribute__((visibility("default"))). In the future it may be possible
+ /// to assume that all direct (or even both direct and indirect) methods are
+ /// visible but for the time being it is prudent to provide this default
+ /// visibility behavior as an opt-in only.
+ bool hasMethodVisibilityDefault() const {
+ return isDirectMethod() &&
+ getExplicitVisibility(VisibilityForValue)
+ .value_or(HiddenVisibility) == DefaultVisibility;
+ }
+
/// True if the method has a parameter that's destroyed in the callee.
bool hasParamDestroyedInCallee() const;
diff --git a/clang/lib/AST/Mangle.cpp b/clang/lib/AST/Mangle.cpp
index d3a6b61fd2bec9..ec409da0e5da00 100644
--- a/clang/lib/AST/Mangle.cpp
+++ b/clang/lib/AST/Mangle.cpp
@@ -363,10 +363,16 @@ void MangleContext::mangleObjCMethodName(const ObjCMethodDecl *MD,
}
// \01+[ContainerName(CategoryName) SelectorName]
+ // Note: If we are mangling for an objc_direct method with external visibility
+ // then the standard mangling scheme will not work. We must replace the
+ // square braces with angle braces so in the case of visible direct objc
+ // methods it results in: +<ContainerName(CategoryName) SelectorName>
+ // This will include a prefix _ on Darwin.
if (includePrefixByte) {
OS << '\01';
}
- OS << (MD->isInstanceMethod() ? '-' : '+') << '[';
+ OS << (MD->isInstanceMethod() ? '-' : '+');
+ OS << (MD->hasMethodVisibilityDefault() ? '<' : '[');
if (const auto *CID = MD->getCategory()) {
OS << CID->getClassInterface()->getName();
if (includeCategoryNamespace) {
@@ -380,7 +386,7 @@ void MangleContext::mangleObjCMethodName(const ObjCMethodDecl *MD,
}
OS << ' ';
MD->getSelector().print(OS);
- OS << ']';
+ OS << (MD->hasMethodVisibilityDefault() ? '>' : ']');
}
void MangleContext::mangleObjCMethodNameAsSourceName(const ObjCMethodDecl *MD,
diff --git a/clang/lib/CodeGen/CGObjC.cpp b/clang/lib/CodeGen/CGObjC.cpp
index acc85165a470be..3dcd5a8334d102 100644
--- a/clang/lib/CodeGen/CGObjC.cpp
+++ b/clang/lib/CodeGen/CGObjC.cpp
@@ -761,7 +761,9 @@ void CodeGenFunction::StartObjCMethod(const ObjCMethodDecl *OMD,
const CGFunctionInfo &FI = CGM.getTypes().arrangeObjCMethodDeclaration(OMD);
if (OMD->isDirectMethod()) {
- Fn->setVisibility(llvm::Function::HiddenVisibility);
+ Fn->setVisibility(OMD->hasMethodVisibilityDefault()
+ ? llvm::Function::DefaultVisibility
+ : llvm::Function::HiddenVisibility);
CGM.SetLLVMFunctionAttributes(OMD, FI, Fn, /*IsThunk=*/false);
CGM.SetLLVMFunctionAttributesForDefinition(OMD, Fn);
} else {
diff --git a/clang/lib/CodeGen/CGObjCRuntime.cpp b/clang/lib/CodeGen/CGObjCRuntime.cpp
index 424564f9759995..1841c8ed23b5db 100644
--- a/clang/lib/CodeGen/CGObjCRuntime.cpp
+++ b/clang/lib/CodeGen/CGObjCRuntime.cpp
@@ -471,7 +471,8 @@ std::string CGObjCRuntime::getSymbolNameForMethod(const ObjCMethodDecl *OMD,
std::string buffer;
llvm::raw_string_ostream out(buffer);
CGM.getCXXABI().getMangleContext().mangleObjCMethodName(OMD, out,
- /*includePrefixByte=*/true,
+ /*includePrefixByte=*/
+ !OMD->hasMethodVisibilityDefault(),
includeCategoryName);
return buffer;
}
diff --git a/clang/test/CodeGenObjC/objc-direct-wrapper.m b/clang/test/CodeGenObjC/objc-direct-wrapper.m
new file mode 100644
index 00000000000000..2c67f252b41dfd
--- /dev/null
+++ b/clang/test/CodeGenObjC/objc-direct-wrapper.m
@@ -0,0 +1,86 @@
+// RUN: %clang -fobjc-arc -Wno-objc-root-class -ObjC -fobjc-runtime=ios -FFoundation \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=1 \
+// RUN: -target x86_64-apple-macosx10.15.0 -c -o - %s | \
+// RUN: llvm-nm - | FileCheck -check-prefix=CHECK-WRAPPER %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s | llvm-nm - | \
+// RUN: FileCheck -check-prefix=CHECK-DEFAULT %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=1 \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s -S -emit-llvm | \
+// RUN: FileCheck -check-prefix=CHECK-WRAPPER-IR-DEF %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class -DNO_OBJC_IMPL \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=1 \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s -S -emit-llvm | \
+// RUN: FileCheck -check-prefix=CHECK-WRAPPER-IR-DECLARE %s
+
+// RUN: not %clang -fobjc-arc -Wno-objc-root-class -DENABLE_PROTOCOL_DIRECT_FAIL \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=1 \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s -S -emit-llvm 2>&1 | \
+// RUN: FileCheck -check-prefix=CHECK-PROTOCOL-DIRECT-FAIL %s
+
+////////////////////////////////////////////////////////////////////////////////
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class -ObjC -fobjc-runtime=ios -FFoundation \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=0 \
+// RUN: -target x86_64-apple-macosx10.15.0 -c -o - %s | \
+// RUN: llvm-nm - | FileCheck -check-prefix=CHECK-WRAPPER-INDIRECT %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=0 \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s -S -emit-llvm | \
+// RUN: FileCheck -check-prefix=CHECK-WRAPPER-IR-DEF-INDIRECT %s
+
+// RUN: %clang -fobjc-arc -Wno-objc-root-class -DNO_OBJC_IMPL \
+// RUN: -DENABLE_VISIBLE_OBJC_DIRECT=0 \
+// RUN: -target x86_64-apple-macosx10.15.0 \
+// RUN: -ObjC -fobjc-runtime=ios -FFoundation -c -o - %s -S -emit-llvm | \
+// RUN: FileCheck -check-prefix=CHECK-WRAPPER-IR-DECLARE-INDIRECT %s
+
+// CHECK-WRAPPER: T _-<C testMethod:bar:>
+ // TODO: Fix this
+// CHECK-DEFAULT: t -[C testMethod:bar:]
+// CHECK-WRAPPER-IR-DEF: define {{(dso_local )?}}void @"-<C testMethod:bar:>"
+// CHECK-WRAPPER-IR-DECLARE: declare {{(dso_local )?}}void @"-<C testMethod:bar:>"
+// CHECK-PROTOCOL-DIRECT-FAIL: error: 'objc_direct' attribute cannot be applied to methods declared in an Objective-C protocol
+
+// CHECK-WRAPPER-INDIRECT-NOT: T _-<C testMethod:bar:>
+// CHECK-WRAPPER-IR-DEF-INDIRECT-NOT: define {{(dso_local )?}}void @"-<C testMethod:bar:>"
+// CHECK-WRAPPER-IR-DECLARE-INDIRECT-NOT: declare {{(dso_local )?}}void @"-<C testMethod:bar:>"
+
+#if ENABLE_VISIBLE_OBJC_DIRECT
+#define OBJC_DIRECT __attribute__((objc_direct)) __attribute__((visibility("default")))
+#else
+#define OBJC_DIRECT
+#endif
+
+@interface C
+- (void)testMethod:(int)arg1 bar:(float)arg2 OBJC_DIRECT;
+@end
+
+#ifndef NO_OBJC_IMPL
+@implementation C
+- (void)testMethod:(int)arg1 bar:(float)arg2 OBJC_DIRECT {
+}
+@end
+#endif
+
+#ifdef ENABLE_PROTOCOL_DIRECT_FAIL
+@protocol ProtoDirectVisibleFail
+- (void)protoMethod OBJC_DIRECT; // expected-error {{'objc_direct' attribute cannot be applied to methods declared in an Objective-C protocol}}
+@end
+#endif
+
+C *c;
+
+void f() {
+ [c testMethod:1 bar:1.0];
+}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
This patch adds the ability to expose direct ObjC methods as visible across link boundaries (dylibs). It is done so by using existing visibility attributes: ``` @interface C - (void)testMethod:(int)arg1 bar:(float)arg2 __attribute__((objc_direct)) __attribute__((visibility("default"))); @EnD ``` This is a work in progress and there are a few pieces that should be landed with this within an LLVM release cycle so as not to cause any inconsistency in the ABI: * First, we want some checks that inform the developer when they are using this feature as part of a framework that this is potentially brittle. Something similar to the following: llvm@1b3b69fbda70d * Next, we want to move the ObjC method nil checks from the callee side to something more useful for analysis and optimization that can be more visible from the caller side. Having the nil checks straight up in the caller side isn't so great for code size so a good alternative is to have a thunk between the call site and the callee for handling nil checks. * Finally, any mangling decisions should be finalized before this is landed. Much of this has been discussed ahead of time, and there is a more extensive document on this proposal at: https://docs.google.com/document/d/16CsNCA2OKWkUM_LCw_qabTcMtPiq9ODVkZHALlDvzR8
plotfi
force-pushed
the
plotfi-objc-direct-abi
branch
from
5e69de7
to
8c13976
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Posting this PR for posterity a bit earlier than I had intended because the old Phabricator is crashing (https://discourse.llvm.org/t/cant-access-https-reviews-llvm-org/75905):
This patch adds the ability to expose direct ObjC methods as visible across link boundaries (dylibs). It is done so by using existing visibility attributes:
This is a work in progress and there are a few pieces that should be landed with this within an LLVM release cycle so as not to cause any inconsistency in the ABI:
First, we want some checks that inform the developer when they are using this feature as part of a framework. This is so that they are aware that using this feature is potentially brittle. Something similar to the following: 1b3b69fbda70d
Next, we want to move the ObjC method nil checks from the callee side to something more useful for analysis and optimization that can be more visible from the caller side. Having the nil checks straight up in the caller side isn't so great for code size so a good alternative is to have a thunk between the call site and the callee for handling nil checks.
Finally, any mangling decisions should be finalized before this is landed.
Much of this has been discussed ahead of time, and there is a more extensive document on this proposal at:
https://docs.google.com/document/d/16CsNCA2OKWkUM_LCw_qabTcMtPiq9ODVkZHALlDvzR8