NG CI: marble needs custom ASAN handling (as on old CI)
Closed, ResolvedPublic

Description

Currenly all Marble tests fail due to ASan not being properly available.
See e.g. https://build-sandbox.kde.org/job/Applications%20marble%20kf5-qt5%20FedoraQt5.8/4/testReport/

Possibly the same extra handling for Marble as on old CI is needed. See old task https://phabricator.kde.org/T2366 and some later commit for old CI https://phabricator.kde.org/R12:055661b2b76d3e1001f047b310c5e32b03f7dd84

Restricted Application added a subscriber: sysadmin. · View Herald TranscriptJun 12 2017, 10:28 PM
bcooksley added a subscriber: bcooksley.

Forcibly injecting ASAN won't help the FreeBSD builds, which bail due to Clang being more strict about it's linking.
Please see https://build-sandbox.kde.org/view/Applications/job/Applications%20marble%20kf5-qt5%20FreeBSDQt5.7/4/console

As it would be nice to fix things properly on the whole, could we either:
a) Get Frameworks CMake adjusted to properly include ASAN as a library to be linked to?
b) Get Marble CMake adjusted to include ASAN when needed.

I should note that Minuet appears to be bitten by the same issue on FreeBSD as well, so a) may be the more long term option, although I suspect it'll probably be harder.

Those linking errors might be due to

Q: When I link my shared library with -fsanitize=address, it fails due to some undefined ASan symbols (e.g. asan_init_v4)?

A: Most probably you link with -Wl,-z,defs or -Wl,--no-undefined. These flags don't work with ASan unless you also use -shared-libasan (which is the default mode for GCC, but not for Clang).

From https://github.com/google/sanitizers/wiki/AddressSanitizer

On another look, seems the related ECM macros already try to deal with that a little:

if (CMAKE_CXX_COMPILER_ID STREQUAL "Clang")
    string(REPLACE "-Wl,--no-undefined" "" CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}")
    string(REPLACE "-Wl,--no-undefined" "" CMAKE_MODULE_LINKER_FLAGS "${CMAKE_MODULE_LINKER_FLAGS}")
endif ()

(from https://cgit.kde.org/extra-cmake-modules.git/tree/modules/ECMEnableSanitizers.cmake#n165)

So either this is broken or perhaps the other option, "-Wl,-z,defs", is hit here. Perhaps instead "-shared-libasan" should be injected, and thus enforcing a shared version of the asan libs?

@aacid , @Wenzel, @lesliezhai, what do you as ECMEnableSanitizers.cmake authors think about this?

aacid added a comment.Jun 15 2017, 9:19 PM

I don't think i know enough about this, @lesliezhai probably knows more?

But also, can't you just try if it works? If it works, it's good :D

Marble doesn't use anything from ECM which is why it hits that issue. I presume Minuet is the same here.

Either:
a) ECM needs to be adjusted to ensure the necessary ASAN flags get pushed down through the *Config.cmake files
b) Marble and Minuet need to be like everyone else and just use ECM.

Any update on this?

I've setup force injection for Linux, but as noted above the FreeBSD failures are still relevant.
Given that at least one of the affected projects on FreeBSD doesn't use ECM fully, the correct fix here is to just use ECM fully (which would also obviate the need to inject on Linux).

kfunk added a subscriber: kfunk.Jul 31 2017, 3:18 PM

I've setup force injection for Linux, but as noted above the FreeBSD failures are still relevant.
Given that at least one of the affected projects on FreeBSD doesn't use ECM fully, the correct fix here is to just use ECM fully (which would also obviate the need to inject on Linux).

+1.

I've just discussed this with Friedrich. The only sane fix is to make Marble (and others) just respect the ECM_ENABLE_SANTIZERS CMake option (IOW: include KDECompilerSettings.cmake).

In T6328#104277, @kfunk wrote:

I've just discussed this with Friedrich. The only sane fix is to make Marble (and others) just respect the ECM_ENABLE_SANTIZERS CMake option (IOW: include KDECompilerSettings.cmake).

Which means: limiting KDE CI to projects using CMake and ECM (if C/C++). But KDE no-where has this requirement for its projects, on what language to use or what build system. Do you propose to block e.g. any qmake/qbs projects from CI?

