[lit] Use CMAKE_C_FLAGS and CMAKE_EXE_LINKER_FLAGS when compiling test plugin #70136

drodriguez · 2023-11-30T22:33:45Z

In cases where the swift-plugin-server is built with specialized CMAKE_C_FLAGS and/or CMAKE_EXE_LINKER_FLAGS, if one wants to load a plugin in it, one needs to build with a compatible set of flags, or the plugin will not be able to be loaded.

Most of the time the tests can build against the / sysroot and be fine, but in the case of these test plugins, since they have to be loaded into the swift-plugin-server process, they need to compiled and linked in a compatible way. Using CMAKE_C_FLAGS and CMAKE_EXE_LINKER_FLAGS uses the same set of flags in both cases, and allow loading these test plugins during testing and avoid failures.

The change in lit.site.cfg.in embeds the values of CMAKE_C_FLAGS and CMAKE_EXE_LINKER_FLAGS from CMake into the Lit configuration. The usage of raw triple quoted strings allow single and double quoted arguments to survive the interpolation.

The change in lit.local.cfg creates two substitutions for them, and uses those substitutions in swift-build-cxx-plugin.

…t plugin In cases where the swift-plugin-server is built with specialized `CMAKE_C_FLAGS` and/or `CMAKE_EXE_LINKER_FLAGS`, if one wants to load a plugin in it, one needs to build with a compatible set of flags, or the plugin will not be able to be loaded. Most of the time the tests can build against the `/` sysroot and be fine, but in the case of these test plugins, since they have to be loaded into the swift-plugin-server process, they need to compiled and linked in a compatible way. Using `CMAKE_C_FLAGS` and `CMAKE_EXE_LINKER_FLAGS` uses the same set of flags in both cases, and allow loading these test plugins during testing and avoid failures. The change in `lit.site.cfg.in` embeds the values of `CMAKE_C_FLAGS` and `CMAKE_EXE_LINKER_FLAGS` from CMake into the Lit configuration. The usage of raw triple quoted strings allow single and double quoted arguments to survive the interpolation. The change in `lit.local.cfg` creates two substitutions for them, and uses those substitutions in `swift-build-cxx-plugin`.

drodriguez · 2023-11-30T22:33:55Z

@swift-ci please test

drodriguez · 2023-11-30T22:34:32Z

@compnerd I don't know if you want a similar change in the Windows branch. It seems to work for the Windows builds, so not sure if they need this extra flexibility.

compnerd · 2023-11-30T22:35:34Z

Do you know specifically what flags are causing issues here?

drodriguez · 2023-11-30T23:14:03Z

Do you know specifically what flags are causing issues here?

In our case, we compile swift-plugin-server against an specific glibc version (2.34) of a different sysroot. When we compile these test plugins, instead of that specific glibc version, the compiler uses the one in /usr/lib (in our case it is 2.28) because no flag tells it otherwise. When the test executes, swift-plugin-server loads the the 2.34 version, and when the plugin is loaded, the symbols referenced by the test plugin (the ones that 2.28 provides) are nowhere to be found, so the plugin does not load and the test fails.

compnerd · 2023-11-30T23:17:21Z

Ah, yes, that is a good point - we should match the host configuration for the tests. We get away with it on Windows because we don't have cross-compilation against different versions atm.

bnbarham

Makes sense 👍 Thanks @drodriguez

bnbarham · 2023-11-30T23:39:40Z

test/Macros/lit.local.cfg

@@ -25,6 +25,8 @@ else:
        0,
        (
            '%swift-build-cxx-plugin',


Makes me think swift-build-cxx-plugin is the wrong name, since it's not C++ 😅.

If you want that change, I would prefer doing it in a different PR. It involves changing a lot of "callsites".

Yeah that's fine, I just noticed because you added C_FLAGS and I was about to comment with "CXX_FLAGS?".

As pointed out in swiftlang#70136 (comment) the substitution name does not make sense because we are compiling C code, not C++. This should not introduce any behavioural changes.

…0188) As pointed out in #70136 (comment) the substitution name does not make sense because we are compiling C code, not C++. This should not introduce any behavioural changes.

rintaro · 2023-12-04T18:46:56Z

@drodriguez
Some plugin tests started failing in ASAN bot https://ci.swift.org/job/oss-swift-incremental-ASAN-RA-macos/4553/
Could this be the cause?

