fix(parser): reject macro parameter lists the toolchain rejects

The C variadic spellings ("..." and the GNU "name..."), an empty or
trailing parameter, a missing comma, a stray token and an unterminated
list all parsed silently before: non-identifier tokens were skipped, so
"#define M(...)" defined a zero-parameter macro and invocations were
diagnosed only by argument count, if at all.  The toolchain rejects the
definition itself ("bad definition for macro"), and so does the
preprocessor now: the parameter list must be identifiers separated by
single commas and closed by the parenthesis, and a rejected definition
leaves the name unbound.  The spellings join the fuzz corpus.

Assisted-by: GLM 5.3

parser/fuzz_test.go

@@ -94,6 +94,8 @@ func FuzzParseExpand(f *testing.F) {
 	f.Add("#include \"no/such/header.h\"\n")
 	f.Add("#line 7 \"f.s\"\n")
 	f.Add("#define M(x) \\\n\tADD $x, R0 \\\n\tSUB $x, R1\nM(1)\n") // continuations
+	f.Add("#define M(...) x\nM(1)\n")                               // the C variadic spelling, a bad definition
+	f.Add("#define M(args...) x\nM(1)\n")                           // the GNU variadic spelling, one dotted name
 	// A body that repeats its argument multiplies every nesting level; the
 	// expansion must stop at the work budget, not produce the exponential
 	// result it names.

parser/preproc.go

@@ -238,6 +238,8 @@ func (pp *preproc) endif(line []token.Token) {
 // define parses "#define NAME[(formals)] body" into the macro table.  The
 // body runs to the end of the logical line (the lexer has already spliced
 // backslash continuations) and stops at a comment, which never expands.
+// A parameter list the toolchain would not accept is a bad definition: the
+// whole #define is rejected, and the name stays unbound.
 func (pp *preproc) define(line []token.Token) {
 	if len(line) < 3 || line[2].Kind != token.Ident {
 		return
@@ -246,22 +248,15 @@ func (pp *preproc) define(line []token.Token) {
 	args := []string(nil)
 	body := line[3:]
 	// The definition is parameterised only when '(' follows the name
-	// directly; the toolchain separates "#define A(x)" from
-	// "#define A (x)" by adjacency, and so does the column check here.
+	// directly; the toolchain separates "#define A(x)" from "#define A (x)"
+	// by adjacency, and so does the column check here.
 	if len(body) > 0 && body[0].Kind == token.LParen &&
 		body[0].Pos.Column == name.Pos.Column+utf8.RuneCountInString(name.Text) {
-		args = []string{}
-		i := 1
-		for i < len(body) && body[i].Kind != token.RParen {
-			if body[i].Kind == token.Ident {
-				args = append(args, body[i].Text)
-			}
-			i++
-		}
-		if i < len(body) {
-			body = body[i+1:]
-		} else {
-			body = nil
+		var ok bool
+		args, body, ok = formalList(body)
+		if !ok {
+			pp.errorf(name.Pos, "bad definition for macro %s", name.Text)
+			return
 		}
 	}
 	if i := slices.IndexFunc(body, func(t token.Token) bool { return t.Kind == token.Comment }); i >= 0 {
@@ -278,6 +273,48 @@ func (pp *preproc) define(line []token.Token) {
 	pp.macros[name.Text] = &macroDef{name: name.Text, args: args, body: stored}
 }
 
+// formalList reads the parameter list at the head of a macro definition, the
+// tokens after the '(' that follows the name.  The list the toolchain
+// accepts is identifiers separated by single commas and closed by ')': the C
+// variadic spellings ("..." and the GNU "name...", a single token here
+// because the lexer reads dots as identifier characters), an empty or
+// trailing parameter, a missing comma, a stray token and an unterminated
+// list are all rejected at the definition, so a use never silently binds to
+// a parameter list the writer did not write.
+func formalList(body []token.Token) (args []string, rest []token.Token, ok bool) {
+	i := 1
+	expectName := true
+	for {
+		if i >= len(body) {
+			return nil, nil, false
+		}
+		switch body[i].Kind {
+		case token.RParen:
+			if expectName && len(args) > 0 {
+				return nil, nil, false // a trailing comma
+			}
+			if args == nil {
+				args = []string{} // zero parameters, still parameterised
+			}
+			return args, body[i+1:], true
+		case token.Ident:
+			if !expectName || strings.ContainsRune(body[i].Text, '.') {
+				return nil, nil, false
+			}
+			args = append(args, body[i].Text)
+			expectName = false
+		case token.Comma:
+			if expectName {
+				return nil, nil, false // an empty parameter
+			}
+			expectName = true
+		default:
+			return nil, nil, false
+		}
+		i++
+	}
+}
+
 // bodyWithBreaks records the statement boundaries the continuations carry.
 // The lexer splices backslash-continued lines into one logical line, but the
 // toolchain keeps the newline as a token in the stored body, which is how a

parser/preproc_test.go

@@ -321,6 +321,55 @@ func TestWrongArgumentCountIsAnError(t *testing.T) {
 	}
 }
 
+// TestBadMacroDefinitionsAreRejected pins the preprocessor's reading of a
+// parameter list against the toolchain's: the list is identifiers separated
+// by single commas and closed by the parenthesis.  The C variadic spellings
+// ("...", and the GNU "name...", one token here because the lexer reads dots
+// as identifier characters), an empty or trailing parameter, a missing
+// comma, a stray token and an unterminated list are all a bad definition,
+// rejected at the definition exactly as the toolchain rejects them, so a use
+// never silently binds to a parameter list the writer did not write.
+func TestBadMacroDefinitionsAreRejected(t *testing.T) {
+	bad := map[string]string{
+		"ellipsis":                    "#define M(...) ADDQ $1, AX\n",
+		"ellipsis after a name":       "#define M(a, ...) ADDQ a, AX\n",
+		"gnu dots":                    "#define M(args...) ADDQ args, AX\n",
+		"trailing comma":              "#define M(a,) ADDQ a, AX\n",
+		"leading comma":               "#define M(,a) ADDQ a, AX\n",
+		"missing comma":               "#define M(a b) ADDQ a, AX\n",
+		"numeric parameter":           "#define M(1) ADDQ $1, AX\n",
+		"unterminated parameter list": "#define M(a\n",
+	}
+	for name, def := range bad {
+		t.Run(name, func(t *testing.T) {
+			f, errs := ParseWithOptions("t_amd64.s", def+"TEXT ·f(SB), NOSPLIT, $0\n\tM(BX)\n\tRET\n", Options{Expand: true})
+			if f == nil {
+				t.Fatal("ParseWithOptions returned a nil file")
+			}
+			if len(errs) == 0 || !strings.Contains(errs[0].Error(), "bad definition for macro M") {
+				t.Fatalf("got %v, want the definition rejected", errs)
+			}
+		})
+	}
+	// The plain shapes stay accepted, with the invocation checked against
+	// the parameter count the writer wrote.
+	for _, def := range []string{
+		"#define M() ADDQ $1, AX\n",
+		"#define M(a) ADDQ a, AX\n",
+		"#define M(a, b) ADDQ a, b\n",
+	} {
+		f, errs := ParseWithOptions("t_amd64.s", def+"TEXT ·f(SB), NOSPLIT, $0\n\tM(BX)\n\tRET\n", Options{Expand: true})
+		if f == nil || len(f.Decls) == 0 {
+			t.Fatalf("%s: unusable file", def)
+		}
+		for _, err := range errs {
+			if strings.Contains(err.Error(), "bad definition") {
+				t.Fatalf("%s: definition wrongly rejected: %v", def, errs)
+			}
+		}
+	}
+}
+
 func TestConditionalsSelectOneBranch(t *testing.T) {
 	_, got := expand(t, `
 #define MODE2