Build out of tree #2183
Closed
Build out of tree #2183
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Autoconf & Automake support builds that aren't in-tree! Tests failed with them though, so that's fixed now. To facilitate testing out-of-tree, Autoconf preset output variables are set for test runners. Also, switch from setting the user-reserved TESTS_ENVRIONMENT to the developer-reserved AM_TESTS_ENVIRONMENT in Makefile.am. JQ_COLORS is now unset before testing, for reproducibility.
Making the C standard happy is a pretty good thing.
Also some setup for Clang tools use. GCC doesn't have an equivalent to Clang's -Wconditional-uninitialized; clang-tidy gets around that. (I used https://github.com/rizsotto/Bear to generate compile_commands.json, and ran with `clang-check --extra-arg=-Wconditional-uninitialized src/(^decNumber)**.c` in Zsh. Turns out `-Wconditional-uninitialized` has a lot of false positives, and we probably wouldn't want it on by default.) Spurred from a tests/mantest Valgrind failure: Conditional jump or move depends on uninitialised value(s) at 0x40A578: checkfail (jq_test.c:50) by 0x40A578: run_jq_tests (jq_test.c:91) by 0x40AFD3: jq_testsuite (jq_test.c:34) by 0x405360: main (main.c:545) Conditional jump or move depends on uninitialised value(s) at 0x40A5A1: checkfail (jq_test.c:50) by 0x40A5A1: run_jq_tests (jq_test.c:91) by 0x40AFD3: jq_testsuite (jq_test.c:34) by 0x405360: main (main.c:545) The offending flow: 1. `static void run_jq_tests(…, FILE *testdata, …) {` 2. ` char prog[4096];` 3. ` while (1) {` 4. ` if (!fgets(prog, sizeof(prog), testdata)) break;` 5. ` if (skipline(prog)) continue;` (This does not modify `prog`.) 6. ` if (checkfail(prog)) {` 7. `static int checkfail(const char *buf) {` 8. ` return strcmp(buf, "%%FAIL\n") == 0 || …` This is the first Valgrind output. 9. ` return … || strcmp(buf, "%%FAIL IGNORE MSG\n") == 0;` This is the second Valgrind output. My hypothesis is that strcmp() is examining characters past EOF. fgets() returns whether it errored or immediately hit EOF, not how many characters it has read, so that's not helpful to us. skipline() stops on an EOF character (NUL byte). Nothing else modifies prog. Computing strlen(prog) earlier and passing it to relevant strncmp() (instead of strcmp()) calls fixes the error. Similar precautions are taken for run_jq_tests()'s `char buf[4096];`.
|
(AppVeyor is experiencing pacman |
|
The main issue of this PR |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Tests weren't happy with a setup like
mkdir -p build && cd build && ../configure && make.make checknow works on out-of-tree-builds.While I was making tests work, one of them failed for me due to Valgrind output, so that's fixed (I think with virtually no runtime costs; the biggest cost would be
strncpy()instead ofstrcpy(), with lengths we already were computing).While I was making that test work, I noticed a function pointer cast error, so that's also received what should be a zero-cost patch (
always_inlined function that exists solely to cast a parameter type and tail call the actual function).Another test failed due to my normal environment exporting
JQ_COLORS, so that's now unset before all tests that try to use it.This change is