Revamp macro arg system

This should significantly improve performance: on pokecrystal builds, perf
reported as much CPU time spent on `yyparse` as on `sym_UseNewMacroArgs`
Measurements show ~6 seconds of improvement on that codebase.

This also fixes #321, as a bonus, due to saner management!

include/asm/fstack.h

@@ -21,12 +21,15 @@
 
 #include "types.h"
 
+struct MacroArgs;
+
 struct sContext {
 	YY_BUFFER_STATE FlexHandle;
 	struct sSymbol *pMacro;
 	struct sContext *pNext;
 	char tzFileName[_MAX_PATH + 1];
-	char *tzMacroArgs[MAXMACROARGS + 1];
+	struct MacroArgs *macroArgs;
+	uint32_t uniqueID;
 	int32_t nLine;
 	uint32_t nStatus;
 	FILE *pFile;
@@ -40,13 +43,12 @@ struct sContext {
 extern unsigned int nMaxRecursionDepth;
 
 void fstk_RunInclude(char *tzFileName);
-void fstk_RunMacroArg(int32_t s);
 void fstk_Init(char *s);
 void fstk_Dump(void);
 void fstk_DumpToStr(char *buf, size_t len);
 void fstk_DumpStringExpansions(void);
 void fstk_AddIncludePath(char *s);
-uint32_t fstk_RunMacro(char *s);
+bool fstk_RunMacro(char *s, struct MacroArgs *args);
 void fstk_RunRept(uint32_t count, int32_t nReptLineNo);
 FILE *fstk_FindFile(char const *fname, char **incPathUsed);
 int32_t fstk_GetLine(void);

include/asm/macro.h

@@ -10,19 +10,25 @@
 #define RGBDS_MACRO_H
 
 #include <stdint.h>
+#include <stdlib.h>
+
+#include "asm/warning.h"
 
 #include "helpers.h"
 
-void macro_AddNewArg(char const *s);
-void macro_SaveCurrentArgs(char *save[]);
-void macro_RestoreCurrentArgs(char *save[]);
-void macro_UseNewArgs(void);
-char *macro_FindArg(int32_t i);
-void macro_UseCurrentArgs(void);
-void macro_SetArgID(uint32_t nMacroCount);
+struct MacroArgs;
+
+struct MacroArgs *macro_GetCurrentArgs(void);
+struct MacroArgs *macro_NewArgs(void);
+void macro_AppendArg(struct MacroArgs *args, char *s);
+void macro_UseNewArgs(struct MacroArgs *args);
+void macro_FreeArgs(struct MacroArgs *args);
+char const *macro_GetArg(uint32_t i);
+
+uint32_t macro_GetUniqueID(void);
+char const *macro_GetUniqueIDStr(void);
+void macro_SetUniqueID(uint32_t id);
 void macro_ShiftCurrentArgs(void);
 uint32_t macro_NbArgs(void);
 
-void macro_Init(void);
-
 #endif

include/helpers.h

@@ -27,4 +27,8 @@
 	#define DEVELOP 0
 #endif
 
+/* Macros for stringification */
+#define STR(x) #x
+#define EXPAND_AND_STR(x) STR(x)
+
 #endif /* HELPERS_H */

src/asm/asmy.y

@@ -496,6 +496,7 @@ static void strsubUTF8(char *dest, const char *src, uint32_t pos, uint32_t len)
 	struct Expression sVal;
 	int32_t nConstValue;
 	struct SectionSpec sectSpec;
+	struct MacroArgs *macroArg;
 }
 
 %type	<sVal>		relocexpr
@@ -595,6 +596,8 @@ static void strsubUTF8(char *dest, const char *src, uint32_t pos, uint32_t len)
 %token	T_SECT_WRAM0 T_SECT_VRAM T_SECT_ROMX T_SECT_ROM0 T_SECT_HRAM
 %token	T_SECT_WRAMX T_SECT_SRAM T_SECT_OAM
 
+%type	<macroArg> macroargs;
+
 %token	T_Z80_ADC T_Z80_ADD T_Z80_AND
 %token	T_Z80_BIT
 %token	T_Z80_CALL T_Z80_CCF T_Z80_CP T_Z80_CPL
@@ -699,17 +702,21 @@ macro		: T_ID {
 			yy_set_state(LEX_STATE_MACROARGS);
 		} macroargs {
 			yy_set_state(LEX_STATE_NORMAL);
-			if (!fstk_RunMacro($1))
+			if (!fstk_RunMacro($1, $3))
 				fatalerror("Macro '%s' not defined", $1);
 		}
 ;
 
-macroargs	: /* empty */
-		| macroarg
-		| macroarg ',' macroargs
-;
-
-macroarg	: T_STRING	{ macro_AddNewArg($1); }
+macroargs	: /* empty */ {
+			$$ = macro_NewArgs();
+		}
+		| T_STRING {
+			$$ = macro_NewArgs();
+			macro_AppendArg($$, strdup($1));
+		}
+		| macroargs ',' T_STRING {
+			macro_AppendArg($$, strdup($3));
+		}
 ;
 
 pseudoop	: equ

src/asm/fstack.c

@@ -85,14 +85,14 @@ static void pushcontext(void)
 	switch ((*ppFileStack)->nStatus = nCurrentStatus) {
 	case STAT_isMacroArg:
 	case STAT_isMacro:
-		macro_SaveCurrentArgs((*ppFileStack)->tzMacroArgs);
+		(*ppFileStack)->macroArgs = macro_GetCurrentArgs();
 		(*ppFileStack)->pMacro = pCurrentMacro;
 		break;
 	case STAT_isInclude:
 		(*ppFileStack)->pFile = pCurrentFile;
 		break;
 	case STAT_isREPTBlock:
-		macro_SaveCurrentArgs((*ppFileStack)->tzMacroArgs);
+		(*ppFileStack)->macroArgs = macro_GetCurrentArgs();
 		(*ppFileStack)->pREPTBlock = pCurrentREPTBlock;
 		(*ppFileStack)->nREPTBlockSize = nCurrentREPTBlockSize;
 		(*ppFileStack)->nREPTBlockCount = nCurrentREPTBlockCount;
@@ -102,6 +102,7 @@ static void pushcontext(void)
 	default:
 		fatalerror("%s: Internal error.", __func__);
 	}
+	(*ppFileStack)->uniqueID = macro_GetUniqueID();
 
 	nLineNo = 0;
 }
