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

Fold modulo operator on constant values and raise zero remainder error quickly #2252

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/parser.c

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions src/parser.y
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,7 @@ static block constant_fold(block a, block b, int op) {
case '-': res = jv_number(na - nb); break;
case '*': res = jv_number(na * nb); break;
case '/': res = jv_number(na / nb); break;
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to fix the division by zero here too. Just FYI.

case '%': res = jv_number((intmax_t)nb == 0 ? INFINITY : (intmax_t)na % (intmax_t)nb); break;
Copy link
Contributor

@nicowilliams nicowilliams Jul 27, 2023

Choose a reason for hiding this comment

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

How about this instead:

diff --git a/src/parser.y b/src/parser.y
index 549f6f74..879774b4 100644
--- a/src/parser.y
+++ b/src/parser.y
@@ -230,7 +230,8 @@ static block constant_fold(block a, block b, int op) {
     case '+': res = jv_number(na + nb); break;
     case '-': res = jv_number(na - nb); break;
     case '*': res = jv_number(na * nb); break;
-    case '/': res = jv_number(na / nb); break;
+    case '/': if (nb == 0.0) return gen_noop(); res = jv_number(na / nb); break;
+    case '%': if (nb == 0.0) return gen_noop(); res = jv_number(na % nb); break;
     case EQ:  res = (cmp == 0 ? jv_true() : jv_false()); break;
     case NEQ: res = (cmp != 0 ? jv_true() : jv_false()); break;
     case '<': res = (cmp < 0 ? jv_true() : jv_false()); break;

Copy link
Contributor

Choose a reason for hiding this comment

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

This turns division of a literal by a literal zero into a run-time error.

Copy link
Member

@emanuele6 emanuele6 Jul 27, 2023

Choose a reason for hiding this comment

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

@nicowilliams @itchyny Yes, I say let's do that and stop treating division by zero (expressions that return INFINITY) compile time errors; that was just confusing.
Then let's also revert #2253, and close #2780 since we don't actually want 0/0 to return NaN.
After that the OSS fuzz issue should be fixed, and costant-folding's behaviour will be aligned with f_divide.

@itchyny Since you can't see it, the OSS fuzz issue is https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=60848 if you want to link it in the commit; the reproducer is jq '0/0' (it doesn't trigger UBSAN on my amd64 PC though)

case EQ: res = (cmp == 0 ? jv_true() : jv_false()); break;
case NEQ: res = (cmp != 0 ? jv_true() : jv_false()); break;
case '<': res = (cmp < 0 ? jv_true() : jv_false()); break;
Expand Down
4 changes: 4 additions & 0 deletions tests/jq.test
Original file line number Diff line number Diff line change
Expand Up @@ -1655,6 +1655,10 @@ try (1%.) catch .
1/0
jq: error: Division by zero? at <top-level>, line 1:

%%FAIL
1%0
jq: error: Remainder by zero? at <top-level>, line 1:

# Basic numbers tests: integers, powers of two
[range(-52;52;1)] as $powers | [$powers[]|pow(2;.)|log2|round] == $powers
null
Expand Down
54 changes: 37 additions & 17 deletions tests/shtest
Original file line number Diff line number Diff line change
Expand Up @@ -30,47 +30,67 @@ $VALGRIND $Q $JQ -Rne '[inputs] == ["a\u0000b", "c\u0000d", "e"]' $d/input

# String constant folding (addition only)
nref=$($VALGRIND $Q $JQ -n --debug-dump-disasm '"foo"' | wc -l)
n=$($VALGRIND $Q $JQ -n --debug-dump-disasm '"foo" + "bar"' | wc -l)
if [ $n -ne $nref ]; then
echo "Constant expression folding for strings didn't work"
exit 1
fi

# Numeric constant folding (not all ops yet)
# Numeric constant folding (binary operators)
n=$($VALGRIND $Q $JQ -n --debug-dump-disasm '1+1' | wc -l)
if [ $n -ne $nref ]; then
echo "Constant expression folding for strings didn't work"
exit 1
echo "Constant expression folding for numbers didn't work"
exit 1
fi
n=$($VALGRIND $Q $JQ -n --debug-dump-disasm '1-1' | wc -l)
if [ $n -ne $nref ]; then
echo "Constant expression folding for strings didn't work"
exit 1
echo "Constant expression folding for numbers didn't work"
exit 1
fi
n=$($VALGRIND $Q $JQ -n --debug-dump-disasm '2*3' | wc -l)
if [ $n -ne $nref ]; then
echo "Constant expression folding for strings didn't work"
exit 1
echo "Constant expression folding for numbers didn't work"
exit 1
fi
n=$($VALGRIND $Q $JQ -n --debug-dump-disasm '9/3' | wc -l)
if [ $n -ne $nref ]; then
echo "Constant expression folding for strings didn't work"
exit 1
echo "Constant expression folding for numbers didn't work"
exit 1
fi
n=$($VALGRIND $Q $JQ -n --debug-dump-disasm '9%3' | wc -l)
if [ $n -ne $nref ]; then
echo "Constant expression folding for numbers didn't work"
exit 1
fi
n=$($VALGRIND $Q $JQ -n --debug-dump-disasm '9==3' | wc -l)
if [ $n -ne $nref ]; then
echo "Constant expression folding for strings didn't work"
exit 1
echo "Constant expression folding for numbers didn't work"
exit 1
fi
n=$($VALGRIND $Q $JQ -n --debug-dump-disasm '9!=3' | wc -l)
if [ $n -ne $nref ]; then
echo "Constant expression folding for strings didn't work"
exit 1
echo "Constant expression folding for numbers didn't work"
exit 1
fi
n=$($VALGRIND $Q $JQ -n --debug-dump-disasm '9<3' | wc -l)
if [ $n -ne $nref ]; then
echo "Constant expression folding for numbers didn't work"
exit 1
fi
n=$($VALGRIND $Q $JQ -n --debug-dump-disasm '9>3' | wc -l)
if [ $n -ne $nref ]; then
echo "Constant expression folding for numbers didn't work"
exit 1
fi
n=$($VALGRIND $Q $JQ -n --debug-dump-disasm '9<=3' | wc -l)
if [ $n -ne $nref ]; then
echo "Constant expression folding for strings didn't work"
exit 1
echo "Constant expression folding for numbers didn't work"
exit 1
fi
n=$($VALGRIND $Q $JQ -n --debug-dump-disasm '9>=3' | wc -l)
if [ $n -ne $nref ]; then
echo "Constant expression folding for strings didn't work"
exit 1
echo "Constant expression folding for numbers didn't work"
exit 1
fi

## Test JSON sequence support
Expand Down