diff --git a/.arclint b/.arclint index 2a75e37a7..c8c25fbdd 100644 --- a/.arclint +++ b/.arclint @@ -1,85 +1,92 @@ { "linters": { "generated": { "type": "generated" }, "clang-format": { "type": "clang-format", "version": "7.0", "bin": ["clang-format-7", "clang-format"], "include": "(^src/.*\\.(h|c|cpp)$)", "exclude": [ "(^src/(secp256k1|univalue|leveldb)/)" ] }, "autopep8": { "type": "autopep8", "version": ">=1.3.4", "include": "(\\.py$)" }, "flake8": { "type": "flake8", "include": "(\\.py$)", "flags": [ "--select=F401,F403,F405" ] }, "lint-format-strings": { "type": "lint-format-strings", "include": "(^src/.*\\.(h|c|cpp)$)", "exclude": [ "(^src/(secp256k1|univalue|leveldb)/)" ] }, "check-doc": { "type": "check-doc", "include": "(^src/.*\\.(h|c|cpp)$)" }, "lint-tests": { "type": "lint-tests", "include": "(^src/(rpc/|wallet/)?test/.*\\.(cpp)$)" }, "lint-python-format": { "type": "lint-python-format", "include": "(\\.py$)", "exclude": [ "(^test/lint/lint-python-format\\.py$)" ] }, "phpcs": { "type": "phpcs", "include": "(\\.php$)", "exclude": [ "(^arcanist/__phutil_library_.+\\.php$)" ], "phpcs.standard": "arcanist/linter/phpcs_ruleset.xml" }, "lint-locale-dependence": { "type": "lint-locale-dependence", "include": "(^src/.*\\.(h|cpp)$)", "exclude": [ "(^src/(crypto/ctaes/|leveldb/|secp256k1/|seeder/|tinyformat.h|univalue/))" ] }, "lint-cheader": { "type": "lint-cheader", "include": "(^src/.*\\.(h|cpp)$)", "exclude": [ "(^src/(crypto/ctaes|secp256k1|univalue|leveldb)/)" ] }, "spelling": { "type": "spelling", "exclude": [ "(^build-aux/m4/)", "(^depends/)", "(^doc/release-notes/)", "(^src/(qt/locale|secp256k1|univalue|leveldb)/)", "(^test/lint/dictionary/)" ], "spelling.dictionaries": [ "test/lint/dictionary/english.json" ] + }, + "lint-assert-with-side-effects": { + "type": "lint-assert-with-side-effects", + "include": "(^src/.*\\.(h|cpp)$)", + "exclude": [ + "(^src/(secp256k1|univalue|leveldb)/)" + ] } } } diff --git a/arcanist/.phutil_module_cache b/arcanist/.phutil_module_cache index 034f7636f..762d7e16c 100644 --- a/arcanist/.phutil_module_cache +++ b/arcanist/.phutil_module_cache @@ -1 +1 @@ -{"__symbol_cache_version__":11,"90a8b110dc475955f15bb81d37268cb5":{"have":{"class":{"AutoPEP8FormatLinter":75}},"need":{"function":{"pht":297,"execx":769,"id":1903},"class":{"ArcanistExternalLinter":104,"ArcanistLintMessage":1910,"Filesystem":1754,"ArcanistLinter":2017,"ArcanistLintSeverity":2095}},"xmap":{"AutoPEP8FormatLinter":["ArcanistExternalLinter"]}},"bf0805c02029a7226e8c0d7dee039b3c":{"have":{"class":{"CheckDocLinter":106}},"need":{"function":{"pht":323,"id":1847},"class":{"ArcanistExternalLinter":129,"ArcanistLintMessage":1854,"Filesystem":731,"ArcanistLinter":1902,"ArcanistLintSeverity":1988}},"xmap":{"CheckDocLinter":["ArcanistExternalLinter"]}},"6af7410cfea496ff1d4dcc2624b6b8ea":{"have":{"class":{"ClangFormatLinter":79}},"need":{"function":{"pht":302,"execx":781,"id":1653},"class":{"ArcanistExternalLinter":105,"ArcanistLintMessage":1660,"Filesystem":1504,"ArcanistLinter":1767,"ArcanistLintSeverity":1845}},"xmap":{"ClangFormatLinter":["ArcanistExternalLinter"]}},"9285ad9415f8ebe564f7119e5a72c559":{"have":{"class":{"FormatStringLinter":146}},"need":{"function":{"pht":377,"csprintf":1492,"id":1872},"class":{"ArcanistExternalLinter":173,"ArcanistLintMessage":1879,"Filesystem":827,"ArcanistLinter":1956,"ArcanistLintSeverity":2044}},"xmap":{"FormatStringLinter":["ArcanistExternalLinter"]}},"224d394856b17878058b4c14acb7178b":{"have":{"class":{"LocaleDependenceLinter":160}},"need":{"function":{"pht":5400},"class":{"ArcanistLinter":191,"ArcanistLintSeverity":5903,"Filesystem":6149}},"xmap":{"LocaleDependenceLinter":["ArcanistLinter"]}},"6f2f22dd0f259fb2eaa284b4fab3bc29":{"have":{"class":{"PythonFormatLinter":123}},"need":{"function":{"pht":353,"id":1838},"class":{"ArcanistExternalLinter":150,"ArcanistLintMessage":1845,"Filesystem":776,"ArcanistLinter":1970,"ArcanistLintSeverity":2053}},"xmap":{"PythonFormatLinter":["ArcanistExternalLinter"]}},"25781df78f6eebfb223296b8265e9d19":{"have":{"class":{"TestsLinter":103}},"need":{"function":{"pht":318,"id":2629},"class":{"ArcanistExternalLinter":123,"ArcanistLintMessage":2636,"Filesystem":776,"ArcanistLinter":2684,"ArcanistLintSeverity":2792}},"xmap":{"TestsLinter":["ArcanistExternalLinter"]}},"38f0c676bff5192a344464142caaa253":{"have":{"class":{"CHeaderLinter":99}},"need":{"function":{"pht":611},"class":{"ArcanistLinter":121,"ArcanistLintSeverity":1060,"Filesystem":1307}},"xmap":{"CHeaderLinter":["ArcanistLinter"]}}} \ No newline at end of file +{"__symbol_cache_version__":11,"90a8b110dc475955f15bb81d37268cb5":{"have":{"class":{"AutoPEP8FormatLinter":75}},"need":{"function":{"pht":297,"execx":769,"id":1903},"class":{"ArcanistExternalLinter":104,"ArcanistLintMessage":1910,"Filesystem":1754,"ArcanistLinter":2017,"ArcanistLintSeverity":2095}},"xmap":{"AutoPEP8FormatLinter":["ArcanistExternalLinter"]}},"38f0c676bff5192a344464142caaa253":{"have":{"class":{"CHeaderLinter":99}},"need":{"function":{"pht":611},"class":{"ArcanistLinter":121,"ArcanistLintSeverity":1060,"Filesystem":1307}},"xmap":{"CHeaderLinter":["ArcanistLinter"]}},"a30e4e25376ca05d4ae719915441be9e":{"have":{"class":{"CheckDocLinter":106}},"need":{"function":{"pht":323,"id":1848},"class":{"ArcanistExternalLinter":129,"ArcanistLintMessage":1855,"Filesystem":731,"ArcanistLinter":1903,"ArcanistLintSeverity":1989}},"xmap":{"CheckDocLinter":["ArcanistExternalLinter"]}},"6af7410cfea496ff1d4dcc2624b6b8ea":{"have":{"class":{"ClangFormatLinter":79}},"need":{"function":{"pht":302,"execx":781,"id":1653},"class":{"ArcanistExternalLinter":105,"ArcanistLintMessage":1660,"Filesystem":1504,"ArcanistLinter":1767,"ArcanistLintSeverity":1845}},"xmap":{"ClangFormatLinter":["ArcanistExternalLinter"]}},"9285ad9415f8ebe564f7119e5a72c559":{"have":{"class":{"FormatStringLinter":146}},"need":{"function":{"pht":377,"csprintf":1492,"id":1872},"class":{"ArcanistExternalLinter":173,"ArcanistLintMessage":1879,"Filesystem":827,"ArcanistLinter":1956,"ArcanistLintSeverity":2044}},"xmap":{"FormatStringLinter":["ArcanistExternalLinter"]}},"224d394856b17878058b4c14acb7178b":{"have":{"class":{"LocaleDependenceLinter":160}},"need":{"function":{"pht":5400},"class":{"ArcanistLinter":191,"ArcanistLintSeverity":5903,"Filesystem":6149}},"xmap":{"LocaleDependenceLinter":["ArcanistLinter"]}},"6f2f22dd0f259fb2eaa284b4fab3bc29":{"have":{"class":{"PythonFormatLinter":123}},"need":{"function":{"pht":353,"id":1838},"class":{"ArcanistExternalLinter":150,"ArcanistLintMessage":1845,"Filesystem":776,"ArcanistLinter":1970,"ArcanistLintSeverity":2053}},"xmap":{"PythonFormatLinter":["ArcanistExternalLinter"]}},"25781df78f6eebfb223296b8265e9d19":{"have":{"class":{"TestsLinter":103}},"need":{"function":{"pht":318,"id":2629},"class":{"ArcanistExternalLinter":123,"ArcanistLintMessage":2636,"Filesystem":776,"ArcanistLinter":2684,"ArcanistLintSeverity":2792}},"xmap":{"TestsLinter":["ArcanistExternalLinter"]}},"2809b09d2021203b43c57da33d1fe8bf":{"have":{"class":{"AssertWithSideEffectsLinter":210}},"need":{"function":{"pht":439},"class":{"ArcanistLinter":246,"ArcanistLintSeverity":926,"Filesystem":1170}},"xmap":{"AssertWithSideEffectsLinter":["ArcanistLinter"]}}} \ No newline at end of file diff --git a/arcanist/__phutil_library_map__.php b/arcanist/__phutil_library_map__.php index bd41fa60e..4e4de828c 100644 --- a/arcanist/__phutil_library_map__.php +++ b/arcanist/__phutil_library_map__.php @@ -1,32 +1,34 @@ 2, 'class' => array( + 'AssertWithSideEffectsLinter' => 'linter/AssertWithSideEffectsLinter.php', 'AutoPEP8FormatLinter' => 'linter/AutoPEP8Linter.php', 'CHeaderLinter' => 'linter/CHeaderLinter.php', 'CheckDocLinter' => 'linter/CheckDocLinter.php', 'ClangFormatLinter' => 'linter/ClangFormatLinter.php', 'FormatStringLinter' => 'linter/FormatStringLinter.php', 'LocaleDependenceLinter' => 'linter/LocaleDependenceLinter.php', 'PythonFormatLinter' => 'linter/PythonFormatLinter.php', 'TestsLinter' => 'linter/TestsLinter.php', ), 'function' => array(), 'xmap' => array( + 'AssertWithSideEffectsLinter' => 'ArcanistLinter', 'AutoPEP8FormatLinter' => 'ArcanistExternalLinter', 'CHeaderLinter' => 'ArcanistLinter', 'CheckDocLinter' => 'ArcanistExternalLinter', 'ClangFormatLinter' => 'ArcanistExternalLinter', 'FormatStringLinter' => 'ArcanistExternalLinter', 'LocaleDependenceLinter' => 'ArcanistLinter', 'PythonFormatLinter' => 'ArcanistExternalLinter', 'TestsLinter' => 'ArcanistExternalLinter', ), )); diff --git a/arcanist/linter/AssertWithSideEffectsLinter.php b/arcanist/linter/AssertWithSideEffectsLinter.php new file mode 100644 index 000000000..e521a3ed3 --- /dev/null +++ b/arcanist/linter/AssertWithSideEffectsLinter.php @@ -0,0 +1,70 @@ + ArcanistLintSeverity::SEVERITY_ERROR, + ); + } + + public function getLintNameMap() { + return array( + self::ASSERT_SIDE_EFFECTS_FOUND => pht('Assertion has side-effects.'), + ); + } + + public function lintPath($path) { + $path = Filesystem::resolvePath($path, $this->getProjectRoot()); + $fileContent = Filesystem::readFile($path); + + if (!preg_match_all("/[^_]assert\(.*(\+\+|\-\-|[^=!<>]=[^=!<>]).*\);/", + $fileContent, $matches, PREG_OFFSET_CAPTURE)) { + return; + } + + foreach ($matches[1] as $match) { + list($sideEffect, $offset) = $match; + + $this->raiseLintAtOffset( + $offset, + self::ASSERT_SIDE_EFFECTS_FOUND, + pht('Having side-effects in assertions is unexpected and makes the '. + 'code harder to understand. From PRE31-C (SEI CERT C Coding '. + 'Standard): "Assertions should not contain assignments, '. + 'increment, or decrement operators."'), + $sideEffect, + null); + } + } +} diff --git a/src/test/scheduler_tests.cpp b/src/test/scheduler_tests.cpp index fc9b125f1..fd700a02f 100644 --- a/src/test/scheduler_tests.cpp +++ b/src/test/scheduler_tests.cpp @@ -1,218 +1,224 @@ // Copyright (c) 2012-2016 The Bitcoin Core developers // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include #include #include #include #include #include #include #include #include BOOST_AUTO_TEST_SUITE(scheduler_tests) static void microTask(CScheduler &s, boost::mutex &mutex, int &counter, int delta, boost::chrono::system_clock::time_point rescheduleTime) { { boost::unique_lock lock(mutex); counter += delta; } boost::chrono::system_clock::time_point noTime = boost::chrono::system_clock::time_point::min(); if (rescheduleTime != noTime) { CScheduler::Function f = boost::bind(µTask, std::ref(s), std::ref(mutex), std::ref(counter), -delta + 1, noTime); s.schedule(f, rescheduleTime); } } static void MicroSleep(uint64_t n) { boost::this_thread::sleep_for(boost::chrono::microseconds(n)); } BOOST_AUTO_TEST_CASE(manythreads) { // Stress test: hundreds of microsecond-scheduled tasks, // serviced by 10 threads. // // So... ten shared counters, which if all the tasks execute // properly will sum to the number of tasks done. // Each task adds or subtracts a random amount from one of the // counters, and then schedules another task 0-1000 // microseconds in the future to subtract or add from // the counter -random_amount+1, so in the end the shared // counters should sum to the number of initial tasks performed. CScheduler microTasks; boost::mutex counterMutex[10]; int counter[10] = {0}; boost::random::mt19937 rng(42); boost::random::uniform_int_distribution<> zeroToNine(0, 9); boost::random::uniform_int_distribution<> randomMsec(-11, 1000); boost::random::uniform_int_distribution<> randomDelta(-1000, 1000); boost::chrono::system_clock::time_point start = boost::chrono::system_clock::now(); boost::chrono::system_clock::time_point now = start; boost::chrono::system_clock::time_point first, last; size_t nTasks = microTasks.getQueueInfo(first, last); BOOST_CHECK(nTasks == 0); for (int i = 0; i < 100; ++i) { boost::chrono::system_clock::time_point t = now + boost::chrono::microseconds(randomMsec(rng)); boost::chrono::system_clock::time_point tReschedule = now + boost::chrono::microseconds(500 + randomMsec(rng)); int whichCounter = zeroToNine(rng); CScheduler::Function f = boost::bind( µTask, std::ref(microTasks), std::ref(counterMutex[whichCounter]), std::ref(counter[whichCounter]), randomDelta(rng), tReschedule); microTasks.schedule(f, t); } nTasks = microTasks.getQueueInfo(first, last); BOOST_CHECK(nTasks == 100); BOOST_CHECK(first < last); BOOST_CHECK(last > now); // As soon as these are created they will start running and servicing the // queue boost::thread_group microThreads; for (int i = 0; i < 5; i++) { microThreads.create_thread( boost::bind(&CScheduler::serviceQueue, µTasks)); } MicroSleep(600); now = boost::chrono::system_clock::now(); // More threads and more tasks: for (int i = 0; i < 5; i++) { microThreads.create_thread( boost::bind(&CScheduler::serviceQueue, µTasks)); } for (int i = 0; i < 100; i++) { boost::chrono::system_clock::time_point t = now + boost::chrono::microseconds(randomMsec(rng)); boost::chrono::system_clock::time_point tReschedule = now + boost::chrono::microseconds(500 + randomMsec(rng)); int whichCounter = zeroToNine(rng); CScheduler::Function f = boost::bind( µTask, std::ref(microTasks), std::ref(counterMutex[whichCounter]), std::ref(counter[whichCounter]), randomDelta(rng), tReschedule); microTasks.schedule(f, t); } // Drain the task queue then exit threads microTasks.stop(true); // ... wait until all the threads are done microThreads.join_all(); int counterSum = 0; for (int i = 0; i < 10; i++) { BOOST_CHECK(counter[i] != 0); counterSum += counter[i]; } BOOST_CHECK_EQUAL(counterSum, 200); } BOOST_AUTO_TEST_CASE(schedule_every) { CScheduler scheduler; boost::condition_variable cvar; std::atomic counter{15}; std::atomic keepRunning{true}; scheduler.scheduleEvery( [&keepRunning, &cvar, &counter, &scheduler]() { BOOST_CHECK(counter > 0); cvar.notify_all(); if (--counter > 0) { return true; } // We reached the end of our test, make sure nothing run again for // 100ms. scheduler.scheduleFromNow( [&keepRunning, &cvar]() { keepRunning = false; cvar.notify_all(); }, 100); // We set the counter to some magic value to check the scheduler // empty its queue properly after 120ms. scheduler.scheduleFromNow([&counter]() { counter = 42; }, 120); return false; }, 5); // Start the scheduler thread. std::thread schedulerThread( std::bind(&CScheduler::serviceQueue, &scheduler)); boost::mutex mutex; boost::unique_lock lock(mutex); while (keepRunning) { cvar.wait(lock); BOOST_CHECK(counter >= 0); } BOOST_CHECK_EQUAL(counter, 0); scheduler.stop(true); schedulerThread.join(); BOOST_CHECK_EQUAL(counter, 42); } BOOST_AUTO_TEST_CASE(singlethreadedscheduler_ordered) { CScheduler scheduler; // each queue should be well ordered with respect to itself but not other // queues SingleThreadedSchedulerClient queue1(&scheduler); SingleThreadedSchedulerClient queue2(&scheduler); // create more threads than queues // if the queues only permit execution of one task at once then // the extra threads should effectively be doing nothing // if they don't we'll get out of order behaviour boost::thread_group threads; for (int i = 0; i < 5; ++i) { threads.create_thread( boost::bind(&CScheduler::serviceQueue, &scheduler)); } // these are not atomic, if SinglethreadedSchedulerClient prevents // parallel execution at the queue level no synchronization should be // required here int counter1 = 0; int counter2 = 0; // just simply count up on each queue - if execution is properly ordered // then the callbacks should run in exactly the order in which they were // enqueued for (int i = 0; i < 100; ++i) { - queue1.AddToProcessQueue([i, &counter1]() { assert(i == counter1++); }); - - queue2.AddToProcessQueue([i, &counter2]() { assert(i == counter2++); }); + queue1.AddToProcessQueue([i, &counter1]() { + bool expectation = i == counter1++; + assert(expectation); + }); + + queue2.AddToProcessQueue([i, &counter2]() { + bool expectation = i == counter2++; + assert(expectation); + }); } // finish up scheduler.stop(true); threads.join_all(); BOOST_CHECK_EQUAL(counter1, 100); BOOST_CHECK_EQUAL(counter2, 100); } BOOST_AUTO_TEST_SUITE_END()