build: enable ASAN #1963

ingwinlu · 2018-05-08T09:27:44Z

Purpose

Enables a docker ASAN build to replace the current one

Checklist

commit messages are fine ("module: short statement" syntax and refer to issues)
affected documentation is fixed
release notes are updated (doc/news/_preparation_next_release.md)

closes #2006
closes #2007

markus2330 · 2018-05-08T17:17:13Z

I think we need to disable lua for asan in this constellation then.

ingwinlu · 2018-05-09T17:59:14Z

@markus2330 just the test or the whole lua binding?

markus2330 · 2018-05-09T20:42:07Z

If the plugin crashes it makes sense to remove it completely (in this situation with ASAN).

We already have code to exclude plugins for ASAN builds, e.g. for yamlcpp.

@sanssecours Why does the lua plugin disable tests for "APPLE AND ENABLE_ASAN"? Does the plugin generally work with ASAN and only the tests fail?

sanssecours · 2018-05-09T21:49:11Z

Why does the lua plugin disable tests for "APPLE AND ENABLE_ASAN"?

As far as I remember because the tests do not work when ASAN is enabled (AddressSanitizer is loaded too late).

Does the plugin generally work with ASAN and only the tests fail?

Seems the bindings work fine. At least require kdb works inside the Lua interpreter after I export LUA_CPATH and set DYLD_INSERT_LIBRARIES according to the instruction printed after I used require "kdb" without setting DYLD_INSERT_LIBRARIES:

==80684==ERROR: Interceptors are not working. This may be because AddressSanitizer is loaded too late (e.g. via dlopen). Please launch the executable with:
DYLD_INSERT_LIBRARIES=/usr/local/opt/llvm/lib/clang/6.0.0/lib/darwin/libclang_rt.asan_osx_dynamic.dylib

.

markus2330 · 2018-05-12T05:38:18Z

I disabled https://build.libelektra.org/jenkins/job/elektra-clang-asan/

ingwinlu · 2018-05-15T05:01:28Z

I am still not sure how you want me to proceed here.

markus2330 · 2018-05-15T08:05:26Z

We should run ASAN with clang and gcc in the Jenkinsfile. If there are errors, we disable the respective plugin (in the constellation where it causes problem), document the reduced test coverage in doc/todo/TESTING (or if there is a good chance that someone fixes it by GitHub Issue).

The lua problems are unlikely to be fixed soon, as currently nobody seems to need this plugin.

ingwinlu · 2018-05-15T08:16:01Z

Did you look at the list of failing tests?

The following tests FAILED:
	 33 - testhaskell_basic (Failed)
	 34 - testhaskell_basic_optimized (Failed)
	 35 - testhaskell_realworld (Failed)
	 36 - testhaskell_realworld_optimized (Failed)
	 37 - testpy2_kdb.py (Failed)
	 38 - testpy2_key.py (Failed)
	 39 - testpy2_keyset.py (Failed)
	 40 - test_kdb.py (Failed)
	 41 - test_key.py (Failed)
	 42 - test_keyset.py (Failed)
	 43 - test_kdb.lua (Failed)
	 44 - test_key.lua (Failed)
	 45 - test_keyset.lua (Failed)
	 46 - testruby_kdb (Failed)
	 47 - testruby_key (Failed)
	 48 - testruby_keyset (Failed)
	 49 - testruby_tools_backendbuilder (Failed)
	 50 - testruby_tools_backendparser (Failed)
	 51 - testruby_tools_modules (Failed)
	 52 - testruby_tools_plugindatabase (Failed)
	 53 - testruby_tools_pluginspec (Failed)
	 54 - testruby_tools_specreader (Failed)
	 55 - testglib_kdb (Failed)
	 56 - testglib_key (Failed)
	 57 - testglib_keyset (Failed)
	 62 - testio_glib (Failed)
	 65 - testshell_markdown_base64 (Failed)
	 80 - testmod_crypto_botan (Failed)
	103 - testshell_markdown_ini (Failed)
	113 - testmod_lua (Failed)
	117 - testshell_markdown_mini (Failed)
	129 - testshell_markdown_tcl (Failed)
	149 - testscr_check_kdb_internal_check (Failed)
	177 - testshell_markdown_tutorial_validation (Failed)

If we disable all those constellations we might as well not run it as all.

markus2330 · 2018-05-15T08:34:19Z

No sorry, I did not have time to look at it.

But nearly everything seems to be related to swig (haskell, python, ruby) and glib. We can also disable all swig/glib bindings for this ASAN build. The other parts are actively maintained:

