ASAN: support on clang and fixed GIR buildin with UBSAN #803

rautesamtr · 2016-06-15T16:50:17Z

This commit adds support for llvm/clang adress sanitizer.
It can be can only be enabled in combination with debug on.
e.g. -DENABLE_DEBUG=ON -DENABLE_ASAN=ON Fixes #801
It also fixes problems when compiling GI bindings with the undefined
behavior sanitizer by passing the corresponding libraries (-lubsan
-lasan). Fixes #786

markus2330 · 2016-06-15T17:07:34Z

I would prefer that ENABLE_DEBUG and ENABLE_ASAN work independently.

Is it really required to have -fsanitize=undefined -fsanitize=integer set so that ASAN works?

rautesamtr · 2016-06-15T17:09:37Z

Is it really required to have -fsanitize=undefined -fsanitize=integer set so that ASAN works?

No we can let it work independently. I was waiting on your opinion on this mater anyway.

rautesamtr · 2016-06-15T17:12:13Z

We could make it available as a general option as it should be possible to use it even with gcc as long as llvm/clang is installed.

markus2330 · 2016-06-15T17:15:50Z

It is always good to avoid dependencies between options. Its confusing and error-prone.

I expected that ENABLE_ASAN is meant to be compiler-independent. So everyone can step up and write support for other compilers.

E.g. ENABLE_COVERAGE is compiler-independent but currently only does something when gcc is used.

rautesamtr · 2016-06-15T17:26:18Z

GCC supports the -fsanitize=address since 4.8.
I probably just misunderstood, it seems they actually integrated the ASAN from LLVM into GCC so yes it should be compiler independent as long as they support the -fsanitize=address option.

markus2330 · 2016-06-15T17:28:53Z

Then please set -fsanitize=address for both gcc and clang if ENABLE_ASAN was set.

This commit adds support for llvm/clang adress sanitizer. It can be can only be enabled with `-DENABLE_ASAN=ON`. Fixes #801 It also fixes problems when compiling GI bindings with the undefined behavior sanitizer by passing the corresponding libraries (-lubsan -lasan). Fixes #786

rautesamtr · 2016-06-15T17:37:46Z

I am running in some issues with GCC end ASAN enabled:

[ 41%] Linking CXX executable ../../../bin/testmod_type
/usr/bin/ld: ../../../lib/libgtest.a(gtest-all.cc.o): undefined reference to symbol 'pthread_key_delete@@GLIBC_2.2.5'
/usr/lib/../lib/libpthread.so.0: error adding symbols: DSO missing from command line

rautesamtr · 2016-06-15T17:50:12Z

It also looks like i still need to handle libasan selection correctly

We should only pass -lasan when using gcc and ASAN_ENABLED as this otherwise results in clang linking the gcc sanitizer which in return results in broken binarys. Also currently use a workaround to add -lpthread when using ASAN with gcc. This probably should be moved into the GTest logic of the cmake files, but i am currently lacking the expertise to do this correctly.

rautesamtr · 2016-06-15T19:29:43Z

Then please set -fsanitize=address for both gcc and clang if ENABLE_ASAN was set.

should work now.

rautesamtr · 2016-06-15T19:30:30Z

cmake/ElektraCompiling.cmake

+		set (CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -fsanitize=address")
+		# this is currently a workaround so gtests compile with ASAN
+		#TODO someone with expertise migrate this into LibAddTest
+		set (COMMON_FLAGS "${COMMON_FLAGS} -lpthread")


this is rather a workaround for the gcc asan problem by explicitly adding -lpthread. Someone with expertise in the gtest phtread setup for elektra should probably take a look if there is a better way to handle this.

You need:

find_package(Threads) and then you can add ${CMAKE_THREAD_LIBS_INIT} to linker flags

Are you sure it should be in COMMON_FLAGS?

find_package(Threads)
and then you can add ${CMAKE_THREAD_LIBS_INIT} to linker flags

This is already used in LibAddTest and i still run into compiling issues. Adding it globally fixed them. Therefore the question if someone with expertise in this can solve this better.

My suggestion was to not directly use -lpthread (that is not portable), but to use ${CMAKE_THREAD_LIBS_INIT} instead. With the change we can merge the PR.

If -fsanitize=address generates code that requires to be linked with ${CMAKE_THREAD_LIBS_INIT} then we will have to do this, someone with more expertise will also not be able to change this fact.

If its needed can be found out by compiling and linking it. No expertise required here.

Maybe @mpranj has time to set up a new build job that uses clang with ENABLE_ASAN?

So you say that it actually is not needed globally but only gtest suddenly does not link with ENABLE_ASAN? Then we should not enable it globally, but fix the linking order for the gtests.

We had a issue with pthread and gtest already here: #737

You can either find out the exact dependencies of every library and do a topological sort or you can simply guess and shuffle around the linker flags until it works (as done in #737)

Do I understand this correctly: A new build job with ENABLE_ASAN is not needed any more?

No it will be needed once i fix the linking granularity and this pr get merged.

Exactly, the build job will be needed ;) The not needed referred to the global pthread linker flag ;) Let us use #160 in future to avoid such confusions.

