Date: Tue, 05 Jul 2011 09:34:37 +0200 From: Stefan Esser <se@freebsd.org> To: Jilles Tjoelker <jilles@stack.nl> Cc: Garrett Wollman <wollman@csail.mit.edu>, freebsd-standards@freebsd.org Subject: Re: New patches for expr Message-ID: <4E12BE8D.7090206@freebsd.org> In-Reply-To: <20110704214917.GA77218@stack.nl> References: <4E09AF8E.5010509@freebsd.org> <4E0B860E.50504@freebsd.org> <20110630164653.GA82980@zim.MIT.EDU> <4E0CACFB.8040906@freebsd.org> <19980.47094.184089.398324@khavrinen.csail.mit.edu> <20110701092909.F1164@besplex.bde.org> <4E0E2535.1060204@freebsd.org> <20110704214917.GA77218@stack.nl>
next in thread | previous in thread | raw e-mail | index | archive | help
Am 04.07.2011 23:49, schrieb Jilles Tjoelker:
> On Fri, Jul 01, 2011 at 09:51:17PM +0200, Stefan Esser wrote:
>> The first file (expr.y.diff1) contains a "diff -w -u3" of the functional
>> changes that I suggest. It is only meant to show the actual code changes
>> (applying this diff results in mis-indented code).
Hi Jilles,
thank you for your comments!
> On amd64, INT_MIN % -1 also causes a divide error SIGFPE. Therefore,
> this check should not be skipped, but perhaps the result 0 should be
> manually put in (in recent sh(1) both $((INT_MIN / -1)) and $((INT_MIN %
> -1)) are errors).
Guess, you are replying to a version of the diffs, which have evolved
due to other comments I had received. The first patch will move the test
for INT_MIN / -1 up a line to be performed before the division (for both
/ and %).
> The overflow checks assert_plus, assert_minus and assert_times are not
> standard-compliant because they first let the overflow occur and then
> attempt to check for it. The C standard permits an optimizing compiler
> to remove the checks (or anything else, as the meaning of the entire
> program is destroyed if there is overflow, but given that the program
> must still work if there is no overflow, it is pretty hard to avoid
> generating the proper add, subtract or multiply instructions).
Ah, I see your point: A conforming compiler could know, that the
overflow check could only be triggered in the case of prior overflow
(and indeed, that's just the intended purpose), and the compiler could
then declare the outcome of the operation either undefined (in which
case no overflow check instructions need to be generated) or valid (i.e.
no overflow has occurred). In either case, the overflow check is
formally not be required to be in the generated code.
Well, if a compiler is free to not emit code for the overflow case, then
it will still not hurt to add that code for all those compilers, that do
not bother and just generate the comparisons and branch.
And, as you say, it is not a common thing to do for compilers. Adding
checks that do not rely on a previous arithmetic overflow might be
possible, but as long as all relevant compilers generate the expected
code, I really don't care.
> Note that the checks were already wrong, so as long as you verify you do
> not trigger new unexpected "optimizations", it should be OK to commit
> regardless.
Ok. Similar (but not as stricts) have been in the NetBSD version since
2001. IIRC, they were added after I initially made FreeBSD expr suppport
64bit and range checks, but I'd have to check in the NetBSD repository
to be sure. Anyway, they don't seem to have hurt the NetBSD project in
the past 10 years or they had been removed from their code.
> Sadly, C makes it really hard to check this correctly, while the CPU
> (amd64 at least) provides a simple flag bit.
Yes, but as you know, other CPUs do not offer such a flag (e.g. MIPS, to
simplify the design and minimize operation latency) and C cannot assume
such a feature exists.
>> I also include a SHAR of regression tests meant to be committed to
>> /usr/src/tools/regression/bin/expr. All tests pass with the current
>> patched version of expr (while the currently shipped version dumps core
>> in one test case).
>
> The tests still have "test" instead of "expr" at some places, see below.
Thanks. I used the regression test for /bin/test as a template. I'll fix
them in the committed version.
>> # This is a shell archive. Save it in a file, remove anything before
>> # this line, and then unpack it by entering "sh file". Note, it may
>> # create directories; files and directories will be owned by you and
>> # have default permissions.
>> #
>> # This archive contains:
>> #
>> # expr/Makefile
>> # expr/regress.sh
>> # expr/regress.t
>> #
>> echo x - expr/Makefile
>> sed 's/^X//' >expr/Makefile << 'f9600fcce6c6ed7d48045c253fb4f463'
>> X# $FreeBSD: head/tools/regression/bin/test/Makefile 215022 2010-11-08 23:15:10Z jilles $
>> X
>> Xall:
>> X sh regress.sh
>> f9600fcce6c6ed7d48045c253fb4f463
>> echo x - expr/regress.sh
>> sed 's/^X//' >expr/regress.sh << '78ebeeef8ff008d708ef3dfc31f2d4b1'
>> X#!/bin/sh
>> X
>> [snip]
>> X
>> X#
>> X# expr.sh - check if test(1) or builtin test works
Ok.
> test -> expr
>
>> X#
>> X# $FreeBSD:$
>> X
>> X# force a specified test program, e.g. `env test=/bin/expr sh regress.sh'
>> X: ${test=expr}
>
> test -> expr
Well, I read this in the way, that the program to test (i.e. DOT) was
expr. But I can change this, since I already wondered whether that
interpretation was right.
>> X
>> Xt ()
>> X{
>> X # $1 -> exit code
>> X # $2 -> result written to stdout
>> X # $3 -> $test expression
>> X
>> X count=$((count+1))
>> X # check for syntax errors
>
> Does this comment still mean something?
This comment is not in the version I sent to the list, yesterday.
>> X output="`eval $test $3 2>/dev/null`"
>> X ret=$?
>> X if [ "$ret" -ne "$1" ]; then
>> X printf "not ok %s - (got $ret, expected $1)\n" "$count $3"
>> X elif [ "$output" != "$2" ]; then
>> X printf "not ok %s - (unexpected output)\n" "$count $3"
>> X else
>> X printf "ok %s\n" "$count $3"
>> X fi
>> X}
>> X
>> [snip]
>
BTW: I intend to add a few more test cases, some for too large decimal
arguments in comparison operators (to test whether string or numeric
comparison is done) and some for left-/right-association of operators.
But else (with the changes you point out fixed), I think the latest
version is ready to be committed.
It might make sense to have a similar regression test script for shell
arithmetic, BTW. If you are not interested in converting the relevant
test cases that might be moved over from expr/regress.sh, then I might
do this for a start. Operations and operand types not covered could then
be added in a second step.
Best regards, STefan
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4E12BE8D.7090206>