swiftlang / swift

The Swift Programming Language
https://swift.org
Apache License 2.0
67.65k stars 10.38k forks source link

🟠 OSS Swift CI: oss-swift-incremental-ASAN-RA-macos failed: test: Frontend/load-pass-plugin.swift (exit code 1) #77771

Open al45tair opened 4 days ago

al45tair commented 4 days ago

Description

68985 introduced a way to load an LLVM pass from a dynamic library into the compiler. Unfortunately the test case for this is subtly broken as it links against the (static) library libLLVMSupport.a, which is already linked into the compiler binary, and so we end up with ODR violations, which are picked up by ASAN; in particular, the ASAN build is complaining about llvm::circular_raw_ostream.

Reproduction

https://ci.swift.org/job/oss-swift-incremental-ASAN-RA-macos/7303/consoleFull#1161724776d6fdb6cb-f376-4f2e-8bce-d31c7304698b

Expected behavior

The test case should pass.

Environment

Swift OSS CI (ASAN build)

Additional information

rdar://140319292

al45tair commented 4 days ago

I think the fix for this is probably to remove -lLLVMSupport from the command line for the libTestPlugin.dylib build and add instead -Wl,-bundle_loader -Wl,%swift_frontend_plain. Something like that, at any rate.

al45tair commented 4 days ago

@antoniofrighetto Would you be able to take a look at this? // @aschwaighofer

al45tair commented 4 days ago

(I've disabled the test when doing the ASAN build for now, but we should fix this and re-enable it.)

antoniofrighetto commented 4 days ago

@al45tair Thank you for the temporary fix! I agree we should have not been linking against libLLVMSupport.a in the plugin in the first place, as the Swift compiler already links against it. I think resolving the undefined symbols at runtime using the symbols already available in the Swift frontend binary looks definitely better. Let me know if you'd like me to test it and open a PR with the fix.

al45tair commented 4 days ago

Let me know if you'd like me to test it and open a PR with the fix.

Yes please. There's no rush — this is new functionality and it's the test case itself that's slightly wrong (while I wouldn't want people to go building passes and making the same mistake because they copied the commands from the test case, I doubt that's really a problem in practice as I imagine most passes added this way are things people are experimenting with).