[embedded] Implicitly define __APPLE__ and __MACH__ when on -apple-none triples #71278
Conversation
kubamracek
requested review from
ktoso,
a team,
hyp and
egorzhdan
as code owners
|
@swift-ci please test |
lib/ClangImporter/ClangImporter.cpp
Outdated
| // a few things that the Apple SDKs expect. | ||
| if (triple.getVendor() == llvm::Triple::VendorType::Apple) { | ||
| invocationArgStrs.insert(invocationArgStrs.end(), | ||
| {"-D__APPLE__", "-D__MACH__"}); |
There was a problem hiding this comment.
mach should only be defined when the triple contains mach:
technically someone could write xxx-apple-none-elf (granted that would be rather strange...)
There was a problem hiding this comment.
+1 this should be conditional object file format == MachO
| @@ -631,6 +631,17 @@ importer::getNormalInvocationArguments( | |||
| }); | |||
| } | |||
|
|
|||
| // To support -apple-none triples, which are non-Darwin, we need to #define | |||
| // a few things that the Apple SDKs expect. | |||
| if (triple.getVendor() == llvm::Triple::VendorType::Apple) { | |||
There was a problem hiding this comment.
Shouldn't this be done by the driver and not the clang importer?
There was a problem hiding this comment.
Hm, that would be against the established pattern -- look at what the rest of this (pretty large) function is doing. It's setting up a ton of things, essentially "construct reasonable defaults for a Clang instance given the current Swift compilation", which my addition seems to match.
I'd say this addition shouldn't go to the driver, on the grounds that that would create an inconsistency. Maybe this whole function should go to the driver? (Even that I would probably argue against.)
There was a problem hiding this comment.
I'll defer to you, its just a bit odd to splat out the platform knowledge in a bunch of places, cc @artemcm for thoughts (not blocking)
There was a problem hiding this comment.
I believe that @rauhul is correct here. __APPLE__ is a vendor definition, and should always be set by clang for *-apple-* triples. I could see the argument for __MACH__ being the clang importer as technically it is the kernel identifier and not the object file format specifier, but it is used for the object file format as well. I think that sinking this into upstream clang is the right approach.
There was a problem hiding this comment.
Oh I misunderstood then. I thought that @rauhul was talking about the Swift driver and that it should receive this logic instead (and pass it as -Xcc flags down to the compiler).
|
@swift-ci please test |
lib/ClangImporter/ClangImporter.cpp
Outdated
| invocationArgStrs.insert(invocationArgStrs.end(), {"-D__MACH__"}); | ||
| } | ||
| if (triple.isOSBinFormatWasm()) { | ||
| invocationArgStrs.insert(invocationArgStrs.end(), {"-D__wasi__"}); |
There was a problem hiding this comment.
The macro here should be __wasm__. WASI is not related to the object file format but the environment (WASI uses WASM and provides a system interface). This would already be set by clang. This likely should be that the condition is isOSWASI
There was a problem hiding this comment.
Aha, so the reason why I seemingly needed to add -D__wasi__ is that we have SwiftShims code that conditionalizes based on it, and we fail to build the embedded stdlib for wasm-unknown-none-wasm without this -D__wasi__. The concrete piece of code is here: https://github.com/apple/swift/blob/main/stdlib/public/SwiftShims/swift/shims/Visibility.h#L137
Sounds like you're saying we should fix Visibility.h instead?
There was a problem hiding this comment.
Yeah, that definitely looks like a bug. That should be __wasm__.
|
@swift-ci please test |
|
@swift-ci please test |
Revert "Merge pull request #71278 from kubamracek/embedded-no-mach-defines"
This removes the need for the stdlib and other modules to -Xcc those defines manually, and also drops this requirement from users of the -apple-none triples (see the lit test changes).