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

set INTERCEPT_ALL_OBJS in tests when needed #28

Merged
merged 2 commits into from
Jul 3, 2017

Conversation

GBuella
Copy link
Contributor

@GBuella GBuella commented Jun 29, 2017

This allows testing build with the -fsanitize=address flag.
The INTERCEPT_ALL_OBJS is not needed during asm_pattern tests,
is needed during logging tests (as some log messages might pass
through syscall instructions in asan object code outside libc),
and are not needed during the prog_* tests - as those tests
only look at a specific syscall made in the app anyways.


This change is Reviewable

@krzycz
Copy link
Contributor

krzycz commented Jun 29, 2017

:lgtm:


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@GBuella
Copy link
Contributor Author

GBuella commented Jun 29, 2017

With this patch, and the one in pmem/pmdk#2063 (which I'll bring downstream from NVML after being merged there) I managed to successfully execute all tests with clang and ASAN.

Ref #7

@codecov-io
Copy link

codecov-io commented Jun 29, 2017

Codecov Report

Merging #28 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #28   +/-   ##
=======================================
  Coverage   56.93%   56.93%           
=======================================
  Files          18       18           
  Lines        1486     1486           
  Branches      413      413           
=======================================
  Hits          846      846           
  Misses        440      440           
  Partials      200      200

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23812ff...f59298d. Read the comment docs.

@GBuella
Copy link
Contributor Author

GBuella commented Jun 29, 2017

Update: I added another commit, for setting another variable.

@GBuella GBuella force-pushed the test_env_set branch 2 times, most recently from 20ec50b to 6b3fe86 Compare June 29, 2017 13:28
@krzycz
Copy link
Contributor

krzycz commented Jun 29, 2017

Reviewed 4 of 4 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

execute_process(COMMAND ${TEST_PROG}
${TEST_PROG_ARG} ${LOG_OUTPUT} ${SECOND_LOG_OUTPUT}
RESULT_VARIABLE HAD_ERROR)
unset(ENV{LD_PRELOAD})
unset(ENV{LD_DEBUG})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this supposed to be here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, I just forgot it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

execute_process(COMMAND ${TEST_PROG} ${TEST_PROG_ARG} ${LOG_OUTPUT}
RESULT_VARIABLE HAD_ERROR)
unset(ENV{LD_PRELOAD})
unset(ENV{LD_DEBUG})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

This allows testing build with the -fsanitize=address flag.
The INTERCEPT_ALL_OBJS is not needed during asm_pattern tests,
is needed during logging tests (as some log messages might pass
through syscall instructions in asan object code outside libc),
and are not needed during the prog_* tests - as those tests
only look at a specific syscall made in the app anyways.
For preload asan runtime when needed.
@marcinslusarz
Copy link
Contributor

:lgtm:


Reviewed 3 of 4 files at r3, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@sarahjelinek
Copy link
Contributor

:lgtm: I am good with this. Just a question. Is there a way to do an 'include' in a CMakeLists.txt file? It seems silly to have to continue to repeat the set the same variable over and over.


Reviewed 3 of 4 files at r3, 1 of 1 files at r4.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@sarahjelinek sarahjelinek merged commit a16a048 into pmem:master Jul 3, 2017
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.

5 participants