Instead of creating dependencies between KDE CI and single custom build system additions, could we instead document the expectations the KDE CI has when it comes to set up for CI?
So projects could decide themselves how they see to fulfill those expectations.

I meanwhile will prepare a patch to add -shared-libasan to the linker flags if clang and those flags -Wl,-z,defs or -Wl,--no-undefined are set.
Should be up as review request later tonight.

kfunk added a comment.Jul 31 2017, 4:23 PM

I meanwhile will prepare a patch to add -shared-libasan to the linker flags if clang and those flags -Wl,-z,defs or -Wl,--no-undefined are set.
Should be up as review request later tonight.

Note that even with -shared-libasan you still need to LD_PRELOAD the ASAN-runtime if the executable you're attempting to run was not linked against the ASAN-runtime directly. So as far as I can see, that does not improve anything, does it?

In T6328#104283, @kfunk wrote:

Note that even with -shared-libasan you still need to LD_PRELOAD the ASAN-runtime if the executable you're attempting to run was not linked against the ASAN-runtime directly. So as far as I can see, that does not improve anything, does it?

Good hint, though when it comes to CI, this is currently done (cmp. T6328#100345 and commit linked above).

kfunk added a comment.Jul 31 2017, 4:27 PM
In T6328#104277, @kfunk wrote:

I've just discussed this with Friedrich. The only sane fix is to make Marble (and others) just respect the ECM_ENABLE_SANTIZERS CMake option (IOW: include KDECompilerSettings.cmake).

Which means: limiting KDE CI to projects using CMake and ECM (if C/C++). But KDE no-where has this requirement for its projects, on what language to use or what build system. Do you propose to block e.g. any qmake/qbs projects from CI?

No. But as KF5 libraries are built with ASAN enabled, you'd be better off building applications *using* KF5 with ASAN enabled, too. Though you can work-around on Linux using LD_PRELOAD, that apparently doesn't work on FreeBSD as reported:

Forcibly injecting ASAN won't help the FreeBSD builds, which bail due to Clang being more strict about it's linking.
Please see https://build-sandbox.kde.org/view/Applications/job/Applications%20marble%20kf5-qt5%20FreeBSDQt5.7/4/console

Note that QMake/QBS can be easily instructed to build with ASAN enabled as well.

Instead of creating dependencies between KDE CI and single custom build system additions, could we instead document the expectations the KDE CI has when it comes to set up for CI?
So projects could decide themselves how they see to fulfill those expectations.

Note that QMake/QBS can be easily instructed to build with ASAN enabled as well.

Yes, but that is my very point :) We would need to know how to instruct them. With QMake & QBS I cannot use ECMEnableSanitizers.cmake either.

Just to make clear, I am not opposing against using ASAN on CI (in fact I very much like it, like everyone. great to have that there).
I just am opposing against any project being forced to use ECM, when so far it did not (for whatever reasons).
And indeed Marble code itself should in the end also be covered by ASAN IMHO.

In general i'm not a fan of QMake, primarily because it's support for things like install prefixes is basically non-existent.
In the past we've had to patch the build system to set an install prefix if the project used QMake.

I wouldn't call these expectations of the CI system (the tooling doesn't care either way). They're expectations imposed by the way we build Frameworks and how it handles the arguments we pass to it.

In terms of the "proper fix" which wouldn't require changing Marble, then we should adjust the Frameworks *Config.cmake files to automatically add the necessary compiler flags if Frameworks was built with ASAN enabled (or just drag in the KDECompilerSettings.cmake and be done with it)

I've no idea how hard that is though, but for FreeBSD at least it seems to be a hard requirement (and is basically a hard requirement on Linux as well, with the notable exception you can workaround with LD_PRELOAD)

aacid added a comment.Jul 31 2017, 8:15 PM

In terms of the "proper fix" which wouldn't require changing Marble, then we should adjust the Frameworks *Config.cmake files to automatically add the necessary compiler flags if Frameworks was built with ASAN enabled

Yes, i think this has been mentioned a few times as the ideal fix, but tbh i don't know how to do it myself.