Skip to content
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

Allow unit test targets to import and link executable targets #3316

Conversation

abertelrud
Copy link
Contributor

@abertelrud abertelrud commented Mar 1, 2021

Allow unit test targets to import and link executable targets. This allows them to test everything except the main function in those targets without having to run the executable as a subprocess. With these changes they can still run the subprocess if they want to.

Motivation:

Many package authors don’t want to split their code into executable targets vs library targets, but still want to test data structures and algorithms in their executables.

Modifications:

This uses a new Swift compiler flag introduced in swiftlang/swift#35595 to compile the main symbol to a different name than _main, so that it doesn't cause symbol collisions when linked into a test executable. When linking the actual executable, the entry point symbol is then renamed back to to _main. This is done using linker flags for both Darwin and Linux in this implementation, but for linkers that don't support such options, a fallback would be to generate a stub source file along the lines of the compiler PR.

This feature is guarded with a tools version check, since packages written this way won't be testable on older toolchains.

This PR supersedes the one at #3103, which modified object files after compilation. That was a much messier solution but the best possible without support from either the compiler or linker. Now that there is compiler support, we can do better.

Modifications:

  • when compiling the main module of an executable, use a Swift flag to set the name of the entry point to a name that's based on the module name
  • when linking an executable, use a linker flag to rename the renamed entry point back to _main
  • undo previous changes to prevent the modules of executables from being found by the Swift compiler
  • added PackageGraph lookup methods for mapping targets and products back to packages (for the tools version)
  • pipe through the tools version in BuildPlan so that logic that generates flags can take it into account

Result:

Unit test targets can now link executable targets that are implemented in Swift. Executables implemented using Clang targets would require a corresponding Clang flag, and are therefore currently not supported.

rdar://58122395

@abertelrud abertelrud marked this pull request as draft March 1, 2021 10:29
@abertelrud
Copy link
Contributor Author

@swift-ci please smoke test