It would also be interesting to have it before the PR is merged: we can trigger the build job from here.

So i investigated this issue further and tried to solve it for gtest but that only moved the problem to the next library using pthread. This bug report explains this issue for gcc further. According to it, clang solves it by linking to pthread when -fsanitize=address is passed, gcc does not. So i propose to use the solution from the the bug report to pass-Wl,--as-needed ${CMAKE_THREAD_LIBS_INIT} as last compiler flag.

This adds the thread link with --as-needed when ASAN option is enabled on gcc builds. The if ENABLE_ASAN block was also moved below the GCC block so it gets appended to both clang and gcc build in the same order.

rautesamtr · 2016-06-17T15:51:59Z

jenkins build clang-asan please

ASAN needs to be linked before other things otherwis it produces the following error message: `ASan runtime does not come first in initial library list; you should either link runtime to your application or manually preload it with LD_PRELOAD.`

markus2330 · 2016-06-20T12:55:04Z

Looks good, everything builds now.

rautesamtr · 2016-06-20T13:04:33Z

jenkins build clang-asan please

rautesamtr · 2016-06-20T13:07:11Z

all but the clang-asan, but that will need some suppression files #788

markus2330 · 2016-11-18T09:28:13Z

@raetiacorvus Thanks, great job! I merged it because clang with ENABLE_DEBUG is broken and ENABLE_ASAN seems like a very nice solution!

rautesamtr reviewed Jun 15, 2016
View reviewed changes

rautesamtr added 2 commits June 16, 2016 13:25

ASAN: use portable thread library linking

904b81b

ASAN: append thread libs as needed for gcc with ASAN

e9903ef

This adds the thread link with --as-needed when ASAN option is enabled on gcc builds. The if ENABLE_ASAN block was also moved below the GCC block so it gets appended to both clang and gcc build in the same order.

ASAN: move asan flag into the front

1e7f9e0

ASAN needs to be linked before other things otherwis it produces the following error message: `ASan runtime does not come first in initial library list; you should either link runtime to your application or manually preload it with LD_PRELOAD.`

markus2330 added work in progress and removed work in progress labels Sep 16, 2016

markus2330 added this to the 0.8.19 milestone Nov 11, 2016

markus2330 mentioned this pull request Nov 15, 2016

cc: error: unrecognized argument to -fsanitize= option: ‘integer’ #1097

Closed

markus2330 merged commit 199bee5 into ElektraInitiative:master Nov 18, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ASAN: support on clang and fixed GIR buildin with UBSAN #803

ASAN: support on clang and fixed GIR buildin with UBSAN #803

rautesamtr commented Jun 15, 2016

markus2330 commented Jun 15, 2016

rautesamtr commented Jun 15, 2016

rautesamtr commented Jun 15, 2016

markus2330 commented Jun 15, 2016 •

edited

Loading

rautesamtr commented Jun 15, 2016

markus2330 commented Jun 15, 2016

rautesamtr commented Jun 15, 2016

rautesamtr commented Jun 15, 2016

rautesamtr commented Jun 15, 2016 •

edited

Loading

rautesamtr Jun 15, 2016

markus2330 Jun 16, 2016

rautesamtr Jun 16, 2016 •

edited

Loading

markus2330 Jun 16, 2016

markus2330 Jun 16, 2016

markus2330 Jun 16, 2016

mpranj Jun 16, 2016

rautesamtr Jun 16, 2016

markus2330 Jun 16, 2016 •

edited

Loading

rautesamtr Jun 17, 2016

rautesamtr commented Jun 17, 2016

markus2330 commented Jun 20, 2016

rautesamtr commented Jun 20, 2016

rautesamtr commented Jun 20, 2016

markus2330 commented Nov 18, 2016

ASAN: support on clang and fixed GIR buildin with UBSAN #803

ASAN: support on clang and fixed GIR buildin with UBSAN #803

Conversation

rautesamtr commented Jun 15, 2016

markus2330 commented Jun 15, 2016

rautesamtr commented Jun 15, 2016

rautesamtr commented Jun 15, 2016

markus2330 commented Jun 15, 2016 • edited Loading

rautesamtr commented Jun 15, 2016

markus2330 commented Jun 15, 2016

rautesamtr commented Jun 15, 2016

rautesamtr commented Jun 15, 2016

rautesamtr commented Jun 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rautesamtr Jun 16, 2016 • edited Loading

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

markus2330 Jun 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rautesamtr commented Jun 17, 2016

markus2330 commented Jun 20, 2016

rautesamtr commented Jun 20, 2016

rautesamtr commented Jun 20, 2016

markus2330 commented Nov 18, 2016

markus2330 commented Jun 15, 2016 •

edited

Loading

rautesamtr commented Jun 15, 2016 •

edited

Loading

rautesamtr Jun 16, 2016 •

edited

Loading

markus2330 Jun 16, 2016 •

edited

Loading