drodriguez · 2023-12-04T18:55:37Z

@drodriguez Some plugin tests started failing in ASAN bot https://ci.swift.org/job/oss-swift-incremental-ASAN-RA-macos/4553/ Could this be the cause?

Probably related. -fsanitize=address ends up in the CMAKE_C_FLAGS, it seems, which end up being used when compiling the plugins. I don't know if the plugins have address problems because the output is not very clear about what fails (some of the failures do not print a lot).

Do you prefer to revert and I will try to investigate ASan problems? It will take me a while to build an ASan build, I am afraid.

rintaro · 2023-12-04T19:12:50Z

FYI, tests are disabled in ASAN #70206
I started to building it as well, but if you have time, I'd appreciate it.

rintaro · 2023-12-04T20:05:16Z

The plugin failed with:

% /Users/rintaro/Repositories/swift-oss/build/Ninja-ReleaseAssert+asan/swift-macosx-arm64/test-macosx-arm64/Macros/Output/macro_swiftdeps.swift.tmp/mock-plugin
mock-plugin(80389,0x1dca2e5c0) malloc: nano zone abandoned due to inability to reserve vm space.
dyld[80389]: missing symbol called
zsh: abort

rintaro · 2023-12-04T20:13:53Z

-------------------------------------
Translated Report (Full Report Below)
-------------------------------------

