Build plugin executables with debug symbols

[neonichu]

This seems like the better default until we might add a dedicated debugging feature for plugins.

[neonichu]

@swift-ci please smoke test

[neonichu]

testCompilationDiagnostics.leDwzd/MyPackage/Plugins/MyPlugin/plugin.swift:8:5: error: missing return in a function expected to return '[Command]'
    }
    ^

🤔

[abertelrud]

testCompilationDiagnostics.leDwzd/MyPackage/Plugins/MyPlugin/plugin.swift:8:5: error: missing return in a function expected to return '[Command]'
    }
    ^

🤔

That's actually the expected compilation error for which this unit test is testing diagnostics. It's unfortunate that this gets printed out where the automation is picking it up as an actual error.

Rather the test failures seem to be:

/Users/ec2-user/jenkins/workspace/swift-package-manager-PR-macos-smoke-test/branch-main/swiftpm/Tests/SPMBuildCoreTests/PluginInvocationTests.swift:379: error: -[SPMBuildCoreTests.PluginInvocationTests testCompilationDiagnostics] : XCTAssertEqual failed: ("0") is not equal to ("1")
/Users/ec2-user/jenkins/workspace/swift-package-manager-PR-macos-smoke-test/branch-main/swiftpm/Tests/SPMBuildCoreTests/PluginInvocationTests.swift:380: error: -[SPMBuildCoreTests.PluginInvocationTests testCompilationDiagnostics] : XCTUnwrap failed: expected non-nil value of type "Diagnostic"

[abertelrud]

This unit test basically checks that a plugin with an error produces the expected diagnostics, and then checks that those errors go away after a corrected plugin source file is written and the plugin is compiled again.

[abertelrud]

Ok, I can repro this locally and this is really weird. The backstory is that this unit test checks a couple of plugin compilation diagnostics, including first that there is a compilation failure with a syntactically incorrect plugin source file, and then that when it's fixed in a way that leaves an unused variable, it checks that there is still a warning.

Here's the weird part: when compiling without -g, the warning gets emitted as text to stderr (warning: variable 'unused' was never used; consider replacing with '_' or removing it) and this warning also appears in the .dia diagnostics file. This is expected.

But when compiling with -g, the warning still gets emitted as text to stderr (expected) but the .dia no longer contains the warning. I verified that it isn't just our parsing that's wrong — the file size of the .dia really is 220, which is the size of an empty Swift diagnostics file. So this looks to me like a compiler and/or driver bug — if the warning is printed I would not expect the .dia to be empty just because we're now passing -g. @artemcm have you seen anything like this?

@neonichu I don't think the right thing is to dumb down the test here, because if the .dia file doesn't contain the warning when compiling with debugging enabled, then those warnings won't appear in Xcode either.

[artemcm]

But when compiling with -g, the warning still gets emitted as text to stderr (expected) but the .dia no longer contains the warning. I verified that it isn't just our parsing that's wrong — the file size of the .dia really is 220, which is the size of an empty Swift diagnostics file. So this looks to me like a compiler and/or driver bug — if the warning is printed I would not expect the .dia to be empty just because we're now passing -g. @artemcm have you seen anything like this?

I've not seen this before, it sounds likely to be a compiler issue where -g is somehow affecting serialized diagnostic emission when it really should not be.

[neonichu]

Yah, I agree that we don't want to change the test -- that said I think we do want to get this into 5.7. I am going to open a 5.7 PR that modifies the test for now but leave this one open for main.

[neonichu]

Interesting, this might be a recent regression in the compiler? Initially, I wasn't able to reproduce it with my installed Xcode and using swift-5.7-DEVELOPMENT-SNAPSHOT-2022-07-05-a, the issue also does not reproduce. If I use swift-DEVELOPMENT-SNAPSHOT-2022-07-12-a, the issue does seem to reproduce.

[neonichu]

Actually, scratch that, the issue I am seeing locally with the main toolchain is different (an additional warning add '@preconcurrency' to suppress 'Sendable'-related warnings from module 'Foundation'). So I am somehow unable to repro locally.

[neonichu]

@swift-ci please smoke test

[neonichu]

@swift-ci please smoke test

[neonichu]

OK, looks like we're clearly using the inferior compiler in smoke tests (e.g. /Users/ec2-user/jenkins/workspace/swift-package-manager-PR-macos-smoke-test/branch-main/build/buildbot_incremental/swiftpm-macosx-x86_64/x86_64-apple-macosx/release/swiftc), but the tests still fail. The test also fails with the installed 5.6 compiler on macOS, but not on Linux 🤔

[neonichu]

@swift-ci please smoke test

[neonichu]

Can't repro with the 5.6.1 toolchain locally, either. Also using swift test now to get closer to what is happening on CI.

[neonichu]

@swift-ci please smoke test

[neonichu]

@swift-ci please smoke test

[neonichu]

Added some more debug info and it definitely seems like the serialized diagnostics are simply missing the warning, but it is being emitted to the console. Don't see how this could be environmental, but maybe some quirk of the debug toolchain?

[neonichu]

Interesting, I can repro the issue locally with Swift 5.5.2 / Xcode 13C100.

[neonichu]

If I switch from the frontend flag to the driver flag -serialize-diagnostics, I also get empty diagnostics files earlier in the test and locally with the current 5.7 compiler 🤔

[neonichu]

I think I found the issue, if I pass -disallow-use-new-driver I can reproduce the issue locally exactly, without further changes. Swift 5.5 also uses the old driver which also matches this observation. I'm now assuming that we are using the old driver in the CI toolchain.

[neonichu]

We should figure out how to enable the new driver in CI so that testing is closer to production, but that shouldn't block this PR. I introduced a new testing helper supportsSerializedDiagnostics now which tests if the bug is present in the current toolchain and if it is, we skip the parts of the tests that are broken due to that.

[neonichu]

@swift-ci please smoke test