-
Notifications
You must be signed in to change notification settings - Fork 31
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
arith: avoid assignment triggering get/getn discipline
An assignment should trigger the 'set' discipline, not the 'get'. Yet, when the assignment is in an arithmetic expression: $ x.set() { echo SET; }; x.get() { echo GET; }; ((x=1)) SET GET That is clearly incorrect, inconsistent and counter to the documentation. For a simple shell assignment 'x=1' instead of '((x=1))', only SET is triggered. The inconsistency also applies to internal shell disciplines. This can be demonstrated using the RANDOM reproducible pseudorandom sequence: $ ksh -c 'RANDOM=123; echo $RANDOM $RANDOM $RANDOM $RANDOM' 29031 19003 403 6924 $ ksh -c '((RANDOM=123)); echo $RANDOM $RANDOM $RANDOM $RANDOM' 19003 403 6924 25974 Note the second output is the same as the first one starting from the second number, meaning RANDOM was incorrectly read from while the arithmetic command was assigning the seed value to it. The second output should be identical to the first. Another reproducer of the same problem: $ (( r = (RANDOM = -1) )); echo $r 2100 The return value r of the "RANDOM = -1" assignment should be -1. No reading from RANDOM should happen here. Analysis: Since shell arithmetic uses C syntax, an assignment also has a return value, i.e., the value assigned to the variable. In the arith() function in arith.c, case ASSIGN, there is a 'r=nv_getnum(np)' call that retrieves this value. However, calling nv_getnum() also triggers a getn/get discipline, which is the bug. Simply replacing this call with "r = n;" (make the return value the assigned value) fixes the bug, but does cause a regression: arith.sh[119]: FAIL: typecast not working in arithmetic evaluation Archaeological research shows that this test was introduced, along with the nv_getnum() call, sometime between 1995 and 1999. The following is most likely the corresponding entry in RELEASE, as the example in it closely matches the arith.sh regression test: 96-07-31 A bug in right to left arithmetic assignment for which the arithmetic expression (( y = x = 1.5 )) did not yield 1 for y when x was declared typeset -i was fixed. In other words, if x is an integer, the return value of the assignment is expected to be typecast to integer before being assigned to y. So that was the purpose of the added nv_getnum() call: it was a lazy way of doing the correct typecast for the variable's type -- unfortunately with side effects. src/cmd/ksh93/include/nval.h: - Add nv_isnum() macro determining if a shell variable has numeric type attributes, and returning their bit mask if so. src/cmd/ksh93/sh/arith.c: arith(): case ASSIGN: - Replace the offending nv_getnum() call with a simple "r = n;" by default, fixing the bug. - To avoid the regression, if nv_isnum() yields attribute bits for the variable, then do "r = n" with a direct typecast to the correct type, depending on the shell variable's attribute bits. src/cmd/ksh93/sh.1: - Rewrite the $RANDOM description to match reality. (re: af6a32d)
- Loading branch information
Showing
8 changed files
with
108 additions
and
12 deletions.
There are no files selected for viewing
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
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
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
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
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
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
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
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
c77999b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same CI failure with this commit that occurs on Linux (I tested locally on Debian/Intel) also occurs on macOS/Intel 12.7.2, but does NOT occur on macOS/M1Pro 12.7.2.
c77999b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I did not test this on other machines before committing, and evidently I should have done. Working on it.
c77999b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found the cause. It's a three more old bugs in ksh -- incorrect typecasts producing undefined results. Commit coming up.