testmod_crypto_botan and testshell_markdown_base64 by @petermax2
testshell_markdown_mini testshell_markdown_ini by @sanssecours
testshell_markdown_tutorial_validation is known to fail, also without ASAN, see testshell_markdown_tutorial_validation fails #1926

If we continue to not run it (or run it with errors which means the build is ignored) the code will degrade even more. If we check for what works now, we at least do not get regressions.

As long as there is no upstream support of ASAN builds for our dependencies (and it seems to be lacking for quite some libs and languages) it is unrealistic that everything works with ASAN. This is not something that can be fixed within our source.

But we as library should enable other applications to be ASAN and valgrind clean. So we need to be pioneers in this area.

sanssecours · 2018-05-15T09:21:48Z

… testshell_markdown_ini…@sanssecours

I am pretty certain that you misspelled @tom-wa here. By the way: It is also possible that base64, which we load in the Markdown Shell Recorder test of the ini plugin, is responsible for the failure of testshell_markdown_ini.

sanssecours · 2018-05-15T09:48:54Z

Concerning testshell_markdown_mini the test works fine on Ubuntu (using GCC and Clang) and macOS (using Clang).

The test fails on my machine, if I enable the leak sanitizer. I can fix the problem by not requiring the code plugin though. So I guess the real culprit here is the code plugin, since requiring other plugins (I tried mathcheck) seems to work fine.

markus2330 · 2018-05-15T09:58:45Z

Thank you for looking into it!

I am pretty certain that you misspelled @tom-wa here.

Okay, then this test needs to be excluded, too 😉 Luckily INI is not the default.

I can fix the problem by not requiring the code plugin though.

Sounds good. Alternatively you could try one of the other code plugins?

I am also okay with excluding all these tests for this ASAN build, none of them tests essential or core functionality.

After this update the test `testshell_markdown_mini` should also complete successfully if we - build Elektra with enabled address sanitizer and - enable the leak sanitizer (via `ASAN_OPTIONS=detect_leaks=1`) . See also: ElektraInitiative#1963

ingwinlu · 2018-05-17T13:56:40Z

excluding stuff

easier said than done. I expected all the dependent stuff to deactivate itself but it seems like it isn't set up correctly.

ingwinlu · 2018-05-17T14:26:32Z

-- Exclude Binding glib because glib is not compatible with ENABLE_ASAN
CMake Error at src/bindings/gsettings/CMakeLists.txt:35 (add_subdirectory):
  The binary directory
    /home/jenkins/workspace/libelektra_PR-1963-UY7VIGJMXZ2D2SSXYCECW7I6XLHKVOJM6XQS67PEYRQNRNHU5NRA@3/build/src/bindings/glib
  is already used to build a source directory.  It cannot be used to build source directory
    /home/jenkins/workspace/libelektra_PR-1963-UY7VIGJMXZ2D2SSXYCECW7I6XLHKVOJM6XQS67PEYRQNRNHU5NRA@3/src/bindings/glib
  Specify a unique binary directory name.

Is there no handling of binding dependencies at all?

markus2330 · 2018-05-17T17:20:34Z

The dependency handling needs to be implemented manually in CMake. Seems like it was forgotten in the (unfortunately unmaintained) gsettings.

Please implement it or at least open separate bug reports.

Is there no handling of binding dependencies at all?

A very short time ago it was not even easily possible to query which binding is included. It was a lot of work done recently by @waht.

markus2330 · 2018-05-17T17:59:28Z

cmake/Modules/LibAddHaskellPlugin.cmake

