Implement -Wtruncation=level (#931)

* Implement -Wtruncation=level

-Wtruncation=0 is the same as the current -Wno-truncation.
-Wtruncation=2 is the same as the current -Wtruncation.
-Wtruncation=1 is the new default; it's less strict, allowing
N-bit values to be between -2**N and 2**N (exclusive).

* Implement generic "parametrized warning" system

* Test more `Wtruncation` variants

Co-authored-by: ISSOtm <eldredhabert0@gmail.com>

include/asm/warning.h

@@ -21,39 +21,48 @@ enum WarningState {
 };
 
 enum WarningID {
-	WARNING_ASSERT,		      /* Assertions */
+	WARNING_ASSERT,		      // Assertions
-	WARNING_BACKWARDS_FOR,	      /* `for` loop with backwards range */
+	WARNING_BACKWARDS_FOR,	      // `for` loop with backwards range
-	WARNING_BUILTIN_ARG,	      /* Invalid args to builtins */
+	WARNING_BUILTIN_ARG,	      // Invalid args to builtins
-	WARNING_CHARMAP_REDEF,        /* Charmap entry re-definition */
+	WARNING_CHARMAP_REDEF,        // Charmap entry re-definition
-	WARNING_DIV,		      /* Division undefined behavior */
+	WARNING_DIV,		      // Division undefined behavior
-	WARNING_EMPTY_DATA_DIRECTIVE, /* `db`, `dw` or `dl` directive without data in ROM */
+	WARNING_EMPTY_DATA_DIRECTIVE, // `db`, `dw` or `dl` directive without data in ROM
-	WARNING_EMPTY_MACRO_ARG,      /* Empty macro argument */
+	WARNING_EMPTY_MACRO_ARG,      // Empty macro argument
-	WARNING_EMPTY_STRRPL,	      /* Empty second argument in `STRRPL` */
+	WARNING_EMPTY_STRRPL,	      // Empty second argument in `STRRPL`
-	WARNING_LARGE_CONSTANT,	      /* Constants too large */
+	WARNING_LARGE_CONSTANT,	      // Constants too large
-	WARNING_LONG_STR,	      /* String too long for internal buffers */
+	WARNING_LONG_STR,	      // String too long for internal buffers
-	WARNING_MACRO_SHIFT,	      /* Shift past available arguments in macro */
+	WARNING_MACRO_SHIFT,	      // Shift past available arguments in macro
-	WARNING_NESTED_COMMENT,	      /* Comment-start delimiter in a block comment */
+	WARNING_NESTED_COMMENT,	      // Comment-start delimiter in a block comment
-	WARNING_OBSOLETE,	      /* Obsolete things */
+	WARNING_OBSOLETE,	      // Obsolete things
-	WARNING_SHIFT,		      /* Shifting undefined behavior */
+	WARNING_SHIFT,		      // Shifting undefined behavior
-	WARNING_SHIFT_AMOUNT,	      /* Strange shift amount */
+	WARNING_SHIFT_AMOUNT,	      // Strange shift amount
-	WARNING_TRUNCATION,	      /* Implicit truncation loses some bits */
+	WARNING_USER,		      // User warnings
-	WARNING_USER,		      /* User warnings */
 
-	NB_WARNINGS,
+	NB_PLAIN_WARNINGS,
 
-	/* Warnings past this point are "meta" warnings */
+	// Warnings past this point are "parametric" warnings, only mapping to a single flag
-	WARNING_ALL = NB_WARNINGS,
+#define PARAM_WARNINGS_START NB_PLAIN_WARNINGS
+	// Implicit truncation loses some bits
+	WARNING_TRUNCATION_1 = PARAM_WARNINGS_START,
+	WARNING_TRUNCATION_2,
+
+	NB_PLAIN_AND_PARAM_WARNINGS,
+#define NB_PARAM_WARNINGS (NB_PLAIN_AND_PARAM_WARNINGS - PARAM_WARNINGS_START)
+
+	// Warnings past this point are "meta" warnings
+#define META_WARNINGS_START NB_PLAIN_AND_PARAM_WARNINGS
+	WARNING_ALL = META_WARNINGS_START,
 	WARNING_EXTRA,
 	WARNING_EVERYTHING,
 
-	NB_WARNINGS_ALL
+	NB_WARNINGS,
-#define NB_META_WARNINGS (NB_WARNINGS_ALL - NB_WARNINGS)
+#define NB_META_WARNINGS (NB_WARNINGS - META_WARNINGS_START)
 };
 
-extern enum WarningState warningStates[NB_WARNINGS];
+extern enum WarningState warningStates[NB_PLAIN_AND_PARAM_WARNINGS];
 extern bool warningsAreErrors;
 
-void processWarningFlag(char const *flag);
+void processWarningFlag(char *flag);
 
 /*
  * Used to warn the user about problems that don't prevent the generation of

src/asm/opt.c

@@ -17,7 +17,8 @@ struct OptStackEntry {
 	bool haltnop;
 	bool optimizeLoads;
 	bool warningsAreErrors;
-	enum WarningState warningStates[NB_WARNINGS];
+	// Don't be confused: we use the size of the **global variable** `warningStates`!
+	enum WarningState warningStates[sizeof(warningStates)];
 	struct OptStackEntry *next;
 };
 
@@ -48,7 +49,7 @@ void opt_L(bool optimize)
 	optimizeLoads = optimize;
 }
 
-void opt_W(char const *flag)
+void opt_W(char *flag)
 {
 	processWarningFlag(flag);
 }

src/asm/rgbasm.1

@@ -248,10 +248,20 @@ Warn when shifting right a negative value.
 Use a division by 2**N instead.
 .It Fl Wshift-amount
 Warn when a shift's operand is negative or greater than 32.
-.It Fl Wno-truncation
+.It Fl Wtruncation=
 Warn when an implicit truncation (for example,
-.Ic db )
+.Ic db
-loses some bits.
+to an 8-bit value) loses some bits.
+.Fl Wtruncation=0
+or
+.Fl Wno-truncation
+disables this warning.
+.Fl Wtruncation=1
+warns when an N-bit value's absolute value is 2**N or greater.
+.Fl Wtruncation=2
+or just
+.Fl Wtruncation
+also warns when an N-bit value is below -2**(N-1), which will not fit in two's complement encoding.
 .It Fl Wno-user
 Warn when the
 .Ic WARN

src/asm/rpn.c

@@ -284,8 +284,10 @@ void rpn_CheckNBit(struct Expression const *expr, uint8_t n)
 	if (rpn_isKnown(expr)) {
 		int32_t val = expr->val;
 
-		if (val < -(1 << (n - 1)) || val >= 1 << n)
+		if (val <= -(1 << n) || val >= 1 << n)
-			warning(WARNING_TRUNCATION, "Expression must be %u-bit\n", n);
+			warning(WARNING_TRUNCATION_1, "Expression must be %u-bit\n", n);
+		else if (val < -(1 << (n - 1)))
+			warning(WARNING_TRUNCATION_2, "Expression must be %u-bit\n", n);
 	}
 }
 

src/asm/warning.c

@@ -6,6 +6,7 @@
  * SPDX-License-Identifier: MIT
  */
 
+#include <inttypes.h>
 #include <limits.h>
 #include <stdarg.h>
 #include <stdint.h>
@@ -21,7 +22,7 @@
 
 unsigned int nbErrors = 0;
 
-static enum WarningState const defaultWarnings[NB_WARNINGS] = {
+static const enum WarningState defaultWarnings[ARRAY_SIZE(warningStates)] = {
 	[WARNING_ASSERT]		= WARNING_ENABLED,
 	[WARNING_BACKWARDS_FOR]		= WARNING_DISABLED,
 	[WARNING_BUILTIN_ARG]		= WARNING_DISABLED,
@@ -37,11 +38,13 @@ static enum WarningState const defaultWarnings[NB_WARNINGS] = {
 	[WARNING_OBSOLETE]		= WARNING_ENABLED,
 	[WARNING_SHIFT]			= WARNING_DISABLED,
 	[WARNING_SHIFT_AMOUNT]		= WARNING_DISABLED,
-	[WARNING_TRUNCATION]		= WARNING_ENABLED,
 	[WARNING_USER]			= WARNING_ENABLED,
+
+	[WARNING_TRUNCATION_1]		= WARNING_ENABLED,
+	[WARNING_TRUNCATION_2]		= WARNING_DISABLED,
 };
 
-enum WarningState warningStates[NB_WARNINGS];
+enum WarningState warningStates[ARRAY_SIZE(warningStates)];
 
 bool warningsAreErrors; /* Set if `-Werror` was specified */
 
@@ -64,7 +67,7 @@ static enum WarningState warningState(enum WarningID id)
 	return state;
 }
 
-static char const *warningFlags[NB_WARNINGS_ALL] = {
+static const char * const warningFlags[NB_WARNINGS] = {
 	"assert",
 	"backwards-for",
 	"builtin-args",
@@ -80,15 +83,63 @@ static char const *warningFlags[NB_WARNINGS_ALL] = {
 	"obsolete",
 	"shift",
 	"shift-amount",
-	"truncation",
 	"user",
 
+	// Parametric warnings
+	"truncation",
+	"truncation",
+
 	/* Meta warnings */
 	"all",
 	"extra",
-	"everything" /* Especially useful for testing */
+	"everything", /* Especially useful for testing */
 };
 
+static const struct {
+	char const *name;
+	uint8_t nbLevels;
+	uint8_t defaultLevel;
+} paramWarnings[] = {
+	{ "truncation", 2, 2 },
+};
+
+static bool tryProcessParamWarning(char const *flag, uint8_t param, enum WarningState state)
+{
+	enum WarningID baseID = PARAM_WARNINGS_START;
+
+	for (size_t i = 0; i < ARRAY_SIZE(paramWarnings); i++) {
+		uint8_t maxParam = paramWarnings[i].nbLevels;
+
+		if (!strcmp(paramWarnings[i].name, flag)) { // Match!
+			// If making the warning an error but param is 0, set to the maximum
+			// This accommodates `-Werror=flag`, but also `-Werror=flag=0`, which is
+			// thus filtered out by the caller.
+			// A param of 0 makes sense for disabling everything, but neither for
+			// enabling nor "erroring". Use the default for those.
+			if (param == 0 && state != WARNING_DISABLED) {
+				param = paramWarnings[i].defaultLevel;
+			} else if (param > maxParam) {
+				if (param != 255) // Don't  warn if already capped
+					warnx("Got parameter %" PRIu8
+					      " for warning flag \"%s\", but the maximum is %"
+					      PRIu8 "; capping.\n",
+					      param, flag, maxParam);
+				param = maxParam;
+			}
+
+			// Set the first <param> to enabled/error, and disable the rest
+			for (uint8_t ofs = 0; ofs < maxParam; ofs++) {
+				warningStates[baseID + ofs] =
+					ofs < param ? state : WARNING_DISABLED;
+			}
+			return true;
+		}
+
+		baseID += maxParam;
+	}
+	return false;
+}
+
 enum MetaWarningCommand {
 	META_WARNING_DONE = NB_WARNINGS
 };
@@ -102,6 +153,8 @@ static uint8_t const _wallCommands[] = {
 	WARNING_EMPTY_STRRPL,
 	WARNING_LARGE_CONSTANT,
 	WARNING_LONG_STR,
+	WARNING_NESTED_COMMENT,
+	WARNING_OBSOLETE,
 	META_WARNING_DONE
 };
 
@@ -110,6 +163,9 @@ static uint8_t const _wextraCommands[] = {
 	WARNING_EMPTY_MACRO_ARG,
 	WARNING_MACRO_SHIFT,
 	WARNING_NESTED_COMMENT,
+	WARNING_OBSOLETE,
+	WARNING_TRUNCATION_1,
+	WARNING_TRUNCATION_2,
 	META_WARNING_DONE
 };
 
@@ -128,7 +184,8 @@ static uint8_t const _weverythingCommands[] = {
 	WARNING_OBSOLETE,
 	WARNING_SHIFT,
 	WARNING_SHIFT_AMOUNT,
-	/* WARNING_TRUNCATION, */
+	WARNING_TRUNCATION_1,
+	WARNING_TRUNCATION_2,
 	/* WARNING_USER, */
 	META_WARNING_DONE
 };
@@ -139,12 +196,12 @@ static uint8_t const *metaWarningCommands[NB_META_WARNINGS] = {
 	_weverythingCommands
 };
 
-void processWarningFlag(char const *flag)
+void processWarningFlag(char *flag)
 {
 	static bool setError = false;
 
 	/* First, try to match against a "meta" warning */
-	for (enum WarningID id = NB_WARNINGS; id < NB_WARNINGS_ALL; id++) {
+	for (enum WarningID id = META_WARNINGS_START; id < NB_WARNINGS; id++) {
 		/* TODO: improve the matching performance? */
 		if (!strcmp(flag, warningFlags[id])) {
 			/* We got a match! */
@@ -152,7 +209,7 @@ void processWarningFlag(char const *flag)
 				errx(1, "Cannot make meta warning \"%s\" into an error",
 				     flag);
 
-			uint8_t const *ptr = metaWarningCommands[id - NB_WARNINGS];
+			uint8_t const *ptr = metaWarningCommands[id - META_WARNINGS_START];
 
 			for (;;) {
 				if (*ptr == META_WARNING_DONE)
@@ -168,7 +225,7 @@ void processWarningFlag(char const *flag)
 
 	/* If it's not a meta warning, specially check against `-Werror` */
 	if (!strncmp(flag, "error", strlen("error"))) {
-		char const *errorFlag = flag + strlen("error");
+		char *errorFlag = flag + strlen("error");
 
 		switch (*errorFlag) {
 		case '\0':
@@ -189,14 +246,60 @@ void processWarningFlag(char const *flag)
 
 	/* Well, it's either a normal warning or a mistake */
 
-	/* Check if this is a negation */
-	bool isNegation = !strncmp(flag, "no-", strlen("no-")) && !setError;
-	char const *rootFlag = isNegation ? flag + strlen("no-") : flag;
 	enum WarningState state = setError ? WARNING_ERROR :
-				  isNegation ? WARNING_DISABLED : WARNING_ENABLED;
+				  /* Not an error, then check if this is a negation */
+				  strncmp(flag, "no-", strlen("no-")) ? WARNING_ENABLED
+								      : WARNING_DISABLED;
+	char const *rootFlag = state == WARNING_DISABLED ? flag + strlen("no-") : flag;
+
+	// Is this a "parametric" warning?
+	if (state != WARNING_DISABLED) { // The `no-` form cannot be parametrized
+		// First, check if there is an "equals" sign followed by a decimal number
+		char *equals = strchr(rootFlag, '=');
+
+		if (equals && equals[1] != '\0') { // Ignore an equal sign at the very end as well
+			// Is the rest of the string a decimal number?
+			// We want to avoid `strtoul`'s whitespace and sign, so we parse manually
+			uint8_t param = 0;
+			char const *ptr = equals + 1;
+			bool warned = false;
+
+			// The `if`'s condition above ensures that this will run at least once
+			do {
+				// If we don't have a digit, bail
+				if (*ptr < '0' || *ptr > '9')
+					break;
+				// Avoid overflowing!
+				if (param > UINT8_MAX - (*ptr - '0')) {
+					if (!warned)
+						warnx("Invalid warning flag \"%s\": capping parameter at 255\n",
+						      flag);
+					warned = true; // Only warn once, cap always
+					param = 255;
+					continue;
+				}
+				param = param * 10 + (*ptr - '0');
+
+				ptr++;
+			} while (*ptr);
+
+			// If we managed to the end of the string, check that the warning indeed
+			// accepts a parameter
+			if (*ptr == '\0') {
+				if (setError && param == 0) {
+					warnx("Ignoring nonsensical warning flag \"%s\"\n", flag);
+					return;
+				}
+				*equals = '\0'; // Truncate the param at the '='
+				if (tryProcessParamWarning(rootFlag, param,
+							   param == 0 ? WARNING_DISABLED : state))
+					return;
+			}
+		}
+	}
 
 	/* Try to match the flag against a "normal" flag */
-	for (enum WarningID id = 0; id < NB_WARNINGS; id++) {
+	for (enum WarningID id = 0; id < NB_PLAIN_WARNINGS; id++) {
 		if (!strcmp(rootFlag, warningFlags[id])) {
 			/* We got a match! */
 			warningStates[id] = state;
@@ -204,6 +307,11 @@ void processWarningFlag(char const *flag)
 		}
 	}
 
+	// Lastly, this might be a "parametric" warning without an equals sign
+	// If it is, treat the param as 1 if enabling, or 0 if disabling
+	if (tryProcessParamWarning(rootFlag, 0, state))
+		return;
+
 	warnx("Unknown warning `%s`", flag);
 }
 

test/asm/warn-truncation.asm

@@ -0,0 +1,38 @@
+FOO_F EQU 5
+BAR_F EQU 7
+
+
+SECTION "RAM", WRAMX[$d500]
+
+wLabel::
+
+
+SECTION "ROM", ROM0
+
+MACRO try
+	OPT \1
+	; no warning
+	db 1 << FOO_F
+	db $ff ^ (1 << FOO_F)
+	db ~(1 << FOO_F)
+	db 1 << BAR_F
+	db $ff ^ (1 << BAR_F)
+	dw wLabel
+	dw $10000 - wLabel
+	; warn at level 1
+	db 1 << 8
+	db ~(1 << 8)
+	dw $1d200
+	dw -$1d200
+	; warn at level 2
+	db ~(1 << BAR_F)
+	dw -wLabel
+ENDM
+
+	try Wno-truncation
+	try Wtruncation
+	try Wtruncation=0
+	try Wtruncation=1
+	try Wtruncation=2
+	try Werror=truncation=1
+	try Werror=truncation=2

test/asm/warn-truncation.err

@@ -0,0 +1,52 @@
+warning: warn-truncation.asm(33) -> warn-truncation.asm::try(23): [-Wtruncation]
+    Expression must be 8-bit
+warning: warn-truncation.asm(33) -> warn-truncation.asm::try(24): [-Wtruncation]
+    Expression must be 8-bit
+warning: warn-truncation.asm(33) -> warn-truncation.asm::try(25): [-Wtruncation]
+    Expression must be 16-bit
+warning: warn-truncation.asm(33) -> warn-truncation.asm::try(26): [-Wtruncation]
+    Expression must be 16-bit
+warning: warn-truncation.asm(33) -> warn-truncation.asm::try(28): [-Wtruncation]
+    Expression must be 8-bit
+warning: warn-truncation.asm(33) -> warn-truncation.asm::try(29): [-Wtruncation]
+    Expression must be 16-bit
+warning: warn-truncation.asm(35) -> warn-truncation.asm::try(23): [-Wtruncation]
+    Expression must be 8-bit
+warning: warn-truncation.asm(35) -> warn-truncation.asm::try(24): [-Wtruncation]
+    Expression must be 8-bit
+warning: warn-truncation.asm(35) -> warn-truncation.asm::try(25): [-Wtruncation]
+    Expression must be 16-bit
+warning: warn-truncation.asm(35) -> warn-truncation.asm::try(26): [-Wtruncation]
+    Expression must be 16-bit
+warning: warn-truncation.asm(36) -> warn-truncation.asm::try(23): [-Wtruncation]
+    Expression must be 8-bit
+warning: warn-truncation.asm(36) -> warn-truncation.asm::try(24): [-Wtruncation]
+    Expression must be 8-bit
+warning: warn-truncation.asm(36) -> warn-truncation.asm::try(25): [-Wtruncation]
+    Expression must be 16-bit
+warning: warn-truncation.asm(36) -> warn-truncation.asm::try(26): [-Wtruncation]
+    Expression must be 16-bit
+warning: warn-truncation.asm(36) -> warn-truncation.asm::try(28): [-Wtruncation]
+    Expression must be 8-bit
+warning: warn-truncation.asm(36) -> warn-truncation.asm::try(29): [-Wtruncation]
+    Expression must be 16-bit
+ERROR: warn-truncation.asm(37) -> warn-truncation.asm::try(23): [-Werror=truncation]
+    Expression must be 8-bit
+ERROR: warn-truncation.asm(37) -> warn-truncation.asm::try(24): [-Werror=truncation]
+    Expression must be 8-bit
+ERROR: warn-truncation.asm(37) -> warn-truncation.asm::try(25): [-Werror=truncation]
+    Expression must be 16-bit
+ERROR: warn-truncation.asm(37) -> warn-truncation.asm::try(26): [-Werror=truncation]
+    Expression must be 16-bit
+ERROR: warn-truncation.asm(38) -> warn-truncation.asm::try(23): [-Werror=truncation]
+    Expression must be 8-bit
+ERROR: warn-truncation.asm(38) -> warn-truncation.asm::try(24): [-Werror=truncation]
+    Expression must be 8-bit
+ERROR: warn-truncation.asm(38) -> warn-truncation.asm::try(25): [-Werror=truncation]
+    Expression must be 16-bit
+ERROR: warn-truncation.asm(38) -> warn-truncation.asm::try(26): [-Werror=truncation]
+    Expression must be 16-bit
+ERROR: warn-truncation.asm(38) -> warn-truncation.asm::try(28): [-Werror=truncation]
+    Expression must be 8-bit
+ERROR: warn-truncation.asm(38) -> warn-truncation.asm::try(29): [-Werror=truncation]
+    Expression must be 16-bit