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>