Skip to content

Commit

Permalink
SHOPT_OPTIMIZE: Do not optimize namerefs in for loops
Browse files Browse the repository at this point in the history
Reproducer:

    one=1 bar=2 baz=3
    nameref vv
    for vv in one bar baz
    do      print -n -- "$vv"
    done

Expected output: 123
Actual output:   111

This is another bug in the loop invariants optimiser. Research on
the abandoned AT&T betas shows this bug was fixed in ksh 93v-
2013-02-13 (tagged as 2013-02-14 in the ksh93-history repo).

The 93v- fix involves the sh_exec() TFOR code in xec.c temporarily
setting the NV_TABLE attribute for namerefs, then adding a check
for it in nv_create() so optimisation is disabled for namerefs.

This commit backports that fix. For code legibility's sake, in this
commit I'm aliasing NV_TABLE to NV_NOOPTIMIZE. This also allows us
to define it as 0 when SHOPT_OPTIMIZE is disabled at compile time,
so the compiler optimises out the new code.

(It makes sense to (ab)use NV_TABLE for this purpose as it is never
normally set for namerefs, plus, tables (as in .foo.bar-style
variables, name starting with dot) aren't optimised either.)

Thanks to @ormaaj for the bug report.
Resolves: #704
  • Loading branch information
McDutchie committed Jan 22, 2024
1 parent 8342cb7 commit 48445fa
Show file tree
Hide file tree
Showing 7 changed files with 83 additions and 3 deletions.
6 changes: 6 additions & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@ This documents significant changes in the 1.0 branch of ksh 93u+m.
For full details, see the git log at: https://github.com/ksh93/ksh/tree/1.0
Uppercase BUG_* IDs are shell bug IDs as used by the Modernish shell library.

2024-01-22:

- Fixed a bug in the loop invariants optimizer (SHOPT_OPTIMIZE) that caused
expansions of name references to be incorrectly treated as invariant so they
yielded the wrong values. Fix backported from from ksh 93v- 2013-02-13.

2024-01-21:

- A bug was fixed that caused both the 'set' and 'get'/'getn' discipline
Expand Down
5 changes: 5 additions & 0 deletions src/cmd/ksh93/include/nval.h
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,11 @@ struct Namval
#define NV_TABLE 0x800 /* node is a dictionary table */
#define NV_IMPORT 0x1000 /* value imported from environment */
#define NV_MINIMAL NV_IMPORT /* node does not contain all fields */
#if SHOPT_OPTIMIZE
#define NV_NOOPTIMIZE NV_TABLE /* disable loop invariants optimizer */
#else
#define NV_NOOPTIMIZE 0
#endif

#define NV_INTEGER 0x2 /* integer attribute */
/* The following attributes are valid only when NV_INTEGER is off */
Expand Down
2 changes: 1 addition & 1 deletion src/cmd/ksh93/include/version.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

#define SH_RELEASE_FORK "93u+m" /* only change if you develop a new ksh93 fork */
#define SH_RELEASE_SVER "1.0.9-beta" /* semantic version number: https://semver.org */
#define SH_RELEASE_DATE "2024-01-21" /* must be in this format for $((.sh.version)) */
#define SH_RELEASE_DATE "2024-01-22" /* must be in this format for $((.sh.version)) */
#define SH_RELEASE_CPYR "(c) 2020-2024 Contributors to ksh " SH_RELEASE_FORK

/* Scripts sometimes field-split ${.sh.version}, so don't change amount of whitespace. */
Expand Down
2 changes: 1 addition & 1 deletion src/cmd/ksh93/sh/name.c
Original file line number Diff line number Diff line change
Expand Up @@ -912,7 +912,7 @@ Namval_t *nv_create(const char *name, Dt_t *root, int flags, Namfun_t *dp)
#if NVCACHE
nvcache.ok = 0;
#endif
if(c=='.') /* don't optimize */
if(nv_isattr(np,NV_NOOPTIMIZE) || c=='.') /* don't optimize */
sh.argaddr = 0;
else if((flags&NV_NOREF) && (c!='[' && *cp!='.'))
{
Expand Down
7 changes: 6 additions & 1 deletion src/cmd/ksh93/sh/xec.c
Original file line number Diff line number Diff line change
Expand Up @@ -2014,12 +2014,15 @@ int sh_exec(const Shnode_t *t, int flags)
}
}
if(nameref)
nv_offattr(np,NV_REF);
nv_offattr(np,NV_REF|NV_NOOPTIMIZE);
else if(nv_isattr(np, NV_ARRAY))
nv_putsub(np,NULL,0L);
nv_putval(np,cp,0);
if(nameref)
{
nv_setref(np,NULL,NV_VARNAME);
nv_onattr(np,NV_NOOPTIMIZE);
}
if(trap=sh.st.trap[SH_DEBUGTRAP])
{
av[0] = (t->tre.tretyp&COMSCAN)?"select":"for";
Expand All @@ -2043,6 +2046,8 @@ int sh_exec(const Shnode_t *t, int flags)
if(sh.st.breakcnt<0)
sh.st.breakcnt++;
}
if(nameref)
nv_offattr(np,NV_NOOPTIMIZE);
#if SHOPT_OPTIMIZE
endfor:
sh_popcontext(buffp);
Expand Down
14 changes: 14 additions & 0 deletions src/cmd/ksh93/tests/nameref.sh
Original file line number Diff line number Diff line change
Expand Up @@ -756,5 +756,19 @@ got=$("$SHELL" -c 'nameref unsetref; unsetref[1]=bar' 2>&1)
"(expected status 1 and match of *$(printf %q "$exp")," \
"got status $e$( ((e>128)) && print -n /SIG && kill -l "$e") and $(printf %q "$got"))"
# ======
# https://github.com/ksh93/ksh/issues/704
# test backported from ksh 93v- 2013-02-14
unset one bar baz arr val vv
one=1 bar=2 baz=3
arr=(one bar baz)
nameref vv
val=$(
for vv in "${arr[@]}"
do print -n -- "$vv"
done
)
[[ $val == 123 ]] || err_exit 'optimization bug with for loops with references'
# ======
exit $((Errors<125?Errors:125))
50 changes: 50 additions & 0 deletions src/cmd/ksh93/tests/types.sh
Original file line number Diff line number Diff line change
Expand Up @@ -795,5 +795,55 @@ e=$?
let "e==0" || err_exit 'crash involving short int as first type member' \
"(got status $e$( ((e>128)) && print -n /SIG && kill -l "$e"), $(printf %q "$got"))"
# ======
# loop invariants optimizer bug
# reproducer by Daniel Douglas
# https://github.com/ksh93/ksh/issues/704
typeset -T Thing=(
integer x
typeset y
)
function f
{
Thing -a t=(
[0]=(x=1; y=hi)
[1]=(x=2; y=yo)
[2]=(x=3; y=moo)
[5]=(x=6; y=boo)
[9]=(x=10; y=boom)
)
typeset -p t
g t
}
function g
{
typeset -n ref=$1 d e
set -- "${!ref[@]}"
set -- "${@/*/ref[\0]}"
for e do
for d in e.x e.y
do printf '%s %s %-14s %s\n' "${@e}" "${!d}" "${@d}" "${d}"
done
done
echo
}
exp='Thing -a t=([0]=(typeset -l -i x=1;y=hi) [1]=(typeset -l -i x=2;y=yo) [2]=(typeset -l -i x=3;y=moo) [5]=(typeset -l -i x=6;y=boo) [9]=(typeset -l -i x=10;y=boom))
Thing t[0].x typeset -l -i 1
Thing t[0].y hi
Thing t[1].x typeset -l -i 2
Thing t[1].y yo
Thing t[2].x typeset -l -i 3
Thing t[2].y moo
Thing t[5].x typeset -l -i 6
Thing t[5].y boo
Thing t[9].x typeset -l -i 10
Thing t[9].y boom'
got=$(f 2>&1)
[[ $got == "$exp" ]] ||
{
err_exit "issue 704: expected '-' lines, got '+' lines:"
diff -U1 <(print "$exp") <(print "$got") | sed $'1,3 d; s,^,\t,' >&2
}
# ======
exit $((Errors<125?Errors:125))

0 comments on commit 48445fa

Please sign in to comment.