From fdd8792ebc1acd3fe2b7b9dabdba742475860312 Mon Sep 17 00:00:00 2001 From: Phil Turnbull Date: Wed, 14 Jun 2017 09:52:55 -0400 Subject: [PATCH 01/13] Correctly set is_first From scan-build: Value stored to 'is_first' is never read --- src/compiler/build_tables/build_parse_table.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/compiler/build_tables/build_parse_table.cc b/src/compiler/build_tables/build_parse_table.cc index 50c84af7..ca9405a0 100644 --- a/src/compiler/build_tables/build_parse_table.cc +++ b/src/compiler/build_tables/build_parse_table.cc @@ -646,8 +646,8 @@ class ParseTableBuilder { if (considered_associativity) { description += " " + to_string(resolution_count++) + ": "; description += "Specify a left or right associativity in"; + bool is_first = true; for (const ParseAction &action : entry.actions) { - bool is_first = true; if (action.type == ParseActionTypeReduce) { if (!is_first) description += " and"; description += " `" + symbol_name(action.symbol) + "`"; From da099d0bbe8b1ca9eabfadac8f7fb8e16bae1ff2 Mon Sep 17 00:00:00 2001 From: Phil Turnbull Date: Wed, 14 Jun 2017 10:55:04 -0400 Subject: [PATCH 02/13] Prevent NULL pointer dereference in parser__repair_error_callback Because repair_reduction_count is unsigned, the default of '-1' is 0xffffffff and will cause the loop to be entered if repair_reduction_count is NULL: src/runtime/parser.c:691:11: warning: Dereference of null pointer if (repair_reductions[j].params.symbol == repair->symbol) { ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ --- src/runtime/parser.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/runtime/parser.c b/src/runtime/parser.c index ef9cd31c..a7198778 100644 --- a/src/runtime/parser.c +++ b/src/runtime/parser.c @@ -656,7 +656,7 @@ static StackIterateAction parser__repair_error_callback( StackIterateAction result = StackIterateNone; uint32_t last_repair_count = -1; - uint32_t repair_reduction_count = -1; + uint32_t repair_reduction_count = 0; const TSParseAction *repair_reductions = NULL; for (uint32_t i = 0; i < repairs->size; i++) { From d1b19e81964df7a19c630787448ac2dc8f710012 Mon Sep 17 00:00:00 2001 From: Phil Turnbull Date: Wed, 14 Jun 2017 10:58:34 -0400 Subject: [PATCH 03/13] Prevent NULL pointer dereference in parser__accept parser__select_tree can return true if 'left != NULL' and 'right == NULL' which will later cause a NULL ptr deref: src/runtime/parser.c:842:14: warning: Access to field 'ref_count' results in a dereference of a null pointer (loaded from variable 'root') assert(root->ref_count > 0); ^~~~~~~~~~~~~~~ --- src/runtime/parser.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/runtime/parser.c b/src/runtime/parser.c index a7198778..b54b8250 100644 --- a/src/runtime/parser.c +++ b/src/runtime/parser.c @@ -839,7 +839,7 @@ static void parser__accept(Parser *self, StackVersion version, if (parser__select_tree(self, self->finished_tree, root)) { ts_tree_release(self->finished_tree); - assert(root->ref_count > 0); + assert((root == NULL) || (root->ref_count > 0)); self->finished_tree = root; } else { ts_tree_release(root); From aa6e93820cb1dd252c7cf4380f9b326e58e89df6 Mon Sep 17 00:00:00 2001 From: Phil Turnbull Date: Wed, 14 Jun 2017 11:02:59 -0400 Subject: [PATCH 04/13] Silence false-positive warning in ts_record_free This is safe but I think it is technically undefined behaviour to use a pointer after it has been freed: test/helpers/record_alloc.cc:75:3: warning: Use of memory after it is freed record_deallocation(pointer); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ --- test/helpers/record_alloc.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/helpers/record_alloc.cc b/test/helpers/record_alloc.cc index fca5ec02..f7cd5950 100644 --- a/test/helpers/record_alloc.cc +++ b/test/helpers/record_alloc.cc @@ -71,8 +71,8 @@ void *ts_record_calloc(size_t count, size_t size) { } void ts_record_free(void *pointer) { - free(pointer); record_deallocation(pointer); + free(pointer); } bool ts_record_allocations_toggle(bool value) { From 18f261ad512b16a29332e89a784a6939e2a1148e Mon Sep 17 00:00:00 2001 From: Phil Turnbull Date: Wed, 14 Jun 2017 11:08:09 -0400 Subject: [PATCH 05/13] Initialise all fields of TSParseOptions in tests This should prevent any confusing failures in the unit tests: test/runtime/document_test.cc:381:7: warning: Passed-by-value struct argument contains uninitialized data (e.g., field: 'changed_range_count') ts_document_parse_with_options(document, options); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ test/runtime/document_test.cc:408:7: warning: Passed-by-value struct argument contains uninitialized data (e.g., field: 'changed_range_count') ts_document_parse_with_options(document, options); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ --- test/runtime/document_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/runtime/document_test.cc b/test/runtime/document_test.cc index 7757823c..0fd2e65f 100644 --- a/test/runtime/document_test.cc +++ b/test/runtime/document_test.cc @@ -374,7 +374,7 @@ describe("Document", [&]() { ts_document_set_language(document, load_real_language("json")); ts_document_set_input_string(document, input_string.c_str()); - TSParseOptions options; + TSParseOptions options = {}; options.changed_ranges = nullptr; options.halt_on_error = false; @@ -402,7 +402,7 @@ describe("Document", [&]() { ts_document_set_language(document, load_real_language("json")); ts_document_set_input_string(document, input_string.c_str()); - TSParseOptions options; + TSParseOptions options = {}; options.changed_ranges = nullptr; options.halt_on_error = true; ts_document_parse_with_options(document, options); From cfca764d48d5d5a342cd9eefe2557970f09094a2 Mon Sep 17 00:00:00 2001 From: Phil Turnbull Date: Thu, 15 Jun 2017 07:47:16 -0400 Subject: [PATCH 06/13] Root can never be NULL in this context --- src/runtime/parser.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/runtime/parser.c b/src/runtime/parser.c index b54b8250..3026f76f 100644 --- a/src/runtime/parser.c +++ b/src/runtime/parser.c @@ -839,7 +839,7 @@ static void parser__accept(Parser *self, StackVersion version, if (parser__select_tree(self, self->finished_tree, root)) { ts_tree_release(self->finished_tree); - assert((root == NULL) || (root->ref_count > 0)); + assert(root && root->ref_count > 0); self->finished_tree = root; } else { ts_tree_release(root); From 97cdd8b7381656f4ffafc79eb076b32f5809d85b Mon Sep 17 00:00:00 2001 From: Phil Turnbull Date: Wed, 14 Jun 2017 13:28:03 -0400 Subject: [PATCH 07/13] Run scan-build during CI This bumps the travis-ci container image to Trusty so that we have a version of clang that includes proper support for C++14. --- .travis.yml | 4 +++- script/ci | 5 ++++- script/lib.sh | 26 ++++++++++++++++++++++++++ script/test | 17 +++++++++++++++-- 4 files changed, 48 insertions(+), 4 deletions(-) create mode 100755 script/lib.sh diff --git a/.travis.yml b/.travis.yml index 5a8e2be9..e8789873 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,4 +1,5 @@ sudo: false +dist: trusty language: cpp compiler: - gcc @@ -9,10 +10,11 @@ addons: - ubuntu-toolchain-r-test packages: - g++-5 + - clang install: - export CXX="g++-5" -- script/configure +- scan-build script/configure script: - script/ci diff --git a/script/ci b/script/ci index 70035525..64690944 100755 --- a/script/ci +++ b/script/ci @@ -2,6 +2,9 @@ set -e +. script/lib.sh + script/fetch-fixtures script/check-mallocs -script/test +scan_build make +script/test -b diff --git a/script/lib.sh b/script/lib.sh new file mode 100755 index 00000000..786fc51d --- /dev/null +++ b/script/lib.sh @@ -0,0 +1,26 @@ +#!/usr/bin/env bash + +function scan_build { + extra_args=() + + # AFAICT, in the trusty travis container the scan-build tool is from the 3.4 + # installation. Therefore, by default it will use clang-3.4 when analysing code + # which doesn't support the '-std=c++14' (it is available via '-std=c++1y'). + # Use the system-wide installed clang instead which is 3.5 and does support + # '-std=c++14'. + extra_args+=("--use-analyzer=$(which clang)") + + # scan-build will try to guess which CXX should be used to compile the actual + # code, which is usually g++ but we need g++5 in the CI. Explicitly pass + # $CC/$CXX to scan-build if they are set in the environment. + + if [[ ! -z "$CC" ]]; then + extra_args+=("--use-cc=$CC") + fi + + if [[ ! -z "$CXX" ]]; then + extra_args+=("--use-c++=$CXX") + fi + + scan-build "${extra_args[@]}" --status-bugs "$@" +} diff --git a/script/test b/script/test index 39f21793..35c7ec33 100755 --- a/script/test +++ b/script/test @@ -2,6 +2,8 @@ set -e +. script/lib.sh + function usage { cat <<-EOF USAGE @@ -12,6 +14,8 @@ OPTIONS -h print this message + -b run make under scan-build static analyzer + -d run tests in a debugger (either lldb or gdb) -g run tests with valgrind's memcheck tool @@ -26,6 +30,7 @@ OPTIONS -z pipe tests' stderr to \`dot(1)\` to render an SVG log + EOF } @@ -37,8 +42,9 @@ args=() target=tests export BUILDTYPE=Test cmd="out/${BUILDTYPE}/${target}" +run_scan_build= -while getopts "df:s:gGhpvS" option; do +while getopts "bdf:s:gGhpvS" option; do case ${option} in h) usage @@ -69,6 +75,9 @@ while getopts "df:s:gGhpvS" option; do S) mode=SVG ;; + b) + run_scan_build=true + ;; esac done @@ -78,7 +87,11 @@ else args+=("--reporter=singleline") fi -make $target +if [[ ! -z "$run_scan_build" ]]; then + scan_build make $target +else + make $target +fi args=${args:-""} if [[ -n $profile ]]; then From 7171664eec2fd5a7edddcefd7ec5c742d02ea0ac Mon Sep 17 00:00:00 2001 From: Phil Turnbull Date: Thu, 15 Jun 2017 17:49:39 -0400 Subject: [PATCH 08/13] Disable DeadStores scan-build checker This silences a true, but minor, bug in the external json-parser: externals/json-parser/json.c:653:37: warning: Value stored to 'b' is never read b = 0; ^ ~ --- script/lib.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/script/lib.sh b/script/lib.sh index 786fc51d..03e24796 100755 --- a/script/lib.sh +++ b/script/lib.sh @@ -22,5 +22,5 @@ function scan_build { extra_args+=("--use-c++=$CXX") fi - scan-build "${extra_args[@]}" --status-bugs "$@" + scan-build "${extra_args[@]}" --status-bugs -disable-checker deadcode.DeadStores "$@" } From 9135d14b81b84a50b2c169a97c528882f512eba6 Mon Sep 17 00:00:00 2001 From: Phil Turnbull Date: Fri, 16 Jun 2017 14:50:48 -0400 Subject: [PATCH 09/13] Add standalone scan-build script For running scan-build outside of CI, e.g. `./script/scan-build -j4` --- script/scan-build | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100755 script/scan-build diff --git a/script/scan-build b/script/scan-build new file mode 100755 index 00000000..c255d674 --- /dev/null +++ b/script/scan-build @@ -0,0 +1,7 @@ +#!/usr/bin/env bash + +set -e + +. script/lib.sh + +scan_build make "$@" From ee3caafe7bdf1e43065150146985b047e9f62903 Mon Sep 17 00:00:00 2001 From: Phil Turnbull Date: Fri, 16 Jun 2017 15:05:14 -0400 Subject: [PATCH 10/13] Use -j2 on the CI boxes The travis-ci trusty container has two cores. --- script/ci | 2 +- script/test | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/script/ci b/script/ci index 64690944..4610a351 100755 --- a/script/ci +++ b/script/ci @@ -6,5 +6,5 @@ set -e script/fetch-fixtures script/check-mallocs -scan_build make +scan_build make -j2 script/test -b diff --git a/script/test b/script/test index 35c7ec33..71f3a89d 100755 --- a/script/test +++ b/script/test @@ -88,9 +88,9 @@ else fi if [[ ! -z "$run_scan_build" ]]; then - scan_build make $target + scan_build make -j2 $target else - make $target + make -j2 $target fi args=${args:-""} From e9bf794cd9e4117279c19a100c508c965ba0799a Mon Sep 17 00:00:00 2001 From: Phil Turnbull Date: Mon, 19 Jun 2017 09:25:16 -0400 Subject: [PATCH 11/13] Remove unneeded build step --- script/ci | 1 - 1 file changed, 1 deletion(-) diff --git a/script/ci b/script/ci index 4610a351..530981ef 100755 --- a/script/ci +++ b/script/ci @@ -6,5 +6,4 @@ set -e script/fetch-fixtures script/check-mallocs -scan_build make -j2 script/test -b From 1b9e78add276e621540269fd8a80d8d6df177ca6 Mon Sep 17 00:00:00 2001 From: Phil Turnbull Date: Mon, 19 Jun 2017 09:26:49 -0400 Subject: [PATCH 12/13] Fix formatting and use '-n' --- script/test | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/script/test b/script/test index 71f3a89d..27aca802 100755 --- a/script/test +++ b/script/test @@ -87,10 +87,10 @@ else args+=("--reporter=singleline") fi -if [[ ! -z "$run_scan_build" ]]; then - scan_build make -j2 $target +if [[ -n "$run_scan_build" ]]; then + scan_build make -j2 $target else - make -j2 $target + make -j2 $target fi args=${args:-""} From 7bdb0917d30ae94c40ef491de33966b6aa417f0a Mon Sep 17 00:00:00 2001 From: Phil Turnbull Date: Mon, 19 Jun 2017 09:27:45 -0400 Subject: [PATCH 13/13] Revert "Add standalone scan-build script" This reverts commit 9135d14b81b84a50b2c169a97c528882f512eba6. --- script/scan-build | 7 ------- 1 file changed, 7 deletions(-) delete mode 100755 script/scan-build diff --git a/script/scan-build b/script/scan-build deleted file mode 100755 index c255d674..00000000 --- a/script/scan-build +++ /dev/null @@ -1,7 +0,0 @@ -#!/usr/bin/env bash - -set -e - -. script/lib.sh - -scan_build make "$@"