Delete np->dynscope botch; refactor nvflags to uint32_t instead

This commit refactors the local builtin to take advantage of some
changes I intend to submit later as separate pull requests. These are:

- np->nvflags has been expanded from 16-bit to 32-bit. This fixes
  the longstanding limitation detailed in a previous ksh2020 bug:
  att#1038.
  The fix consists of masking out nv_open-centric flags with
  explicit uses of the new NV_OPENMASK, rather than implicitly and
  obtusely abusing the limitations of unsigned short types.
  Here, I take advantage of it for storing NV_DYNAMIC in np->nvflags
  and using it from there, which is a safer and more simple choice
  long term.

- A bugfix from ksh93v- 2012-08-24 has been backported to fix
  a potential segfault that is triggered under ASan when changing
  the sizes of np->nvsize and np->nvflags to uint32_t. This cannot
  be triggered on the dev branch, but I will submit this individually
  soon enough. (I would have already submitted it had I not been
  forced to deal with a broken PSU shortly after submitting ksh93#805;
  0 out of 10 experience don't recommend).

- Updated comment regarding pitfalls in the ksh93v- and current
  implementation of local.

In any case I intend to separate out various fixes from this branch and
submit them as separate pull requests, if only to eventually reduce the
scope of this one to just the local builtin and nothing else (the
sh_funscope refactor for instance fixes an out of bounds bug that has
been on the dev branch for far too long). Unfortunately my life has
been quite hellish lately, so there is no ETA yet.

Author: JohnoKing

src/cmd/ksh93/include/name.h

@@ -87,7 +87,8 @@ struct Ufunction
 
 /* The following attributes are for internal use */
 #define NV_NOCHANGE	(NV_EXPORT|NV_MINIMAL|NV_RDONLY|NV_TAGGED|NV_NOFREE|NV_ARRAY)
-#define NV_ATTRIBUTES	(~(NV_NOSCOPE|NV_ARRAY|NV_NOARRAY|NV_IDENT|NV_ASSIGN|NV_REF|NV_VARNAME|NV_STATIC))
+#define NV_ATTRIBUTES	(NV_ARRAY|NV_IDENT|NV_ASSIGN|NV_REF)
+#define NV_OPENMASK	(NV_APPEND|NV_MOVE|NV_NOARRAY|NV_IARRAY|NV_VARNAME|NV_NOADD|NV_NOSCOPE|NV_NOFAIL|NV_UNATTR|NV_GLOBAL|NV_TYPE|NV_STATIC|NV_COMVAR|NV_STATSCOPE)
 #define NV_PARAM	NV_NODISC	/* expansion use positional params */
 
 /* This following are for use with nodes which are not name-values */

src/cmd/ksh93/include/nval.h

@@ -107,20 +107,8 @@ struct Namval
 {
 	Dtlink_t	nvlink;		/* space for cdt links */
 	char		*nvname;	/* pointer to name of the node */
-#if _ast_sizeof_pointer == 8
-#   if _ast_intswap > 0
-	unsigned short	nvflag; 	/* attributes */
-	unsigned short	dynscope;
-#   else
-	unsigned short	dynscope;
-	unsigned short	nvflag; 	/* attributes */
-#   endif
+	uint32_t	nvflag; 	/* attributes */
 	uint32_t  	nvsize;		/* size or base */
-#else
-	unsigned short	nvflag; 	/* attributes */
-	unsigned short 	nvsize;		/* size or base */
-	unsigned int	dynscope;
-#endif
 	Namfun_t	*nvfun;		/* pointer to trap functions */
 	void		*nvalue;	/* pointer to any kind of value */
 	void		*nvmeta;	/* pointer to any of various kinds of type-dependent data */
@@ -188,8 +176,8 @@ struct Namval
 #define NV_NODISC	NV_IDENT	/* ignore disciplines */
 #define NV_UNATTR	0x800000	/* unset attributes before assignment */
 #define NV_GLOBAL	0x20000000	/* create global variable, ignoring local scope */
-#define NV_DYNAMIC	0x40000000	/* create dynamically scoped variable */
-#define NV_STATSCOPE	0x80000000	/* force creation of statically scoped variable */
+#define NV_STATSCOPE	0x40000000	/* force creation of statically scoped variable */
+#define NV_DYNAMIC	0x80000000	/* create dynamically scoped variable */
 
 #define NV_FUNCT	NV_IDENT	/* option for nv_create */
 #define NV_BLTINOPT	NV_ZFILL	/* mark builtins in libcmd */

src/cmd/ksh93/sh/arith.c

@@ -231,7 +231,7 @@ static Sfdouble_t arith(const char **ptr, struct lval *lvalue, int type, Sfdoubl
 	    case ASSIGN:
 	    {
 		Namval_t *np;
-		unsigned short attr;
+		uint32_t attr;
 		if (lvalue->sub && lvalue->nosub > 0) /* indexed array ARITH_ASSIGNOP */
 		{
 			np = (Namval_t*)lvalue->sub; /* use saved subscript reference instead of last worked value */

src/cmd/ksh93/sh/name.c

@@ -79,7 +79,7 @@ struct sh_type
 		Namval_t	*last_table;
 		Namval_t	*namespace;
 		int		flags;
-		short		size;
+		int32_t		size;
 		short		len;
 	} entries[NVCACHE];
 	short		index;
@@ -443,13 +443,11 @@ void nv_setlist(struct argnod *arg,int flags, Namval_t *typ)
 							nv_putsub(np, NULL, ARRAY_SCAN);
 							if(!ap->fun && !(ap->nelem&ARRAY_TREE) && !np->nvfun->next && !nv_type(np))
 							{
-								unsigned short nvflag = np->nvflag;
+								uint32_t nvflag = np->nvflag;
 								uint32_t nvsize = np->nvsize;
-								uint32_t nvdynscope = np->dynscope;
 								_nv_unset(np,NV_EXPORT);
 								np->nvflag = nvflag;
 								np->nvsize = nvsize;
-								np->dynscope = nvdynscope;
 							}
 							else
 							{
@@ -563,7 +561,6 @@ void nv_setlist(struct argnod *arg,int flags, Namval_t *typ)
 				{
 					L_ARGNOD->nvalue = node.nvalue;
 					L_ARGNOD->nvflag = node.nvflag;
-					L_ARGNOD->dynscope = node.dynscope;
 					L_ARGNOD->nvfun = node.nvfun;
 				}
 				sh.prefix = prefix;
@@ -600,7 +597,6 @@ void nv_setlist(struct argnod *arg,int flags, Namval_t *typ)
 				mp = nv_search(cp,vartree,NV_ADD|NV_NOSCOPE);
 				mp->nvname = np->nvname;  /* put_lang() (init.c) compares nvname pointers */
 				mp->nvflag = np->nvflag;
-				mp->dynscope = np->dynscope;
 				mp->nvsize = np->nvsize;
 				mp->nvfun = nv_cover(np);
 			}
@@ -667,7 +663,6 @@ void nv_setlist(struct argnod *arg,int flags, Namval_t *typ)
 			{
 				L_ARGNOD->nvalue = node.nvalue;
 				L_ARGNOD->nvflag = node.nvflag;
-				L_ARGNOD->dynscope = node.dynscope;
 				L_ARGNOD->nvfun = node.nvfun;
 			}
 		}
@@ -1575,11 +1570,7 @@ Namval_t *nv_open(const char *name, Dt_t *root, int flags)
 			savesub = sub;
 			sh.prefix = prefix;
 		}
-		nv_onattr(np, flags&NV_ATTRIBUTES);
-		if(flags&NV_DYNAMIC)
-			np->dynscope = 1;
-		else
-			np->dynscope = 0;
+		nv_onattr(np, flags&~(NV_ATTRIBUTES|NV_OPENMASK));
 	}
 	else if(c)
 	{
@@ -2201,8 +2192,6 @@ static int scanfilter(Namval_t *np, struct scan *sp)
 		return 0;
 	if(sp->scanmask==NV_TABLE && nv_isvtree(np))
 		np_flags = NV_TABLE;
-	else if(np->dynscope==1)
-		np_flags |= NV_DYNAMIC;
 	if(sp->scanmask?(np_flags&sp->scanmask)==sp->scanflags:(!sp->scanflags || (np_flags&sp->scanflags)))
 	{
 		if(tp && tp->mapname)
@@ -2858,7 +2847,7 @@ void nv_newattr (Namval_t *np, unsigned newatts, int size)
 		else if(size==NV_FLTSIZEZERO)
 			np->nvsize = 0;
 		nv_offattr(np, ~NV_NOFREE);
-		nv_onattr(np, newatts);
+		nv_onattr(np, newatts&~(NV_OPENMASK));
 		return;
 	}
 	if(size==NV_FLTSIZEZERO)
@@ -2878,7 +2867,7 @@ void nv_newattr (Namval_t *np, unsigned newatts, int size)
 		{
 			nv_setsize(np,size);
 			np->nvflag &= NV_ARRAY;
-			np->nvflag |= newatts;
+			np->nvflag |= newatts&~(NV_OPENMASK);
 			goto skip;
 		}
 #endif /* SHOPT_FIXEDARRAY */
@@ -2937,8 +2926,8 @@ void nv_newattr (Namval_t *np, unsigned newatts, int size)
 			_nv_unset(np,NV_EXPORT);
 		nv_setsize(np,size);
 		np->nvflag &= (NV_ARRAY|NV_NOFREE);
-		np->nvflag |= newatts;
-		if (cp)
+		np->nvflag |= newatts&~(NV_OPENMASK);
+		if(cp)
 		{
 			if(!mp)
 				nv_putval (np, cp, NV_RDONLY);

src/cmd/ksh93/sh/nvdisc.c

@@ -894,8 +894,8 @@ int nv_clone(Namval_t *np, Namval_t *mp, int flags)
 {
 	Namfun_t	*fp, *fpnext;
 	const char	*val = mp->nvalue;
-	unsigned short	flag = mp->nvflag;
-	unsigned short	size = mp->nvsize;
+	uint32_t	flag = mp->nvflag;
+	uint32_t	size = mp->nvsize;
 	for(fp=mp->nvfun; fp; fp=fpnext)
 	{
 		fpnext = fp->next;

src/cmd/ksh93/sh/nvtree.c

@@ -404,7 +404,7 @@ void nv_attribute(Namval_t *np,Sfio_t *out,char *prefix,int noname)
 			else if((!np->nvalue||np->nvalue==Empty) && nv_isattr(np,~NV_NOFREE)==NV_MINIMAL && strcmp(np->nvname,"_"))
 				sfputr(out,prefix,' ');
 		}
-		if(np->dynscope)
+		if(nv_isattr(np,NV_DYNAMIC))
 			sfprintf(out,"%s -D ",prefix);
 		return;
 	}
@@ -435,7 +435,7 @@ void nv_attribute(Namval_t *np,Sfio_t *out,char *prefix,int noname)
 		else if(prefix && *prefix)
 		{
 			sfputr(out,prefix,' ');
-			if(np->dynscope)
+			if(nv_isattr(np,NV_DYNAMIC))
 				sfprintf(out,"-D ");
 		}
 		for(tp = shtab_attributes; *tp->sh_name;tp++)

src/cmd/ksh93/sh/nvtype.c

@@ -481,8 +481,8 @@ static Namval_t *create_type(Namval_t *np,const char *name,int flag,Namfun_t *fp
 	n = (cp-1) -name;
 	if(dp->numnodes && dp->strsize<0)
 	{
-		char *base =  (char*)np-sizeof(Dtlink_t);
-		int m=strlen(np->nvname);
+		char *base = (char*)np-(NV_MINSZ-sizeof(Dtlink_t));
+		size_t m = strlen(np->nvname);
 		while((nq=nv_namptr(base,++i)) && strncmp(nq->nvname,np->nvname,m)==0)
 		{
 			if(nq->nvname[m]=='.' && strncmp(name,&nq->nvname[m+1],n)==0 && nq->nvname[m+n+1]==0)

src/cmd/ksh93/sh/xec.c

@@ -3083,19 +3083,21 @@ int sh_funscope(int argn, char *argv[],int(*fun)(void*),void *arg,int execflg)
 	if(dtret)
 	{
 		/*
-		 * Dynamic variables are implemented by being marked via np->dynscope, then
-		 * imported into nested functions via the nv_scan call below with scanflags
-		 * set to NV_DYNAMIC. The separate variable is necessary because np->nvflag
-		 * is an unsigned short and cannot hold the NV_DYNAMIC bitflag. It cannot
-		 * be changed to an unsigned int because the codebase makes too many assumptions
-		 * about its size (cf. https://github.com/att/ast/issues/1038).
+		 * Dynamic variables are implemented by being imported into nested functions
+		 * via the nv_scan call.
 		 *
-		 * The old method ksh93v- uses for its bash mode creates a viewport between
-		 * sh.var_tree and sh.var_base with the help of a sh.st.var_local pointer.
-		 * This could fake dynamic scoping convincingly for the bash mode, but the
-		 * ksh93v- code only manages to create a global scope local to the top level
-		 * function (i.e., nested functions have no scoping at all and their variables
-		 * leak into the calling function.)
+		 * TODO: The old method ksh93v- uses for its bash mode creates a viewport
+		 * between sh.var_tree and sh.var_base with the help of a sh.st.var_local
+		 * pointer. This could fake dynamic scoping convincingly for the bash mode,
+		 * but the ksh93v- code only manages to create a global scope local to the top
+		 * level function (i.e., nested functions have no scoping at all and their
+		 * variables leak into the calling function). That code could to be robustified
+		 * to perhaps create a viewport between the parent function's scope and that of
+		 * the child function, but that runs the risk of breaking static scoping.
+		 * Alternatively, the current method could be altered to simply copy or move
+		 * dynamic local variables from the child function back into the parent function
+		 * when function execution ends; that has its own set of associated risks.
+		 * Cf. https://github.com/ksh93/ksh/pull/703#issuecomment-1898680500
 		 */
 		nv_scan(prevscope->save_tree, local_exports, NULL, 0, NV_EXPORT|NV_DYNAMIC|NV_NOSCOPE);
 	}