Skip to content

Conversation

@MaxDesiatov
Copy link
Contributor

Currently, clang doesn't support Objective-C with the WebAssembly object format. Because of this, sil-function-extractor tool crashes in Clang's CGObjCCommonMac::GetSectionName function. Since Swift doesn't provide Objective-C interop for WebAssembly, it would make sense to disable it explicitly. sil-function-extractor is used in the test suite, so this PR is required for those tests to proceed when testing the WebAssembly toolchain and SDK.

Related to SR-9307.

(cc @compnerd)

@compnerd
Copy link
Member

This really should mirror the behaviour from the driver -enable-objc-interop is a flag and is default enabled on Apple platforms and disabled on others. I think that sil-function-extractor could grow the same appendages.

@CodaFi
Copy link
Contributor

CodaFi commented May 22, 2020

Right. In general, Target.isOSDarwin(); is the predicate we want here.

@MaxDesiatov
Copy link
Contributor Author

Would you prefer sil-function-extractor to have the same -enable-objc-interop flag, or should it just check Target.isOSDarwin() internally and flip EnableObjCInterop based on that?

@compnerd
Copy link
Member

Both actually, it should have -enable-objc-interop to control the behaviour, but the default should be the value of Target.isOSDarwin().

@MaxDesiatov MaxDesiatov changed the base branch from master to main September 24, 2020 08:42
@MaxDesiatov
Copy link
Contributor Author

Thanks for the review, that's addressed now.

@MaxDesiatov MaxDesiatov requested review from CodaFi and compnerd August 8, 2021 14:45
@MaxDesiatov
Copy link
Contributor Author

@swift-ci please smoke test

Copy link
Member

Choose a reason for hiding this comment

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

EnableObjCInterop should take precedent over Target.isOSDarwin(). That is, I should be able to disable ObjC interop even on Darwin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any way to check if EnableObjCInterop was defined or not as a command-line argument? I had a look in LLVM docs, and looks like the only options for bool CL arguments are true or false (with false as the default) with no way to determine if it was explicitly passed or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, llvm::cl::boolOrDefault does the job.

@MaxDesiatov
Copy link
Contributor Author

@swift-ci please smoke test

@MaxDesiatov MaxDesiatov requested a review from compnerd August 8, 2021 19:53
Copy link
Contributor

@CodaFi CodaFi left a comment

Choose a reason for hiding this comment

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

This looks pretty good. But can we match sil-opt where you can explicitly disable as well. https://github.com/apple/swift/blob/d5e5253cee0599a5362363d6ba0fe640493ea0e6/tools/sil-opt/SILOpt.cpp#L419

@MaxDesiatov
Copy link
Contributor Author

@CodaFi were you looking at the old diff by any chance? With 2a220e1 that's exactly how it works, if I understand correctly. Or do you want to have a separate -disable-objc-interop option? That would be equivalent to -enable-objc-interop=false, which is already possible here. Would we want to have more than one way to explicitly disable it?

@MaxDesiatov
Copy link
Contributor Author

@swift-ci please smoke test

@MaxDesiatov
Copy link
Contributor Author

@swift-ci please smoke test Linux platform

@MaxDesiatov MaxDesiatov merged commit 69bdaad into swiftlang:main Aug 9, 2021
@MaxDesiatov MaxDesiatov deleted the sil-extractor-wasm branch August 9, 2021 11:57
MaxDesiatov added a commit to swiftwasm/swift that referenced this pull request Sep 29, 2021
This check is no longer needed since Wasm handling was addressed upstream in swiftlang#31920
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants