Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Sanitizer tools #726

Merged
merged 4 commits into from
Jul 8, 2016
Merged

Add support for Sanitizer tools #726

merged 4 commits into from
Jul 8, 2016

Conversation

dongahn
Copy link
Member

@dongahn dongahn commented Jul 8, 2016

Initial support for AddressSanitizer (ASan) and ThreadSanitizer (TSan).

This is tested with clang-omp-3.5.0 installed at LLNL TOSS2 systems (e.g., Cab). For example,

% use clang-omp-3.5.0
% ./configure CC=clang

Also mildly tested with gcc 4.9.3p. For example,

% use gcc 4.9.3p
% ./configure

@garlick garlick added the review label Jul 8, 2016
@dongahn dongahn mentioned this pull request Jul 8, 2016
@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 75.233% when pulling 965fbd9 on dongahn:new_san into 3efa493 on flux-framework:master.

@grondo
Copy link
Contributor

grondo commented Jul 8, 2016

Thanks @dongahn!

Minor suggestion for the leak fix commit 965fbd9. Instead of subject leak: Fix a minor leak... we should keep the prefix: to be the "subsystem" of the tree which the patch affects (and idiom we've been loosely following). In this case I'd suggest perhaps libutil: fix leak in nodeset test found by AddressSanitizer or test/libutil: fix leak in nodeset test found by AddressSanitizer.

It is nicer if a commit can be mostly understood (what is fixed/changed and why) from just the commit summary.

@dongahn
Copy link
Member Author

dongahn commented Jul 8, 2016

Thanks. Yes, it makes sense. I will change it and force a push.

@dongahn
Copy link
Member Author

dongahn commented Jul 8, 2016

OK. Pushed.

@coveralls
Copy link

coveralls commented Jul 8, 2016

Coverage Status

Coverage decreased (-0.004%) to 75.136% when pulling 7edf40b on dongahn:new_san into 3efa493 on flux-framework:master.

@garlick
Copy link
Member

garlick commented Jul 8, 2016

This looks good to me. Could we have it rebased on current master? Then it can go in I think.

Add `--enable-sanitizer` as a configure option.

Support ASan and TSan for now. Not LeakSanitizer (LSan) and MemorySanitizer (MSan).
This is because ASan is a superset of LSan and the lower versions
of the compilers available on LLNL systems do not yet support LSan and MSan.

Integration is straightforward. It appeared that Sanitizers do not like
the `--no-undefined` linker option. So, this commit drops this option
for linking when `--enable-sanitizer` is given.

Other follow-up issues have been created in GitHub:
Issue #720, #721, #722, #724 and #725.
@coveralls
Copy link

coveralls commented Jul 8, 2016

Coverage Status

Changes Unknown when pulling 2f0a1dd on dongahn:new_san into * on flux-framework:master*.

@garlick
Copy link
Member

garlick commented Jul 8, 2016

Ready @dongahn?

@dongahn
Copy link
Member Author

dongahn commented Jul 8, 2016

Give me just one min. I am checking one thing.

@dongahn
Copy link
Member Author

dongahn commented Jul 8, 2016

I was looking at a compilation error as clang 3.5 was complaining on the new cronodate.c. @garlick @grondo: Do you want me to add a fix to this as part of this PR?

  CC     test/test_cronodate_t-cronodate.o
test/cronodate.c:59:38: error: comparison of constant 0 with expression of type 'bool' is always true
      [-Werror,-Wtautological-constant-out-of-range-compare]
    ok (string_to_tm (expected, &tm) >= 0,
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~
../../../src/common/libtap/tap.h:48:56: note: expanded from macro 'ok'
#define ok(...)          ok_at_loc(__FILE__, __LINE__, __VA_ARGS__, NULL)
                                                       ^
test/cronodate.c:64:35: error: comparison of constant 0 with expression of type 'bool' is always true
      [-Werror,-Wtautological-constant-out-of-range-compare]
    ok (string_to_tm (start, &tm) >= 0,
        ~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~
../../../src/common/libtap/tap.h:48:56: note: expanded from macro 'ok'
#define ok(...)          ok_at_loc(__FILE__, __LINE__, __VA_ARGS__, NULL)
                                                       ^
test/cronodate.c:222:51: error: comparison of constant 0 with expression of type 'bool' is always true
      [-Werror,-Wtautological-constant-out-of-range-compare]
    ok (string_to_tm ("2016-06-06 08:00:00", &tm) >= 0, "string_to_tm");
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~
../../../src/common/libtap/tap.h:48:56: note: expanded from macro 'ok'
#define ok(...)          ok_at_loc(__FILE__, __LINE__, __VA_ARGS__, NULL)
                                                       ^
test/cronodate.c:231:53: error: comparison of constant 0 with expression of type 'bool' is always true
      [-Werror,-Wtautological-constant-out-of-range-compare]
    ok (string_to_tv ("2016-06-06 07:00:00.3", &tv) >= 0, "string_to_tv");
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~
../../../src/common/libtap/tap.h:48:56: note: expanded from macro 'ok'
#define ok(...)          ok_at_loc(__FILE__, __LINE__, __VA_ARGS__, NULL)
                                                       ^
test/cronodate.c:235:51: error: comparison of constant 0 with expression of type 'bool' is always true
      [-Werror,-Wtautological-constant-out-of-range-compare]
    ok (string_to_tv ("2016-06-06 08:00:00", &tv) >= 0, "string_to_tv");
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^  ~
../../../src/common/libtap/tap.h:48:56: note: expanded from macro 'ok'
#define ok(...)          ok_at_loc(__FILE__, __LINE__, __VA_ARGS__, NULL)

@garlick
Copy link
Member

garlick commented Jul 8, 2016

Oh good catch. No objection if you want to tack a fix on to this PR, or defer till later - your call.

@grondo
Copy link
Contributor

grondo commented Jul 8, 2016

Ooh, yeah @dongahn can you go ahead and fix that? Sloppy conversion on my part from the old approxidate functions I guess (or maybe it was that way all along, in any case that is a good fix)

@coveralls
Copy link

coveralls commented Jul 8, 2016

Coverage Status

Changes Unknown when pulling 7f5331c on dongahn:new_san into * on flux-framework:master*.

@dongahn
Copy link
Member Author

dongahn commented Jul 8, 2016

@garlick: Ok. I think this is ready.

@garlick
Copy link
Member

garlick commented Jul 8, 2016

Thanks!

@garlick garlick merged commit 0a7613e into flux-framework:master Jul 8, 2016
@garlick garlick removed the review label Jul 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants