Skip to content

5.9 Windows macros support changes #68334

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

Merged
merged 50 commits into from
Sep 14, 2023
Merged

Conversation

compnerd
Copy link
Member

@compnerd compnerd commented Sep 5, 2023

This backports the changes to support macros on Windows. The bulk of the changes are to the tests, the C++ and runtime pieces fix mismatches between operator new/malloc and free and implement Windows process execution support.

@compnerd compnerd requested a review from a team as a code owner September 5, 2023 22:47
@compnerd
Copy link
Member Author

compnerd commented Sep 5, 2023

@swift-ci please test

@compnerd
Copy link
Member Author

compnerd commented Sep 5, 2023

@compnerd
Copy link
Member Author

compnerd commented Sep 5, 2023

Please test with following PRs:
swiftlang/llvm-project#7399

@swift-ci please test

@compnerd
Copy link
Member Author

compnerd commented Sep 6, 2023

@swift-ci please test

1 similar comment
@compnerd
Copy link
Member Author

compnerd commented Sep 6, 2023

@swift-ci please test

@compnerd
Copy link
Member Author

compnerd commented Sep 6, 2023

Please test with following PRs:
swiftlang/llvm-project#7399

@swift-ci please test

1 similar comment
@compnerd
Copy link
Member Author

compnerd commented Sep 6, 2023

Please test with following PRs:
swiftlang/llvm-project#7399

@swift-ci please test

@compnerd
Copy link
Member Author

compnerd commented Sep 6, 2023

@swift-ci please test Linux platform

@compnerd
Copy link
Member Author

compnerd commented Sep 6, 2023

Please test with following PRs:
swiftlang/llvm-project#7399

@swift-ci please test Linux platform

@compnerd
Copy link
Member Author

compnerd commented Sep 6, 2023

Please test with following PRs:
swiftlang/llvm-project#7399

@swift-ci please test

@compnerd
Copy link
Member Author

compnerd commented Sep 6, 2023

Please test with following PRs:
swiftlang/llvm-project#7399

@swift-ci please test macOS platform

3 similar comments
@compnerd
Copy link
Member Author

compnerd commented Sep 6, 2023

Please test with following PRs:
swiftlang/llvm-project#7399

@swift-ci please test macOS platform

@compnerd
Copy link
Member Author

compnerd commented Sep 6, 2023

Please test with following PRs:
swiftlang/llvm-project#7399

@swift-ci please test macOS platform

@compnerd
Copy link
Member Author

compnerd commented Sep 6, 2023

Please test with following PRs:
swiftlang/llvm-project#7399

@swift-ci please test macOS platform

compnerd and others added 15 commits September 6, 2023 20:40
`:` is not a valid file system character but is used to namespace the
imported targets.  This is then used to create the stamp file.  Sanitize
the name prior to use as a stamp file name.
`CMAKE_SHARED_LIBRARY_PREFIX` is empty on Windows.  This accounts for
that and quotes the parameter which we otherwise will see as an error
due to insufficient parameters being passed.
This begins reworking the API to be less POSIX centric and more
generally usable.  The API was defined in terms of `dlopen`/`dlsym`
rather than the better suited `llvm::sys::DynamicLibrary` APIs which
would avoid most of the work that needs to be done here for platform
specifics.
Include `stdint.h` for non-MSVC compilers.
We cannot depend on Swift's unsafe pointers using `malloc` under the
hood, so don't use `free` on them. Instead, expose entry points from
Swift to C++ to free any Swift-allocated data structures that need to
be deallocated from C++.
Buyer beware: the `MallocAllocator` does not, in fact, use `malloc`, so
one cannot pair an allocation using `MallocAllocator` with an actual
`free`.
The C type tha corresponds to Swift's pointer-sized `Int` and `UInt`
types varies from one platform to the next. The canonical C types are
`ptrdiff_t` and `size_t`, but we're in the depths of the compiler we
can't include the C library headers that provide them because they
introduce cyclic module dependencies. Sigh.

