Skip to content

Make valgrind run on travis tests of v8js in php7 (Both ZTS and NTS) #249

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

Closed
wants to merge 1 commit into from
Closed

Make valgrind run on travis tests of v8js in php7 (Both ZTS and NTS) #249

wants to merge 1 commit into from

Conversation

TysonAndre
Copy link
Contributor

Redid #248

Valgrind checks for invalid memory accesses and memory leaks.

"-m" makes php unit tests use valgrind (and malloc instead of emalloc, etc.)
(Takes around 20 minutes)

Also support testing v8js with NTS (The default used by Travis is ZTS)
One of the builds will download and install PHP. This will be cached.

For some reason, the memory and time limit tests sometimes fail under valgrind, but valgrind itself doesn't detect any problems. Marking those as disabled for now.

@stesie
Copy link
Member

stesie commented Aug 12, 2016

I think both testing under NTS and testing with valgrind are a good idea. When I still had the Jenkins instance around I actually had a test based on ASAN ...

... so thank you for coming up with this and I'll happily merge this patch once two issues are resolved:

  • run-tests.php doesn't report non-zero exit status if there are (only) leaks
  • your SKIP based on TEST_PHP_ARGS implementation doesn't yet work

The former is generally a problem with run-tests.php and probably debatable (also see https://bugs.php.net/bug.php?id=67177). But as V8Js meanwhile is seemingly free from memory leaks and errors (at least again once we merge #255) I'd vote to fail the build once we accidentally re-introduce one of those errors. Having a swift look at run-tests.php this should be easily patchable at the test doing exit(1); conditionally ...

Your SKIP-test based on TEST_PHP_ARGS doesn't work as run-tests.php cleans the environment. See the travis run of this pull request, the NTS test didn't fail the build, but the ZTS one did -- due to the setMemoryLimit & setTimeLimit tests being run nonetheless. Choosing from the variables that are actually available to the PHP process

stesie@hahnschaaf:~/Projekte/v8js-php7$ tr '\000' '\n' < /proc/29232/environ 
LD_LIBRARY_PATH=/usr/lib/debug
PATH_TRANSLATED=/home/stesie/Projekte/v8js-php7/tests/extensions_basic.php
SCRIPT_FILENAME=/home/stesie/Projekte/v8js-php7/tests/extensions_basic.php
ZEND_DONT_UNLOAD_MODULES=1
TEST_PHP_EXECUTABLE=/usr/bin/php7.0
GLIBCPP_FORCE_NEW=1
REDIRECT_STATUS=1
REQUEST_METHOD=GET
PWD=/home/stesie/Projekte/v8js-php7
USE_ZEND_ALLOC=0
GLIBCXX_FORCE_NEW=1
VALGRIND_LAUNCHER=/usr/bin/valgrind.bin

... I think a test on VALGRIND_LAUNCHER should work reliably.

@TysonAndre
Copy link
Contributor Author

Thanks, will address comments after work this weekend

@stesie
Copy link
Member

stesie commented Aug 13, 2016

You included the ZTS with Valgrind part in your other pull request already, I've just noticed after merging that it reports memory leaks all over then (which it doesn't for me locally). Are you able to reproduce this?

For the moment I've reverted the Valgrind-enabling part as it is included in this patch anyways. Maybe just rebase it onto current master if it'll report leaks also

@TysonAndre
Copy link
Contributor Author

I rebased against master. I still have these problems locally. I'm running v8 5.4.0 candidate 1 on the laptop for tests (422d14350d0678317f77a58b4d418d22818a1db8), should probably use stable instead.

A similar issue seems to be https://bugs.chromium.org/p/v8/issues/detail?id=4122

I can reproduce some leaks and test failures locally when using ZTS (Maybe php and v8js garbage collect in different orders in zts and nts). https://gist.github.com/TysonAndre/b50f8643fafe1000de8a2172ae52ef35

Options:
  --prefix            [/usr/local/php-7.0.9-zts]
  --includes          [-I/usr/local/php-7.0.9-zts/include/php -I/usr/local/php-7.0.9-zts/include/php/main -I/usr/local/php-7.0.9-zts/include/php/TSRM -I/usr/local/php-7.0.9-zts/include/php/Zend -I/usr/local/php-7.0.9-zts/include/php/ext -I/usr/local/php-7.0.9-zts/include/php/ext/date/lib]
  --ldflags           []
  --libs              [-lcrypt   -lresolv -lcrypt -lrt -lm -ldl -lnsl  -lcrypt -lcrypt ]
  --extension-dir     [/usr/local/php-7.0.9-zts/lib/php/extensions/debug-zts-20151012]
  --include-dir       [/usr/local/php-7.0.9-zts/include/php]
  --man-dir           [/usr/local/php-7.0.9-zts/php/man]
  --php-binary        [/usr/local/php-7.0.9-zts/bin/php]
  --php-sapis         [ cli phpdbg cgi]
  --configure-options [--prefix=/usr/local/php-7.0.9-zts --enable-maintainer-zts --disable-all --enable-debug --enable-session --enable-cgi]
  --version           [7.0.9]
  --vernum            [70009]