@@ -122,9 +123,7 @@ static int32_t popcontext(void)
 				yy_scan_bytes(pCurrentREPTBlock,
 					      nCurrentREPTBlockSize);
 			yy_switch_to_buffer(CurrentFlexHandle);
-			macro_UseCurrentArgs();
-			macro_SetArgID(nMacroCount++);
-			macro_UseNewArgs();
+			macro_SetUniqueID(nMacroCount++);
 
 			/* Increment REPT count in file path */
 			pREPTIterationWritePtr =
@@ -176,17 +175,29 @@ static int32_t popcontext(void)
 	CurrentFlexHandle = pLastFile->FlexHandle;
 	strcpy((char *)tzCurrentFileName, (char *)pLastFile->tzFileName);
 
-	switch (nCurrentStatus = pLastFile->nStatus) {
+	switch (pLastFile->nStatus) {
+		struct MacroArgs *args;
+
 	case STAT_isMacroArg:
 	case STAT_isMacro:
-		macro_RestoreCurrentArgs(pLastFile->tzMacroArgs);
+		args = macro_GetCurrentArgs();
+		if (nCurrentStatus == STAT_isMacro) {
+			macro_FreeArgs(args);
+			free(args);
+		}
+		macro_UseNewArgs(pLastFile->macroArgs);
 		pCurrentMacro = pLastFile->pMacro;
 		break;
 	case STAT_isInclude:
 		pCurrentFile = pLastFile->pFile;
 		break;
 	case STAT_isREPTBlock:
-		macro_RestoreCurrentArgs(pLastFile->tzMacroArgs);
+		args = macro_GetCurrentArgs();
+		if (nCurrentStatus == STAT_isMacro) {
+			macro_FreeArgs(args);
+			free(args);
+		}
+		macro_UseNewArgs(pLastFile->macroArgs);
 		pCurrentREPTBlock = pLastFile->pREPTBlock;
 		nCurrentREPTBlockSize = pLastFile->nREPTBlockSize;
 		nCurrentREPTBlockCount = pLastFile->nREPTBlockCount;
@@ -195,6 +206,9 @@ static int32_t popcontext(void)
 	default:
 		fatalerror("%s: Internal error.", __func__);
 	}
+	macro_SetUniqueID(pLastFile->uniqueID);
+
+	nCurrentStatus = pLastFile->nStatus;
 
 	nFileStackDepth--;
 
@@ -422,19 +436,19 @@ void fstk_RunInclude(char *tzFileName)
 /*
  * Set up a macro for parsing
  */
-uint32_t fstk_RunMacro(char *s)
+bool fstk_RunMacro(char *s, struct MacroArgs *args)
 {
 	struct sSymbol *sym = sym_FindMacro(s);
 	int nPrintedChars;
 
 	if (sym == NULL || sym->pMacro == NULL)
-		return 0;
+		return false;
 
 	pushcontext();
-	macro_SetArgID(nMacroCount++);
+	macro_SetUniqueID(nMacroCount++);
 	/* Minus 1 because there is a newline at the beginning of the buffer */
 	nLineNo = sym->nFileLine - 1;
-	macro_UseNewArgs();
+	macro_UseNewArgs(args);
 	nCurrentStatus = STAT_isMacro;
 	nPrintedChars = snprintf(tzCurrentFileName, _MAX_PATH + 1,
 				 "%s::%s", sym->tzFileName, s);
@@ -448,31 +462,7 @@ uint32_t fstk_RunMacro(char *s)
 					  strlen(pCurrentMacro->pMacro));
 	yy_switch_to_buffer(CurrentFlexHandle);
 
-	return 1;
-}
-
-/*
- * Set up a macroargument for parsing
- */
-void fstk_RunMacroArg(int32_t s)
-{
-	char *sym;
-
-	if (s == '@')
-		s = -1;
-	else
-		s -= '0';
-
-	sym = macro_FindArg(s);
-
-	if (sym == NULL)
-		fatalerror("No such macroargument");
-
-	pushcontext();
-	nCurrentStatus = STAT_isMacroArg;
-	snprintf(tzCurrentFileName, _MAX_PATH + 1, "%c", (uint8_t)s);
-	CurrentFlexHandle = yy_scan_bytes(sym, strlen(sym));
-	yy_switch_to_buffer(CurrentFlexHandle);
+	return true;
 }
 
 /*
@@ -505,10 +495,8 @@ void fstk_RunRept(uint32_t count, int32_t nReptLineNo)
 		/* For error printing to make sense, fake nLineNo */
 		nCurrentREPTBodyLastLine = nLineNo;
 		nLineNo = nReptLineNo;