SwiftShims has some logic to compute these types. However, SwiftShims
is part of the Swift runtime, not the compiler, so those headers
cannot be included here. So, we clone the logic and simplify it
somewhat for our use case.

This fixes truncation issues on Windows, where the uses of
`unsigned long` and `long` for Swift(U)Int are incorrect.
…t/UInt

64-bit Windows defines both _WIN64 and _WIN32, so the logic here would
always end up defining 32-bit C types for Swift's `Int` and `UInt`.
Fix the ordering to check for 64-bit first, then 32-bit second.

Note that the SwiftShims version of this code has always been wrong,
but it's completely benign because SwiftShims is only used in the
Swift runtime itself, which is built with Clang (on all platforms),
and doesn't need to go through this code path. Still, we fix it in both
places, so we don't get a nasty surprise if the SwiftShims version of
the header later gets included in a non-Clang C++ compiler.
Co-authored-by: Saleem Abdulrasool <[email protected]>
Co-authored-by: Saleem Abdulrasool <[email protected]>
The type definitions do not uniformly import as `Swift.Int` and `Swift.UInt`.  Add some casts to accommodate the type conversions.
compnerd and others added 15 commits September 6, 2023 20:40
The plugin layouts are different across platforms.  Move this into a
virtual method and allow replacement.  On Windows, the plugins are
placed into the `bin` directory as the DLLs should always be co-located
to ensure that the proper DLLs are found (there is no concept of RPATH).
A thrown error is stored as `any Error` which does not conform to
`CustomStringConvertible`.  When we perform the conversion via the
`String(describing:)` initialiser, for some reason the runtime does not
find the conformance and the error is not translated properly.
Explicitly casting to `PluginError` resolves the missing conformance and
fixes a test failure.
Account for import libraries and the associated layout difference on
platforms (e.g. DLLs are placed in `bin`).  This is required to enable
building the macro path on Windows.
This wires up the new macro properly into the build machinery to ensure
that the `distribution` target properly builds and installs the
dependencies.  This fixes the missing `swift-plugin-server` on Windows.
Add missing include for `atexit`.
@compnerd
Copy link
Member Author

compnerd commented Sep 7, 2023

Please test with following PRs:
swiftlang/llvm-project#7404
swiftlang/swift-syntax#2161
swiftlang/swift-driver#1435

@swift-ci please test

Copy link
Member

@DougGregor DougGregor 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 great. Lots of changes, but we've been vetting these on main already, and they are mainly focused in portability (so low risk) or are limited to relatively new code for the Windows build. Thank you!

This fixes the test suite when performing a clean build.  The libraries
were being copied to the wrong location after the generalisation changes
made earlier.
@compnerd
Copy link
Member Author

Please test with following PRs:
swiftlang/llvm-project#7404
swiftlang/swift-syntax#2161
swiftlang/swift-driver#1435

@swift-ci please test

Comment on lines +19 to +20
// CHECK-SYSV:-plugin-path {{.*}}plugins
// CHECK-SYSV:-plugin-path {{.*}}plugins
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no corresponding FileCheck for this now. Doesn't seem like much point for Linux to be checking local either IMO, but 🤷. Windows would still have one -plugin-path IIUC.

Redefine the types rather than use the standard headers due to the
circular dependency between Darwin and Swift.
@compnerd
Copy link
Member Author

Please test with following PRs:
swiftlang/llvm-project#7404
swiftlang/swift-syntax#2161
swiftlang/swift-driver#1435

@swift-ci please test

@compnerd compnerd merged commit 33917a3 into swiftlang:release/5.9 Sep 14, 2023
@compnerd compnerd deleted the 5.9-macros branch September 14, 2023 16:59
bnbarham pushed a commit to bnbarham/swift that referenced this pull request Sep 29, 2023
bnbarham pushed a commit to bnbarham/swift that referenced this pull request Sep 29, 2023
bnbarham pushed a commit to bnbarham/swift that referenced this pull request Oct 3, 2023
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