=====================================================================
TEST RESULT SUMMARY
---------------------------------------------------------------------
Exts skipped    :    0
Exts tested     :    8
---------------------------------------------------------------------

Number of tests :  163               160
Tests skipped   :    3 (  1.8%) --------
Tests warned    :    0 (  0.0%) (  0.0%)
Tests failed    :    0 (  0.0%) (  0.0%)
Expected fail   :    0 (  0.0%) (  0.0%)
Tests leaked    :    8 (  4.9%) (  5.0%)
Tests passed    :  152 ( 93.3%) ( 95.0%)
---------------------------------------------------------------------
Time taken      :  758 seconds
=====================================================================

=====================================================================
FAILED TEST SUMMARY
---------------------------------------------------------------------
Test V8::setMemoryLimit() : Memory limit applied to V8Function calls [tests/set_memory_limit_001.phpt]
Test V8::setMemoryLimit() : Memory limit can be imposed later [tests/set_memory_limit_003.phpt]
Test V8::setTimeLimit() : Time limit can be changed [tests/set_time_limit_002.phpt]
=====================================================================

=====================================================================
LEAKED TEST SUMMARY
---------------------------------------------------------------------
Test V8Js::createSnapshot() : Basic snapshot creation & re-use [tests/create_snapshot_basic.phpt]
Test V8::executeString() : Memory limit [tests/memory_limit.phpt]
Test V8::setMemoryLimit() : Memory limit applied to V8Function calls [tests/set_memory_limit_001.phpt]
Test V8::setMemoryLimit() : Memory limit can be imposed later [tests/set_memory_limit_003.phpt]
Test V8::setMemoryLimit() : Memory limit can be set on V8Js object [tests/set_memory_limit_basic.phpt]
Test V8::setTimeLimit() : Time limit applied to V8Function calls [tests/set_time_limit_001.phpt]
Test V8::setTimeLimit() : Time limit can be changed [tests/set_time_limit_002.phpt]
Test V8::setTimeLimit() : Time limit can be imposed later on [tests/set_time_limit_003.phpt]
=====================================================================

@TysonAndre
Copy link
Contributor Author

TysonAndre commented Mar 5, 2017

Edit: never mind about dist: trusty, got projects mixed up.

This seems like it's mostly working in libv8 5.7 in combination with valgrind. There are PPAs for 5.7 and newer from the same person (installing to /opt/libv8-5.7, etc), could the README (and README.linux) be updated to mention those?

Also, I don't see a maximum libv8 version which phpv8 has been tested with.

@TysonAndre
Copy link
Contributor Author

https://travis-ci.org/TysonAndre/v8js/jobs/207990386

... I think a test on VALGRIND_LAUNCHER should work reliably.

I don't see it if I dump $_ENV to a temporary file in the skipif component. e.g. file_put_contents('/tmp/v8jsenvskipif', var_export($_ENV, true));
Also, note that the environment the skipif code is run under is probably different from the environment the test is run under (I doubt there's a reason to valgrind skipif).

  • Haven't checked the run-tests implementation

However, I do see 'TESTS' => 'tests/memory_limit.phpt -m'

Valgrind checks for invalid memory accesses and memory leaks.

"-m" makes php unit tests use valgrind (and malloc instead of emalloc, etc.)
(Takes around 20 minutes)

Also support testing v8js with NTS (The default used by Travis is ZTS)
One of the builds will download and install PHP. This will be cached.

- This reproduces #247 on travis
- For details on travis build caching, see
  https://docs.travis-ci.com/user/caching/#Pull-request-builds-and-caches

Skip memory and time limit tests - Those are flaky with valgrind.

The tests fail but don't have memory leaks.
(When #248 was tested)

Use 5.7 (earliest ppa >= 5.6) for running valgrind checks.

Fix ppa checks - The installation folder changed to /opt/libv8-5.7

- run tests both with and without valgrind in libv8 5.7
  Otherwise, time limits and memory limits wouldn't be tested at all in 5.7

Always pass strings for splitting environment variables by spaces.
@stesie
Copy link
Member

stesie commented Mar 6, 2017

Hey @TysonAndre,

thanks for continuing working on this issue :-)

We can indeed switch the travis tests to V8 5.7 and 5.8 or so. Generally there shouldn't be a upper bound regarding supported versions; contrary I don't think we need to test with really old versions. After all they don't have security support anyways and hence shouldn't be used. Even so I try not to actively break with old versions, it's not a priority goal IMHO

Regarding https://travis-ci.org/TysonAndre/v8js/jobs/207990386,
it did not skip the test tests/memory_limit.phpt as it should have (according to your code)

@TysonAndre
Copy link
Contributor Author

This PR continues to have problems setting up exceptions for unstable tests. I don't think I'll have time to finish this; feel free to work off of this branch.

@TysonAndre TysonAndre closed this Jun 24, 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.

2 participants