Process:               mock-plugin [80504]
Path:                  /Users/USER/*/mock-plugin
Identifier:            mock-plugin

Exception Type:        EXC_CRASH (SIGABRT)
Exception Codes:       0x0000000000000000, 0x0000000000000000

Termination Reason:    Namespace DYLD, Code 9 
missing symbol called

Application Specific Information:
missing symbol called


Thread 0 Crashed::  Dispatch queue: com.apple.main-thread
0   dyld                          	       0x185cde55c __abort_with_payload + 8
1   dyld                          	       0x185ceab10 abort_with_payload_wrapper_internal + 104
2   dyld                          	       0x185ceab44 abort_with_payload + 16
3   dyld                          	       0x185c71584 dyld4::halt(char const*, dyld4::StructuredError const*) + 304
4   dyld                          	       0x185ca9ba0 dyld4::APIs::_dyld_missing_symbol_abort() + 28
5   lib_swiftMockPlugin.dylib     	       0x104c58d14 asan.module_ctor + 16
6   dyld                          	       0x185c89a24 invocation function for block in dyld4::Loader::findAndRunAllInitializers(dyld4::RuntimeState&) const::$_0::operator()() const + 168
7   dyld                          	       0x185ccf0f4 invocation function for block in dyld3::MachOAnalyzer::forEachInitializer(Diagnostics&, dyld3::MachOAnalyzer::VMAddrConverter const&, void (unsigned int) block_pointer, void const*) const + 172
8   dyld                          	       0x185cc2668 invocation function for block in dyld3::MachOFile::forEachSection(void (dyld3::MachOFile::SectionInfo const&, bool, bool&) block_pointer) const + 496
9   dyld                          	       0x185c692fc dyld3::MachOFile::forEachLoadCommand(Diagnostics&, void (load_command const*, bool&) block_pointer) const + 300
10  dyld                          	       0x185cc16a0 dyld3::MachOFile::forEachSection(void (dyld3::MachOFile::SectionInfo const&, bool, bool&) block_pointer) const + 192
11  dyld                          	       0x185cc4188 dyld3::MachOFile::forEachInitializerPointerSection(Diagnostics&, void (unsigned int, unsigned int, bool&) block_pointer) const + 160
12  dyld                          	       0x185ccede8 dyld3::MachOAnalyzer::forEachInitializer(Diagnostics&, dyld3::MachOAnalyzer::VMAddrConverter const&, void (unsigned int) block_pointer, void const*) const + 432
13  dyld                          	       0x185c85b38 dyld4::Loader::findAndRunAllInitializers(dyld4::RuntimeState&) const + 524
14  dyld                          	       0x185c8bf70 dyld4::JustInTimeLoader::runInitializers(dyld4::RuntimeState&) const + 36
15  dyld                          	       0x185c85f24 dyld4::Loader::runInitializersBottomUp(dyld4::RuntimeState&, dyld3::Array<dyld4::Loader const*>&) const + 220
16  dyld                          	       0x185c85f00 dyld4::Loader::runInitializersBottomUp(dyld4::RuntimeState&, dyld3::Array<dyld4::Loader const*>&) const + 184
17  dyld                          	       0x185c89ab0 dyld4::Loader::runInitializersBottomUpPlusUpwardLinks(dyld4::RuntimeState&) const::$_1::operator()() const + 112
18  dyld                          	       0x185c860f0 dyld4::Loader::runInitializersBottomUpPlusUpwardLinks(dyld4::RuntimeState&) const + 380
19  dyld                          	       0x185cab4dc dyld4::APIs::runAllInitializersForMain() + 464
20  dyld                          	       0x185c6dfa0 dyld4::prepare(dyld4::APIs&, dyld3::MachOAnalyzer const*) + 3192
21  dyld                          	       0x185c6cedc start + 1844

rintaro · 2023-12-04T21:14:14Z

Apparently this is because lib_swiftMockPlugin.dylib is built with the builder machine's compiler. But mock-plugin is built with the just built clang.

drodriguez · 2023-12-04T21:21:50Z

Thanks for all the clues. I am still in 1509/2444 when building the ASAN preset.

Maybe the trick is trying to match %swift-build-cxx-plugin to what it is used for lib_swiftMockPlugin.dylib.

If I understand https://github.com/apple/swift/blob/a7a0b329f21bf036cf65ebf073af8773b03eb934/cmake/modules/AddSwift.cmake#L149C4-L149C43 correctly, it might happen that add_swift_host_library will only add the sanitizer flags if a CMAKE_Swift_COMPILER is provided, which I am not sure if it is available in every case for lib_swiftMockPlugin.dylib.

I will keep waiting for the build to finish to be able to test theories on the actual compiled code.

rintaro · 2023-12-04T21:35:57Z

Yeah, I will open a PR to fix it. Basically, I will expose CMAKE_C_COMPILER via lit.site.cfg.in.

rintaro · 2023-12-04T22:07:21Z

#70213

…iftlang#70188) As pointed out in swiftlang#70136 (comment) the substitution name does not make sense because we are compiling C code, not C++. This should not introduce any behavioural changes.

drodriguez requested a review from compnerd November 30, 2023 22:33

compnerd requested review from rintaro and bnbarham November 30, 2023 22:35

compnerd approved these changes Nov 30, 2023

View reviewed changes

bnbarham approved these changes Nov 30, 2023

View reviewed changes

drodriguez merged commit b2e5d86 into swiftlang:main Dec 2, 2023

drodriguez deleted the test-plugin-compilation-weird-sysroots branch December 2, 2023 20:13

drodriguez mentioned this pull request Dec 2, 2023

[test] Replace %swift-build-cxx-plugin with %swift-build-c-plugin #70188

Merged

[lit] Use CMAKE_C_FLAGS and CMAKE_EXE_LINKER_FLAGS when compiling test plugin #70136

[lit] Use CMAKE_C_FLAGS and CMAKE_EXE_LINKER_FLAGS when compiling test plugin #70136

Uh oh!

Conversation

drodriguez commented Nov 30, 2023

Uh oh!

drodriguez commented Nov 30, 2023

Uh oh!

drodriguez commented Nov 30, 2023

Uh oh!

compnerd commented Nov 30, 2023

Uh oh!

drodriguez commented Nov 30, 2023

Uh oh!

compnerd commented Nov 30, 2023

Uh oh!

bnbarham left a comment

Choose a reason for hiding this comment

Uh oh!

bnbarham Nov 30, 2023

Choose a reason for hiding this comment

Uh oh!

drodriguez Nov 30, 2023

Choose a reason for hiding this comment

Uh oh!

bnbarham Dec 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rintaro commented Dec 4, 2023

Uh oh!

drodriguez commented Dec 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rintaro commented Dec 4, 2023

Uh oh!

rintaro commented Dec 4, 2023

Uh oh!

rintaro commented Dec 4, 2023

Uh oh!

rintaro commented Dec 4, 2023

Uh oh!

drodriguez commented Dec 4, 2023

Uh oh!

rintaro commented Dec 4, 2023

Uh oh!

rintaro commented Dec 4, 2023

Uh oh!

Uh oh!

bnbarham Dec 1, 2023 •

edited

Loading

drodriguez commented Dec 4, 2023 •

edited

Loading