@@ -499,8 +499,7 @@ public final class SwiftTargetBuildDescription {

/// The path to the swiftmodule file after compilation.
var moduleOutputPath: AbsolutePath {
let dirPath = (target.type == .executable) ? tempsPath : buildParameters.buildPath
return dirPath.appending(component: target.c99name + ".swiftmodule")
return buildParameters.buildPath.appending(component: target.c99name + ".swiftmodule")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This restores the code from before a previous change to make it more obvious that tests couldn't import executable modules, by moving those modules into subdirectories of the main build directory.

Copy link
Contributor Author

@abertelrud abertelrud Mar 1, 2021

Choose a reason for hiding this comment

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

Because Clang targets don't produce a module but instead use a module map in the public headers directory, the more obvious error is preserved when trying to import a Clang executable module from a test target. It is not preserved when importing an executable module from another one, however. We should consider moving the module back into its subdirectory and instead adding additional search paths to test targets that declare a dependency on the executable target, to preserve the good error message if an executable tries to import another executable's module.

This would be good to do in general, so that a dependency between two targets needs to be declared before the module can be imported (otherwise it leads to hard-to-diagnose linker errors rather than a more obvious cannot-import-module error).

@abertelrud abertelrud force-pushed the allow-test-products-to-link-against-executables-take-two branch from 798d7d9 to 668f8d1 Compare March 1, 2021 10:36
@abertelrud
Copy link
Contributor Author

@swift-ci please smoke test

@abertelrud abertelrud force-pushed the allow-test-products-to-link-against-executables-take-two branch from 668f8d1 to 685eb8d Compare March 1, 2021 10:46
@abertelrud
Copy link
Contributor Author

@swift-ci please smoke test

@tomerd
Copy link
Contributor

tomerd commented Mar 1, 2021

This should possibly be guarded by a tools version check, since packages written this way won't be testable on older toolchains.

makes sense

I also need to check whether this will work with the Windows linker.

cc @compnerd

@abertelrud
Copy link
Contributor Author

Some of the macOS failures are because of unit tests that need to be updated, but there's a real issue with how gold is getting invoked that I'll need to look into.

@abertelrud
Copy link
Contributor Author

So with the nightly toolchain the general principle seems to work:

❯ echo 'print("hello main")' > foo.swift
❯ swiftc -c -Xfrontend -entry-point-function-name -Xfrontend foo foo.swift -o foo.o
❯ nm foo.o
0000000000000000 T foo
0000000000000000 r l_entry_point
0000000000000000 r .L__unnamed_1
000000000000000d r .L__unnamed_2
000000000000000b r .L__unnamed_3
                 U $ss27_allocateUninitializedArrayySayxG_BptBwlFyp_Tg5
00000000000000c0 W $ss27_finalizeUninitializedArrayySayxGABnlF
                 U $ss5print_9separator10terminatoryypd_S2StF
0000000000000100 W $ss5print_9separator10terminatoryypd_S2StFfA0_
0000000000000120 W $ss5print_9separator10terminatoryypd_S2StFfA1_
0000000000000140 W $sSa12_endMutationyyF
                 U $sSaMa
                 U $sSS21_builtinStringLiteral17utf8CodeUnitCount7isASCIISSBp_BwBi1_tcfC
                 U $sSSN
                 U swift_bridgeObjectRelease
0000000000000000 V __swift_reflection_version
                 U swift_release
                 U $sypN
❯ swiftc foo.o -o foo
/usr/lib/gcc/x86_64-linux-gnu/5.4.0/../../../x86_64-linux-gnu/Scrt1.o:function _start: error: undefined reference to 'main'
clang-10: error: linker command failed with exit code 1 (use -v to see invocation)
<unknown>:0: error: link command failed with exit code 1 (use -v to see invocation)
❯ swiftc foo.o -Xlinker --defsym -Xlinker main=foo -o foo
❯ ./foo
hello main

@abertelrud
Copy link
Contributor Author

I also need to check whether this will work with the Windows linker.

cc @compnerd

AFAIK, the Windows toolchain currently uses lld — is that right? If so, then --defsym should be supported. Worst case, we can always do the generated source file suggested in swiftlang/swift#35595, but a linker flag seems a little cleaner.

@abertelrud
Copy link
Contributor Author

Interestingly, from the command line using swift-DEVELOPMENT-SNAPSHOT-2021-02-24-a-ubuntu16.04 it actually works. I wonder if the module wrapping has anything to do with this.

@abertelrud
Copy link
Contributor Author

Looks like this happens when linking the test suites themselves as executables. That's the case in which we don't want to rename back to main, since the test stub's main is the one symbol with that name that we want to leave.

@abertelrud abertelrud force-pushed the allow-test-products-to-link-against-executables-take-two branch from 685eb8d to 5ef1e9e Compare March 2, 2021 02:11
@abertelrud
Copy link
Contributor Author

@swift-ci please smoke test

@abertelrud abertelrud force-pushed the allow-test-products-to-link-against-executables-take-two branch from 5ef1e9e to 8fe92a5 Compare March 2, 2021 02:51
@abertelrud
Copy link
Contributor Author

@swift-ci please smoke test

@abertelrud abertelrud force-pushed the allow-test-products-to-link-against-executables-take-two branch from 8fe92a5 to c16815f Compare March 2, 2021 03:06
@abertelrud
Copy link
Contributor Author

@swift-ci please smoke test

@abertelrud abertelrud force-pushed the allow-test-products-to-link-against-executables-take-two branch from c16815f to 85db1a0 Compare March 2, 2021 06:00
@abertelrud
Copy link
Contributor Author

@swift-ci please smoke test

@abertelrud abertelrud force-pushed the allow-test-products-to-link-against-executables-take-two branch from 85db1a0 to 787dc2e Compare March 2, 2021 06:10
@abertelrud
Copy link
Contributor Author

@swift-ci please smoke test

@abertelrud
Copy link
Contributor Author

The self-hosed macOS platform test is failing because:

swift --version
+ swift --version
Apple Swift version 5.3.2 (swiftlang-1200.0.45 clang-1200.0.32.28)
Target: x86_64-apple-darwin19.6.0

This should be addressed by tying this to the package tools version.

@abertelrud
Copy link
Contributor Author

This should be addressed by tying this to the package tools version.

Actually, that won't help — we're testing the mainline SwiftPM (with a 999.0 tools version) but using a 5.3 Swift compiler version. So we would either need detect that the compiler we're using doesn't support it, or update the self-hosted macOS tools to one that has the mainline compiler. This won't be so interesting in practice since SwiftPM ships as part of the toolchain, so it will always be paired with a recent enough compiler. So this is mostly a testing problem.

@abertelrud abertelrud force-pushed the allow-test-products-to-link-against-executables-take-two branch from 787dc2e to 2ed483e Compare March 2, 2021 23:55
@abertelrud
Copy link
Contributor Author

@swift-ci please smoke test

@tomerd
Copy link
Contributor

tomerd commented Mar 3, 2021

looking good! can't wait to start using this

@abertelrud abertelrud force-pushed the allow-test-products-to-link-against-executables-take-two branch from 96c5014 to d43fd9e Compare March 3, 2021 10:08
@abertelrud
Copy link
Contributor Author

@swift-ci please smoke test

@abertelrud abertelrud force-pushed the allow-test-products-to-link-against-executables-take-two branch from d43fd9e to a578ab1 Compare March 3, 2021 10:25
@abertelrud
Copy link
Contributor Author

@swift-ci please smoke test

@abertelrud abertelrud self-assigned this Mar 3, 2021
@abertelrud abertelrud added the ready Author believes the PR is ready to be merged & any feedback has been addressed label Mar 3, 2021
@abertelrud abertelrud marked this pull request as ready for review March 3, 2021 18:32
@abertelrud abertelrud requested a review from compnerd March 3, 2021 20:16
@abertelrud
Copy link
Contributor Author

Hi @compnerd, do you know if there are any Windows linker options that could be used here for renaming <module>_main back to main, or would we need to resort to generating a source file?

@compnerd
Copy link
Member

compnerd commented Mar 3, 2021

I don't think that there is a good way to do a rename on a symbol on Windows. I cannot think of any option in link that would let you do that. You would need to generate a new source file to accomplish it, either way a thunk or some less than ideal horrible things to do the aliasing optionally as a replacement for main.

@abertelrud
Copy link
Contributor Author

I don't think that there is a good way to do a rename on a symbol on Windows. I cannot think of any option in link that would let you do that. You would need to generate a new source file to accomplish it, either way a thunk or some less than ideal horrible things to do the aliasing optionally as a replacement for main.

Thanks, @compnerd. Would /ALTERNATENAME:_main=_Foo_main work? That would be for LINK.EXE — if lld is used (I don't remember whether it is and don't have access to Windows), then --defsym should work.

Otherwise the generated source file would always be a last resort.

@compnerd
Copy link
Member

compnerd commented Mar 5, 2021

The /alternatename is an undocumented flag that could work (and is what I meant by the horrible things). I don't know if that is permitted on the command line on all linkers (i.e. we need to test with both link and lld). You would need to embed that into an object file and that is why the generated source file is what came to mind. The behaviour of it is that if one symbol is not provided, this will be used as a fallback. Then the problem becomes that if you have multiple references, it could be a problem.

if lld is used (I don't remember whether it is and don't have access to Windows), then --defsym should work.

lld is meant to be 100% identical to link in invocation (and is how we use it in Swift as well). The --defsym= option might work under the MinGW mode, but that has semantic differences and would break the Windows targets in subtle ways.

For Windows, assume that the only valid way to test is link - lld should be a drop in replacement for link.

…ription to know the tools version of the package in which they are defined. This allows compiler flags and other semantically significant changes to be conditionalized on the tools version.

In the cases where tools versions are synthesized, they get the tools version of the package defining the product or target for which they are being synthesized.  The fallback for anything that cannot be determined at all is always `.vNext`, which is the same as has been the case until now.
…hat are implemented in Swift. This uses a new Swift compiler flag to set the name of the entry point when emitting object code, and then uses linker flags to rename the main executable module's entry point back to `_main` again when actually linking the executable.

This is guarded by a tools version check, since packages written this way won't be testable on older toolchains.

Also, this is currently only done on Darwin and Linux.  A supplemental PR will generate a small stub containing code that implements `main` to call the per-module `<module>_main` function, which will be linked into the executable.
@abertelrud abertelrud force-pushed the allow-test-products-to-link-against-executables-take-two branch from a578ab1 to 05babe9 Compare March 6, 2021 17:43
@abertelrud
Copy link
Contributor Author

@swift-ci please smoke test

@abertelrud
Copy link
Contributor Author

abertelrud commented Mar 6, 2021

The /alternatename is an undocumented flag that could work (and is what I meant by the horrible things). I don't know if that is permitted on the command line on all linkers (i.e. we need to test with both link and lld). You would need to embed that into an object file and that is why the generated source file is what came to mind. The behaviour of it is that if one symbol is not provided, this will be used as a fallback. Then the problem becomes that if you have multiple references, it could be a problem.

if lld is used (I don't remember whether it is and don't have access to Windows), then --defsym should work.

lld is meant to be 100% identical to link in invocation (and is how we use it in Swift as well). The --defsym= option might work under the MinGW mode, but that has semantic differences and would break the Windows targets in subtle ways.

For Windows, assume that the only valid way to test is link - lld should be a drop in replacement for link.

Thanks, @compnerd. For now I have updated the PR to only do this renaming on Darwin and Linux, but I will put up a supplemental PR to generate a source file to use as fallback. I tried an attempt at doing so in this PR, but the approach suggested in the compiler PR using a Swift source file didn't work. I will add a facility to compile and link C stub source files when building executables, which might be handy in any case, and then we can use that to compile and link in this:

extern int _foo_main(int, char **);
int main(int argc, char **argv) {
   return _foo_main(argc, argv);
}

when producing the executable, which will work on any platform. The stub was the original plan and the linker flags were only an optimization to avoid this extra code generation, but the stub will work for any platform without undocumented features or magic tricks.

@abertelrud abertelrud merged commit 037a616 into swiftlang:main Mar 7, 2021
@abertelrud abertelrud deleted the allow-test-products-to-link-against-executables-take-two branch March 7, 2021 11:11
// XCTAssertEqual(String(cString: GetGreeting3()), "Hello, universe")

// Some of the APIs that we use below are available in macOS 10.13 and above.
guard #available(macOS 10.13, *) else {
Copy link
Contributor

Choose a reason for hiding this comment

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

@abertelrud does this stuff really not work on any platform but macOS?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready Author believes the PR is ready to be merged & any feedback has been addressed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants