Merge pull request #7431 from kalsprite/asm_named_register_slot

Reject asm operands in slots only a named hardware register can fill

core/rexcode/isa/riscv/tablegen/cpp-compiler/cpp-gen.odin

@@ -663,6 +663,22 @@ main :: proc() {
 	""")
 
 
+	strings.write_string(&sb, "\n\n")
+
+	strings.write_string(&sb, """
+		// RISC-V has no slot that only one named hardware register can fill.
+		u16 operand_type_named_reg_class(OperandType t) const {
+			gb_unused(t);
+			return REG_CLASS_NONE;
+		}
+
+		String named_reg_class_string(u16 reg_class) const {
+			gb_unused(reg_class);
+			return str_lit("hardware");
+		}
+	""")
+
+
 	strings.write_string(&sb, "\n\n")
 
 	strings.write_string(&sb, """

core/rexcode/isa/x86/tablegen/cpp-compiler/cpp-gen.odin

@@ -682,6 +682,36 @@ main :: proc() {
 	""")
 
 
+	strings.write_string(&sb, "\n\n")
+
+	strings.write_string(&sb, """
+		// Slots only a specific named hardware register can fill. They carry no GPR/vector
+		// class and, apart from OP_MM, no width either. Nothing else in the size/class
+		// check constrains them and a template parameter would otherwise slip through.
+		u16 operand_type_named_reg_class(OperandType t) const {
+			switch (t) {
+			case OP_SREG: return REG_CLASS_SEG;
+			case OP_CR:   return REG_CLASS_CR;
+			case OP_DR:   return REG_CLASS_DR;
+			case OP_STI:  return REG_CLASS_ST;
+			case OP_MM:   return REG_CLASS_MM;
+			}
+			return REG_CLASS_NONE;
+		}
+
+		String named_reg_class_string(u16 reg_class) const {
+			switch (reg_class) {
+			case REG_CLASS_SEG: return str_lit("segment");
+			case REG_CLASS_CR:  return str_lit("control");
+			case REG_CLASS_DR:  return str_lit("debug");
+			case REG_CLASS_ST:  return str_lit("x87 stack");
+			case REG_CLASS_MM:  return str_lit("MMX");
+			}
+			return str_lit("hardware");
+		}
+	""")
+
+
 	strings.write_string(&sb, "\n\n")
 
 	strings.write_string(&sb, """

src/asm_tables_amd64.cpp

@@ -695,6 +695,31 @@ struct Asm_amd64 {
 		return AsmRegClass_Unknown; // OP_M*, OP_IMM*, OP_REL*, OP_SREG/CR/DR/MM/STi, moffs, ptr, m16_16... : no GPR/XMM class constraint here
 	}
 
+	// Slots only a specific named hardware register can fill. They carry no GPR/vector
+	// class and, apart from OP_MM, no width either. Nothing else in the size/class
+	// check constrains them and a template parameter would otherwise slip through.
+	u16 operand_type_named_reg_class(OperandType t) const {
+		switch (t) {
+		case OP_SREG: return REG_CLASS_SEG;
+		case OP_CR:   return REG_CLASS_CR;
+		case OP_DR:   return REG_CLASS_DR;
+		case OP_STI:  return REG_CLASS_ST;
+		case OP_MM:   return REG_CLASS_MM;
+		}
+		return REG_CLASS_NONE;
+	}
+
+	String named_reg_class_string(u16 reg_class) const {
+		switch (reg_class) {
+		case REG_CLASS_SEG: return str_lit("segment");
+		case REG_CLASS_CR:  return str_lit("control");
+		case REG_CLASS_DR:  return str_lit("debug");
+		case REG_CLASS_ST:  return str_lit("x87 stack");
+		case REG_CLASS_MM:  return str_lit("MMX");
+		}
+		return str_lit("hardware");
+	}
+
 	u16 operand_type_bit_width(OperandType t) const {
 		switch (t) {
 		case OP_R8:  case OP_RM8:  case OP_M8:  case OP_AL_IMPL:  case OP_CL_IMPL: case OP_K_M8:  return 8;

src/asm_tables_riscv.cpp

@@ -618,6 +618,17 @@ struct Asm_riscv {
 		return reg_class_from_operand_type(t);
 	}
 
+	// RISC-V has no slot that only one named hardware register can fill.
+	u16 operand_type_named_reg_class(OperandType t) const {
+		gb_unused(t);
+		return REG_CLASS_NONE;
+	}
+
+	String named_reg_class_string(u16 reg_class) const {
+		gb_unused(reg_class);
+		return str_lit("hardware");
+	}
+
 	u16 operand_type_bit_width(OperandType t) const {
 		switch (t) {
 		case OP_NONE:

src/check_asm.cpp

@@ -152,6 +152,7 @@ enum AsmMismatch : u8 {
 	AsmMismatch_Class,     // register class mismatch
 	AsmMismatch_ImmRange,  // constant immediate does not fit the slot width
 	AsmMismatch_ImmType,   // non-integer constant where an integer immediate is required
+	AsmMismatch_NamedReg,  // slot only a named hardware register can fill
 };
 
 // Does a constant immediate value fit a slot of `bits` width (0 == unconstrained)?
@@ -241,6 +242,23 @@ gb_internal bool check_asm_operand_size_class(AsmCtx *asm_ctx, typename AsmCtx::
 	AsmRegClass want_class = asm_ctx->operand_type_reg_class(slot);
 	i32         want_w     = asm_ctx->operand_type_bit_width(slot);
 
+	// A slot only a named hardware register can fill (segment/control/debug/x87/MMX)
+	// carries no class and, apart from MMX, no width either. Nothing below would
+	// reject a template parameter standing in for one.
+	u16 want_named = asm_ctx->operand_type_named_reg_class(slot);
+	if (want_named != 0) {
+		bool ok = false;
+		if (operand->expr != nullptr && operand->expr->kind == Ast_AsmRegister) {
+			auto r = asm_ctx->register_lookup(operand->expr->AsmRegister.name.string);
+			ok = r && asm_ctx->reg_class(asm_ctx->register_codes[r]) == want_named;
+		}
+		if (!ok) {
+			if (mismatch_) *mismatch_ = AsmMismatch_NamedReg;
+			return false;
+		}
+		return true;
+	}
+
 	// A pure-label / sizeless slot imposes no reg width/class.
 	if (want_class == AsmRegClass_Unknown && want_w == 0) {
 		return true;
@@ -1366,13 +1384,13 @@ gb_internal void check_mnemonic(AsmCtx *asm_ctx, CheckerContext *ctx, Entity *tm
 			width_pref += cast(int)asm_ctx->operand_type_bit_width(type);
 
 			bool spot_ok = false;
+			AsmMismatch m = AsmMismatch_None;
 			if (kind_ok) {
 				bool mem_unsized = (src == AsmOperand_Memory) && are_types_identical(operand->type, t_rawptr);
 
 				if (dst == AsmOperand_Register_Or_Memory && src == AsmOperand_Memory && mem_unsized) {
 					spot_ok = true; // memory form accepts memory; no size check
 				} else {
-					AsmMismatch m = AsmMismatch_None;
 					i32 wb_ = 0, gb_ = 0;
 					spot_ok = check_asm_operand_size_class(asm_ctx, type, operand, &m, &wb_, &gb_);
 					if (!spot_ok && (m == AsmMismatch_Size || m == AsmMismatch_ImmRange) && wb_ > 0 && gb_ > 0) {
@@ -1385,7 +1403,9 @@ gb_internal void check_mnemonic(AsmCtx *asm_ctx, CheckerContext *ctx, Entity *tm
 			if (spot_ok) {
 				score += 2;
 				valid_spots[i] = true;
-			} else if (kind_ok) {
+			} else if (kind_ok && m != AsmMismatch_NamedReg) {
+				// A slot wanting a named hardware register is not a near miss for anything
+				// else, so it must not outrank a form that merely has the widths wrong.
 				score += 1; // kind matched, only value/size/class failed
 			}
 		}
@@ -1654,6 +1674,12 @@ gb_internal void check_mnemonic(AsmCtx *asm_ctx, CheckerContext *ctx, Entity *tm
 				      LIT(name), i,
 				      want_bits[i], LIT(asm_reg_class_strings[dst_reg_class]), LIT(asm_operand_kind_strings[dst]),
 				      got_bits[i],  LIT(asm_reg_class_strings[src_reg_class]), LIT(asm_operand_kind_strings[src]));
+			} else if (m == AsmMismatch_NamedReg) {
+				auto slot = operand_slot_type(forms[best_form], cast(int)i);
+				error(operands[i].expr, "'%.*s' operand-%td must be a named %.*s register, got a %.*s",
+				      LIT(name), i,
+				      LIT(asm_ctx->named_reg_class_string(asm_ctx->operand_type_named_reg_class(slot))),
+				      LIT(asm_operand_kind_strings[src]));
 			} else if (dst == AsmOperand_Immediate) {
 				error(operands[i].expr, "'%.*s' operand-%td must be an assemble-time constant or a $ immediate parameter, got a %.*s",
 				      LIT(name), i, LIT(asm_operand_kind_strings[src]));

tests/issues/run.bat

@@ -43,6 +43,7 @@ set COMMON=-define:ODIN_TEST_FANCY=false -file -vet -strict-style -ignore-unused
 ..\..\..\odin test ..\test_issue_7008.odin %COMMON%  || exit /b
 ..\..\..\odin check ..\test_issue_7012.odin -no-entry-point %COMMON% || exit /b
 ..\..\..\odin check ..\test_issue_7260.odin -no-entry-point %COMMON% || exit /b
+..\..\..\odin check ..\test_issue_asm_named_register_slot.odin -no-entry-point %COMMON% 2>&1 | find /c "Error:" | findstr /x "8" || exit /b
 ..\..\..\odin check ..\test_issue_ellipsis_type_call.odin -no-entry-point %COMMON% 2>&1 | find /c "Error:" | findstr /x "10" || exit /b
 ..\..\..\odin check ..\test_issue_foreign_redeclaration.odin -no-entry-point %COMMON% || exit /b
 ..\..\..\odin check ..\test_issue_foreign_redeclaration_mismatch.odin -no-entry-point %COMMON% 2>&1 | find /c "Error:" | findstr /x "1" || exit /b

tests/issues/run.sh

@@ -95,6 +95,16 @@ $ODIN test ../test_issue_7356.odin $COMMON
 $ODIN build ../test_issue_7167.odin $COMMON
 $ODIN build ../test_issue_7188.odin $COMMON
 $ODIN check ../test_issue_7260.odin -no-entry-point $COMMON_CHECK
+
+# `asm` templates are amd64-only, so this file is empty on every other architecture
+if [[ "$(uname -m)" == "x86_64" || "$(uname -m)" == "amd64" ]]; then
+	if [[ $($ODIN check ../test_issue_asm_named_register_slot.odin -no-entry-point $COMMON_CHECK 2>&1 >/dev/null | grep -c "Error:") -eq 8 ]]; then
+		echo "SUCCESSFUL 1/1"
+	else
+		echo "SUCCESSFUL 0/1"
+		exit 1
+	fi
+fi
 $ODIN check ../test_issue_foreign_redeclaration.odin -no-entry-point $COMMON_CHECK
 if [[ $($ODIN check ../test_issue_foreign_redeclaration_mismatch.odin -no-entry-point $COMMON_CHECK 2>&1 >/dev/null | grep -c "Error:") -eq 1 ]]; then
 	echo "SUCCESSFUL 1/1"

tests/issues/test_issue_asm_named_register_slot.odin

@@ -0,0 +1,41 @@
+#+build amd64
+// A slot that only a named hardware register can fill (segment/control/debug/x87/MMX)
+// carries no width and no register class, so it used to absorb any operand at all and
+// hand the backend an instruction that does not encode.
+package test_issues
+
+// Rejected: none of these widths pair up, and only `mov`'s segment-register forms ever
+// admitted them.
+bad_64_32 :: asm(a: i64) -> (r: i32) { mov r, a; }
+bad_8_16  :: asm(a: u8)  -> (r: u16) { mov r, a; }
+bad_16_8  :: asm(a: u16) -> (r: u8)  { mov r, a; }
+bad_64_8  :: asm(a: u64) -> (r: u8)  { mov r, a; }
+
+// Accepted: equal widths, regardless of signedness or pointer spelling.
+ok_32   :: asm(a: i32)    -> (r: u32)    { mov r, a; }
+ok_64   :: asm(a: u64)    -> (r: i64)    { mov r, a; }
+ok_ptr  :: asm(a: rawptr) -> (r: ^i32)   { mov r, a; }
+
+// Accepted: the named registers those forms are actually for.
+ok_seg  :: asm(a: u64) -> (r: u64) { mov %ds,  a; mov r, a; }
+ok_ctrl :: asm(a: u64) -> (r: u64) { mov %cr0, a; mov r, a; }
+ok_dbg  :: asm(a: u64) -> (r: u64) { mov %dr0, a; mov r, a; }
+
+use :: proc() {
+	a8:  u8
+	a16: u16
+	a32: i32
+	a64: i64
+	au64: u64
+	ap:  rawptr
+	_ = bad_64_32(a64)
+	_ = bad_8_16(a8)
+	_ = bad_16_8(a16)
+	_ = bad_64_8(au64)
+	_ = ok_32(a32)
+	_ = ok_64(au64)
+	_ = ok_ptr(ap)
+	_ = ok_seg(au64)
+	_ = ok_ctrl(au64)
+	_ = ok_dbg(au64)
+}