-
-	# compile our c wrapper which takes care of invoking the haskell runtime
-	# the actual haskell plugin gets linked in dynamically as a library
-	add_plugin (${target}


add_plugin needs to be present at all stages, to disable plugins remove_plugin needs to be called.

that was not the problem

ingwinlu · 2018-05-17T18:19:12Z

@markus2330 i can not add codeblocks for in code comments so I will answer here in full

-- Configuring done
CMake Error at cmake/Modules/LibAddPlugin.cmake:442(add_dependencies):
  The dependency target "c2hs_haskell" of target "elektra-haskell" does not
  exist.
Call Stack (most recent call first):
  cmake/Modules/LibAddHaskellPlugin.cmake:239 (add_plugin)
  src/plugins/haskell/CMakeLists.txt:3 (add_haskell_plugin)
CMake Error at cmake/Modules/LibAddPlugin.cmake:402 (add_dependencies):
  The dependency target "c2hs_haskell" of target "elektra-haskell-objects"
  does not exist.
Call Stack (most recent call first):
  cmake/Modules/LibAddHaskellPlugin.cmake:239 (add_plugin)
  src/plugins/haskell/CMakeLists.txt:3 (add_haskell_plugin)

As you can see the block needed to be moved up to where all the if blocks are. And that is all that I changed.

Notification async example needs oi_uv bindings. This binding is not build for ASAN builds. Until proper handling of dependencies in cmake is implemented (ElektraInitiative#2007) we manually disable the example for such builds.

This update fixes memory leaks in the following tests: - `testshell_markdown_base64`, - `testshell_markdown_ini`, and - `testshell_markdown_tcl` .

Since the Markdown Shell Recorder test leaks memory, if we load `binary` instead of `base64` (which provides `binary`), we also rewrite the MSR test.

markus2330

Thank you, looks great.

Some questions and minor things.

markus2330 · 2018-05-23T09:06:35Z

tests/shell/check_kdb_internal_check.sh

@@ -33,6 +33,23 @@ do
 		;;
 	esac

+	# The following checks fail on an ASAN enabled build
+	# See also: https://github.com/ElektraInitiative/libelektra/pull/1963


please document in doc/todo/TESTING

done in 90a6608

markus2330 · 2018-05-23T09:07:23Z

src/plugins/tcl/README.md

@@ -2,7 +2,7 @@
 - infos/author = Markus Raab <elektra@libelektra.org>
 - infos/licence = BSD
 - infos/provides = storage/tcl
- infos/needs = code null binary
+- infos/needs = ccode null base64


as already said, it would be better if we can use generic providers here (like binary) and fix the tutorials to use concrete plugins (that pass ASAN checks)

Would it make sense to solve this in a different PR?

we can use generic providers here (like binary) and fix the tutorials to use concrete plugins

I do not think that will work here, since these plugins are listed under infos/needs and not under infos/recommends.

If you explicitly state needed plugins, kdb mount will know that the dependency is already fulfilled.

markus2330 · 2018-05-23T09:08:49Z

src/plugins/ini/README.md

@@ -146,7 +146,7 @@ By default the INI plugin does not support binary data. You can use the [Base64

 ```sh
 # Mount INI and recommended plugin Base64
-sudo kdb mount --with-recommends config.ini user/examples/ini ini
+sudo kdb mount config.ini user/examples/ini ini base64


Doesn't --with-recommends together with base64 also do the right thing? And we should document that base64 is added to pass ASAN tests.

Doesn't --with-recommends together with base64 also do the right thing?

While this works, using --with-recommends is redundant and might also be confusing here.

Yes, thus we should explain that "base64" is only there to make ASAN happy.

markus2330 · 2018-05-23T09:11:49Z

src/bindings/swig/CMakeLists.txt

@@ -6,6 +6,11 @@ if (NOT SWIG_FOUND)
 	return ()
 endif ()

+if (ENABLE_ASAN)
+	exclude_binding (swig "SWIG is not compatible with ENABLE_ASAN")
+	return ()


Would this return be problematic once we change the bindings to behave like the plugins?

I don't expect so

markus2330 · 2018-05-23T09:13:35Z

src/bindings/glib/CMakeLists.txt

 if (NOT GOBJECT_FOUND)
 	exclude_binding (glib "No gobject found using pkg-config, also remove gi bindings because they depend on it")
-else ()
-	add_binding (glib)
+	return ()


See below. Furthermore the diff is hard to read because of this reformatting.

markus2330 · 2018-05-23T09:14:06Z

cmake/Modules/LibAddPlugin.cmake

@@ -52,6 +52,10 @@ function (add_plugintest testname)
 						"Trying to add plugintest ${testname} but no such plugin was added (try to use LINK_PLUGIN)"
 					)
 			endif ()
+
+			# plugin for test was not added in previous phase or removed


Is this also relevant for the docu above?

markus2330 · 2018-05-23T09:14:43Z

cmake/Modules/LibAddHaskellPlugin.cmake

@@ -215,6 +215,78 @@ macro (add_haskell_plugin target)
 									     "${GHC_GMP_PATH}"
 									     "${CMAKE_INSTALL_RPATH}"
 									     "${CMAKE_INSTALL_PREFIX}/lib${LIB_SUFFIX}/elektra/haskell")
+


@e1528532 Can you take a look?

This certainly needs a restructuring.

ingwinlu · 2018-05-23T09:23:48Z

doc/news/_preparation_next_release.md

@@ -147,6 +148,7 @@ These notes are of interest for people developing Elektra:
 - A build job checks if PRs modify the release notes. *(Markus Raab)*
 - <<TODO>>
 - <<TODO>>
+- Ported GCC ASAN build job to new build system *(René Schwaiger + Lukas Winkler)


I missed a * here :(

ingwinlu · 2018-05-23T14:43:20Z

anything else @markus2330?

markus2330 · 2018-05-23T15:37:15Z

jenkins build all please

markus2330 · 2018-05-23T15:40:05Z

Thank you, looks good!

I am not sure if the kdb mount itself leaks memory or later commands but I think the added lines should be clear anyway.

anything else @markus2330?

You can simply add "ready to merge" if you finished the reviews and all build jobs finished successfully. (Or additionally ping me if I do not respond).

Btw. if you have changes beyond Jenkinsfile, please do not forget to call "jenkins build all please".

markus2330 · 2018-05-23T19:10:57Z

Thank you for your great work!

ingwinlu added the build label May 8, 2018

ingwinlu self-assigned this May 8, 2018

ingwinlu added the work in progress label May 8, 2018

ingwinlu mentioned this pull request May 8, 2018

asan tests fail after build on stretch #1895

Closed

markus2330 mentioned this pull request May 12, 2018

Crypto tutorial #1955

Merged

8 tasks

ingwinlu force-pushed the enable_asan branch 5 times, most recently from 5703d75 to 50bdeb3 Compare May 17, 2018 13:48

ingwinlu force-pushed the enable_asan branch from 50bdeb3 to f0dd911 Compare May 17, 2018 15:06

markus2330 reviewed May 17, 2018

View reviewed changes

ingwinlu force-pushed the enable_asan branch from 769dcd7 to 6198ea0 Compare May 17, 2018 19:49

ingwinlu and others added 15 commits May 23, 2018 08:57

cmake: disable io binding for ENABLE_ASAN

93d1e92

build: disable notification async example

b9cc989

Notification async example needs oi_uv bindings. This binding is not build for ASAN builds. Until proper handling of dependencies in cmake is implemented (ElektraInitiative#2007) we manually disable the example for such builds.

build: Do not build haskell binding with ASAN

8faad33

build: do not set haskell targets when haskell is not build

8035f3e

Plugins: Use plugin name instead of provider name

474053e

This update fixes memory leaks in the following tests: - `testshell_markdown_base64`, - `testshell_markdown_ini`, and - `testshell_markdown_tcl` .

INI: Recommend binary as plugin provider

34a2710

Since the Markdown Shell Recorder test leaks memory, if we load `binary` instead of `base64` (which provides `binary`), we also rewrite the MSR test.

Validation: Require plugin ni for MSR test

aff0f99

Ni: Disable plugin on ASAN enabled build

19d22f6

Build: Reformat CMake code

702f20d

Conditionals: Add required plugins for MSR test

0ecb672

Base64: Fix minor spelling mistake in ReadMe

f89a77e

Release: Remove useless whitespace

e74db88

Release: Add information about Jenkins ASAN build

07d4828

Internal Check: Disable plugins on ASAN build

749040d

doc: mention asan job port in release news

5882def

ingwinlu force-pushed the enable_asan branch from db548c9 to 5882def Compare May 23, 2018 07:04

ingwinlu added ready to merge and removed work in progress labels May 23, 2018

markus2330 reviewed May 23, 2018

View reviewed changes

markus2330 removed the ready to merge label May 23, 2018

ingwinlu commented May 23, 2018

View reviewed changes

ingwinlu and others added 3 commits May 23, 2018 11:24

fixup: add missing *

24bd870

docs: document disabled tests when using ASAN

90a6608

Plugins: Use provider names in contract

0c00c60

markus2330 merged commit c315471 into ElektraInitiative:master May 23, 2018

ingwinlu deleted the enable_asan branch May 25, 2018 08:37

build: enable ASAN #1963

build: enable ASAN #1963

Conversation

ingwinlu commented May 8, 2018 • edited Loading

Purpose

Checklist

markus2330 commented May 8, 2018

ingwinlu commented May 9, 2018

markus2330 commented May 9, 2018

sanssecours commented May 9, 2018

markus2330 commented May 12, 2018

ingwinlu commented May 15, 2018

markus2330 commented May 15, 2018

ingwinlu commented May 15, 2018

markus2330 commented May 15, 2018

sanssecours commented May 15, 2018

sanssecours commented May 15, 2018

markus2330 commented May 15, 2018

ingwinlu commented May 17, 2018

ingwinlu commented May 17, 2018

markus2330 commented May 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ingwinlu commented May 17, 2018

markus2330 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ingwinlu commented May 23, 2018

markus2330 commented May 23, 2018

markus2330 commented May 23, 2018

markus2330 commented May 23, 2018

ingwinlu commented May 8, 2018 •

edited

Loading