-		macro_UseCurrentArgs();
 		pushcontext();
-		macro_SetArgID(nMacroCount++);
-		macro_UseNewArgs();
+		macro_SetUniqueID(nMacroCount++);
 		nCurrentREPTBlockCount = count;
 		nCurrentStatus = STAT_isREPTBlock;
 		nCurrentREPTBlockSize = ulNewMacroSize;

src/asm/globlex.c

@@ -192,14 +192,14 @@ uint32_t ParseNumber(char *s, uint32_t size)
  * return a pointer to the rest of the macro arg.
  * Otherwise, return NULL.
  */
-char *AppendMacroArg(char whichArg, char *dest, size_t *destIndex)
+char const *AppendMacroArg(char whichArg, char *dest, size_t *destIndex)
 {
-	char *marg;
+	char const *marg;
 
 	if (whichArg == '@')
-		marg = macro_FindArg(-1);
+		marg = macro_GetUniqueIDStr();
 	else if (whichArg >= '1' && whichArg <= '9')
-		marg = macro_FindArg(whichArg - '0');
+		marg = macro_GetArg(whichArg - '0');
 	else
 		fatalerror("Invalid macro argument '\\%c' in symbol", whichArg);
 
@@ -236,7 +236,7 @@ uint32_t ParseSymbol(char *src, uint32_t size)
 	char dest[MAXSYMLEN + 1];
 	size_t srcIndex = 0;
 	size_t destIndex = 0;
-	char *rest = NULL;
+	char const *rest = NULL;
 
 	while (srcIndex < size) {
 		char ch = src[srcIndex++];
@@ -302,11 +302,11 @@ uint32_t ParseSymbol(char *src, uint32_t size)
 
 uint32_t PutMacroArg(char *src, uint32_t size)
 {
-	char *s;
+	char const *s;
 
 	yyskipbytes(size);
 	if ((size == 2 && src[1] >= '1' && src[1] <= '9')) {
-		s = macro_FindArg(src[1] - '0');
+		s = macro_GetArg(src[1] - '0');
 
 		if (s != NULL)
 			yyunputstr(s);
@@ -318,14 +318,14 @@ uint32_t PutMacroArg(char *src, uint32_t size)
 	return 0;
 }
 
-uint32_t PutUniqueArg(char *src, uint32_t size)
+uint32_t PutUniqueID(char *src, uint32_t size)
 {
 	(void)src;
-	char *s;
+	char const *s;
 
 	yyskipbytes(size);
 
-	s = macro_FindArg(-1);
+	s = macro_GetUniqueIDStr();
 
 	if (s != NULL)
 		yyunputstr(s);
@@ -567,7 +567,7 @@ const struct sLexFloat tMacroArgToken = {
 };
 
 const struct sLexFloat tMacroUniqueToken = {
-	PutUniqueArg,
+	PutUniqueID,
 	T_LEX_MACROUNIQUE
 };
 

src/asm/lexer.c

@@ -583,33 +583,19 @@ struct sLexString *yylex_GetLongestFixed(void)
 size_t CopyMacroArg(char *dest, size_t maxLength, char c)
 {
 	size_t i;
-	char *s;
-	int32_t argNum;
+	char const *s;
 
-	switch (c) {
-	case '1':
-	case '2':
-	case '3':
-	case '4':
-	case '5':
-	case '6':
-	case '7':
-	case '8':
-	case '9':
-		argNum = c - '0';
-		break;
-	case '@':
-		argNum = -1;
-		break;
-	default:
+	if (c == '@')
+		s = macro_GetUniqueIDStr();
+	else if (c >= '1' && c <= '9')
+		s = macro_GetArg(c - '0');
+	else
 		return 0;
-	}
-
-	s = macro_FindArg(argNum);
 
 	if (s == NULL)
-		fatalerror("Macro argument not defined");
+		fatalerror("Macro argument '\\%c' not defined", c);
 
+	// TODO: `strncpy`, nay?
 	for (i = 0; s[i] != 0; i++) {
 		if (i >= maxLength)
 			fatalerror("Macro argument too long to fit buffer");

src/asm/macro.c

@@ -8,111 +8,92 @@
 #include "asm/macro.h"
 #include "asm/warning.h"
 
-static char *currentmacroargs[MAXMACROARGS + 1];
-static char *newmacroargs[MAXMACROARGS + 1];
+struct MacroArgs {
+	char *args[MAXMACROARGS];
+	unsigned int nbArgs;
+	unsigned int shift;
+};
 
-void macro_AddNewArg(char const *s)
+static struct MacroArgs defaultArgs = { .nbArgs = 0, .shift = 0 };
+static struct MacroArgs *macroArgs = &defaultArgs;
+static uint32_t uniqueID = -1;
+/*
+ * The initialization is somewhat harmful, since it is never used, but it
+ * guarantees the size of the buffer will be correct. I was unable to find a
+ * better solution, but if you have one, please feel free!
+ */
+static char uniqueIDBuf[] = "_" EXPAND_AND_STR(UINT32_MAX);
+static char *uniqueIDPtr = NULL;
+
+struct MacroArgs *macro_GetCurrentArgs(void)
 {
-	int32_t i = 0;
+	return macroArgs;
+}
 
-	while (i < MAXMACROARGS && newmacroargs[i] != NULL)
-		i++;
+struct MacroArgs *macro_NewArgs(void)
+{
+	struct MacroArgs *args = malloc(sizeof(*args));
 
-	if (i < MAXMACROARGS) {
-		if (s)
-			newmacroargs[i] = strdup(s);
-		else
-			newmacroargs[i] = NULL;
+	args->nbArgs = 0;
+	args->shift = 0;
+	return args;
+}
+
+void macro_AppendArg(struct MacroArgs *args, char *s)
+{
+	if (args->nbArgs == MAXMACROARGS)
+		yyerror("A maximum of " EXPAND_AND_STR(MAXMACROARGS)
+			" arguments is allowed");
+	args->args[args->nbArgs++] = s;
+}
+
+void macro_UseNewArgs(struct MacroArgs *args)
+{
+	macroArgs = args;
+}
+
+void macro_FreeArgs(struct MacroArgs *args)
+{
+	for (uint32_t i = 0; i < macroArgs->nbArgs; i++)
+		free(args->args[i]);
+}
+
+char const *macro_GetArg(uint32_t i)
+{
+	uint32_t realIndex = i + macroArgs->shift - 1;
+
+	return realIndex >= MAXMACROARGS ? NULL : macroArgs->args[realIndex];
+}
+
+uint32_t macro_GetUniqueID(void)
+{
+	return uniqueID;
+}
+
+char const *macro_GetUniqueIDStr(void)
+{
+	return uniqueIDPtr;
+}
+
+void macro_SetUniqueID(uint32_t id)
+{
+	uniqueID = id;
+	if (id == -1) {
+		uniqueIDPtr = NULL;
 	} else {
-		yyerror("A maximum of %d arguments allowed", MAXMACROARGS);
+		/* The buffer is guaranteed to be the correct size */
+		sprintf(uniqueIDBuf, "_%u", id);
+		uniqueIDPtr = uniqueIDBuf;
 	}
 }
 
-void macro_SaveCurrentArgs(char *save[])
-{
-	int32_t i;
-
-	for (i = 0; i <= MAXMACROARGS; i++) {
-		save[i] = currentmacroargs[i];
-		currentmacroargs[i] = NULL;
-	}
-}
-
-void macro_RestoreCurrentArgs(char *save[])
-{
-	int32_t i;
-
-	for (i = 0; i <= MAXMACROARGS; i++) {
-		free(currentmacroargs[i]);
-		currentmacroargs[i] = save[i];
-	}
-}
-
-void macro_UseNewArgs(void)
-{
-	int32_t i;
-
-	for (i = 0; i <= MAXMACROARGS; i++) {
-		free(currentmacroargs[i]);
-		currentmacroargs[i] = newmacroargs[i];
-		newmacroargs[i] = NULL;
-	}
-}
-
-char *macro_FindArg(int32_t i)
-{
-	if (i == -1)
-		i = MAXMACROARGS + 1;
-
-	assert(i >= 1);
-
-	assert((size_t)(i - 1)
-	       < sizeof(currentmacroargs) / sizeof(*currentmacroargs));
-
-	return currentmacroargs[i - 1];
-}
-
-void macro_UseCurrentArgs(void)
-{
-	int32_t i;
-
-	for (i = 1; i <= MAXMACROARGS; i++)
-		macro_AddNewArg(macro_FindArg(i));
-}
-
-void macro_SetArgID(uint32_t nMacroCount)
-{
-	char s[256];
-
-	snprintf(s, sizeof(s) - 1, "_%u", nMacroCount);
-	newmacroargs[MAXMACROARGS] = strdup(s);
-}
-
 void macro_ShiftCurrentArgs(void)
 {
-	int32_t i;
-
-	free(currentmacroargs[0]);
-	for (i = 0; i < MAXMACROARGS - 1; i++)
-		currentmacroargs[i] = currentmacroargs[i + 1];
-
-	currentmacroargs[MAXMACROARGS - 1] = NULL;
+	if (macroArgs->shift != macroArgs->nbArgs)
+		macroArgs->shift++;
 }
 
 uint32_t macro_NbArgs(void)
 {
-	uint32_t i = 0;
-
-	while (currentmacroargs[i] && i < MAXMACROARGS)
-		i++;
-
-	return i;
-}
-
-void macro_Init(void)
-{
-	for (uint32_t i = 0; i < MAXMACROARGS; i++) {
-		currentmacroargs[i] = NULL;
-		newmacroargs[i] = NULL;
-	}
+	return macroArgs->nbArgs - macroArgs->shift;
 }

src/asm/symbol.c

@@ -570,8 +570,6 @@ static inline char const *removeLeadingZeros(char const *ptr)
  */
 void sym_Init(void)
 {
-	macro_Init();
-
 	for (int32_t i = 0; i < HASHSIZE; i++)
 		tHashedSymbols[i] = NULL;
 

test/asm/rept-shift.asm

@@ -0,0 +1,17 @@
+m: macro
+	PRINTT "\1 "
+	REPT 4
+		SHIFT
+	ENDR
+	PRINTT "\1s!\n"
+
+	; Shifting a little more to check that over-shifting doesn't crash
+	SHIFT
+	SHIFT
+	REPT 256
+		SHIFT
+	ENDR
+	PRINTT "\1\n"
+endm
+
+ m This, used, not, to, work

test/asm/rept-shift.err

@@ -0,0 +1,2 @@
+ERROR: rept-shift.asm(17) -> rept-shift.asm::m(14):
+    Macro argument '\1' not defined

test/asm/rept-shift.out

@@ -0,0 +1 @@
+This works!

test/asm/unique-id.asm

@@ -0,0 +1,15 @@
+print EQUS "WARN \"\\@\""
+
+m: macro
+    print
+    REPT 2
+    	print
+    ENDR
+    print
+endm
+	; TODO: Ideally we'd test now as well, but it'd cause a fatal error
+	;print
+	m
+	;print
+	m
+	print

test/asm/unique-id.err

@@ -0,0 +1,19 @@
+warning: unique-id.asm(12) -> unique-id.asm::m(4): [-Wuser]
+    _0
+warning: unique-id.asm(12) -> unique-id.asm::m(5) -> unique-id.asm::m::REPT~1(6): [-Wuser]
+    _1
+warning: unique-id.asm(12) -> unique-id.asm::m(5) -> unique-id.asm::m::REPT~2(6): [-Wuser]
+    _2
+warning: unique-id.asm(12) -> unique-id.asm::m(8): [-Wuser]
+    _0
+warning: unique-id.asm(14) -> unique-id.asm::m(4): [-Wuser]
+    _3
+warning: unique-id.asm(14) -> unique-id.asm::m(5) -> unique-id.asm::m::REPT~1(6): [-Wuser]
+    _4
+warning: unique-id.asm(14) -> unique-id.asm::m(5) -> unique-id.asm::m::REPT~2(6): [-Wuser]
+    _5
+warning: unique-id.asm(14) -> unique-id.asm::m(8): [-Wuser]
+    _3
+ERROR: unique-id.asm(15):
+    Macro argument '\@' not defined
+while expanding symbol "print"