[PATCH 0/5] xdrgen: Improve diagnostic reporting

[PATCH 0/5] xdrgen: Improve diagnostic reporting

[Chuck Lever]

Add missing detection and reporting for semantic errors specified in
RFCs 4506 and 5531.

Chuck Lever (5):
  xdrgen: Align the error caret under tab-indented source
  xdrgen: Record the source position of each declared identifier
  xdrgen: Reject specifications that define a name twice
  xdrgen: Enforce RFC 5531 name and number scoping for RPC programs
  xdrgen: Reject out-of-range program, version, and procedure numbers

 .../net/sunrpc/xdrgen/subcmds/declarations.py |  10 +-
 .../net/sunrpc/xdrgen/subcmds/definitions.py  |   7 +-
 tools/net/sunrpc/xdrgen/subcmds/lint.py       |   7 +-
 tools/net/sunrpc/xdrgen/subcmds/source.py     |   7 +-
 .../tests/bad-procedure-number-negative.x     |  20 ++
 .../tests/bad-procedure-number-too-large.x    |  20 ++
 .../tests/bad-program-number-negative.x       |  19 ++
 .../tests/bad-program-number-too-large.x      |  19 ++
 .../tests/bad-version-number-negative.x       |  19 ++
 .../tests/bad-version-number-too-large.x      |  19 ++
 tools/net/sunrpc/xdrgen/xdr_ast.py            | 290 +++++++++++++++---
 tools/net/sunrpc/xdrgen/xdr_parse.py          |  46 ++-
 12 files changed, 428 insertions(+), 55 deletions(-)
 create mode 100644 tools/net/sunrpc/xdrgen/tests/bad-procedure-number-negative.x
 create mode 100644 tools/net/sunrpc/xdrgen/tests/bad-procedure-number-too-large.x
 create mode 100644 tools/net/sunrpc/xdrgen/tests/bad-program-number-negative.x
 create mode 100644 tools/net/sunrpc/xdrgen/tests/bad-program-number-too-large.x
 create mode 100644 tools/net/sunrpc/xdrgen/tests/bad-version-number-negative.x
 create mode 100644 tools/net/sunrpc/xdrgen/tests/bad-version-number-too-large.x

-- 
2.54.0

[PATCH 1/5] xdrgen: Align the error caret under tab-indented source

[Chuck Lever]

When xdrgen reports a parse or transform error, it prints the
offending source line followed by a caret marking the column. The
source line is emitted with its tab characters intact, but the caret
offset is computed from a tab-expanded copy of the text ahead of the
column. A terminal expands the line's leading tabs relative to the
four-space output indent, while the caret math expands the same tabs
from column zero, so the two disagree whenever the line is indented
with tabs and the caret lands past the token it should mark.

Render the displayed line with its tabs already expanded so the line
and the caret share one tab origin and the four-space indent cancels.
Fold the now-identical line-and-caret formatting out of both error
handlers into a single helper, so every caller reports the same
aligned output.

Signed-off-by: Chuck Lever <cel@kernel.org>
---
 tools/net/sunrpc/xdrgen/xdr_parse.py | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/tools/net/sunrpc/xdrgen/xdr_parse.py b/tools/net/sunrpc/xdrgen/xdr_parse.py
index 241e96c1fdd9..3e76717d2f85 100644
--- a/tools/net/sunrpc/xdrgen/xdr_parse.py
+++ b/tools/net/sunrpc/xdrgen/xdr_parse.py
@@ -63,6 +63,22 @@ def get_xdr_enum_validation() -> bool:
     return enum_validation
 
 
+def format_source_caret(line_text: str, column: int) -> list[str]:
+    """Render an offending source line with a caret beneath a column.
+
+    Args:
+        line_text: The raw source line containing the error
+        column: 1-based column of the offending token within line_text
+
+    Returns:
+        Output lines for the diagnostic: a blank separator, the source
+        line with tabs expanded, and a caret aligned under the column.
+    """
+    expanded = line_text.expandtabs()
+    caret = len(line_text[: column - 1].expandtabs())
+    return ["", f"    {expanded}", f"    {' ' * caret}^"]
+
+
 def make_error_handler(source: str, filename: str) -> Callable[[UnexpectedInput], bool]:
     """Create an error handler that reports the first parse error and aborts.
 
@@ -110,10 +126,7 @@ def make_error_handler(source: str, filename: str) -> Callable[[UnexpectedInput]
             msg_parts.append(str(e).split("\n")[0])
 
         # Show the offending line with a caret pointing to the error
-        msg_parts.append("")
-        msg_parts.append(f"    {line_text}")
-        prefix = line_text[: column - 1].expandtabs()
-        msg_parts.append(f"    {' ' * len(prefix)}^")
+        msg_parts.extend(format_source_caret(line_text, column))
 
         sys.stderr.write("\n".join(msg_parts) + "\n")
         raise XdrParseError()
@@ -151,10 +164,7 @@ def handle_transform_error(e: VisitError, source: str, filename: str) -> None:
 
     # Show the offending line with a caret pointing to the error
     if line_text:
-        msg_parts.append("")
-        msg_parts.append(f"    {line_text}")
-        prefix = line_text[: column - 1].expandtabs()
-        msg_parts.append(f"    {' ' * len(prefix)}^")
+        msg_parts.extend(format_source_caret(line_text, column))
 
     sys.stderr.write("\n".join(msg_parts) + "\n")
 
-- 
2.54.0

[PATCH 2/5] xdrgen: Record the source position of each declared identifier

[Chuck Lever]

In preparation for semantic checks that reject a malformed
specification, record where each declared identifier appears in the
source so a diagnostic can point at the name in error.

The transformer keeps each identifier's spelling but discards its
position, retaining only the position of the enclosing definition.
A caret built from that position falls on the definition keyword
rather than on the identifier, because the definition production
begins at the keyword.

Store the identifier's own line and column on every named
construct: constants, enumerated types and their enumerators,
structs, unions, pointers, typedef declarations, and RPC program,
version, and procedure names. The fields live on the AST base node
and are keyword-only, so lark's positional construction of each
node is unaffected; a construct whose position is not recorded
leaves them zero.

Signed-off-by: Chuck Lever <cel@kernel.org>
---
 tools/net/sunrpc/xdrgen/xdr_ast.py | 116 +++++++++++++++++++++--------
 1 file changed, 83 insertions(+), 33 deletions(-)

diff --git a/tools/net/sunrpc/xdrgen/xdr_ast.py b/tools/net/sunrpc/xdrgen/xdr_ast.py
index 14bff9477473..15d2c6c40dd9 100644
--- a/tools/net/sunrpc/xdrgen/xdr_ast.py
+++ b/tools/net/sunrpc/xdrgen/xdr_ast.py
@@ -5,7 +5,7 @@
 
 import sys
 from typing import List
-from dataclasses import dataclass
+from dataclasses import dataclass, KW_ONLY
 
 from lark import ast_utils, Transformer
 from lark.tree import Meta
@@ -65,6 +65,16 @@ max_widths = {
 class _XdrAst(ast_utils.Ast):
     """Base class for the XDR abstract syntax tree"""
 
+    # Source position of the construct's declared identifier, when
+    # the transformer records one, so semantic diagnostics can point
+    # at the exact declaration. The KW_ONLY marker makes the fields
+    # keyword-only, so they never disturb the positional child
+    # ordering lark uses to build each node; 0 means the position was
+    # not recorded.
+    _: KW_ONLY
+    line: int = 0
+    column: int = 0
+
 
 @dataclass
 class _XdrIdentifier(_XdrAst):
@@ -543,7 +553,8 @@ class ParseToAst(Transformer):
 
     def identifier(self, children):
         """Instantiate one _XdrIdentifier object"""
-        return _XdrIdentifier(children[0].value)
+        token = children[0]
+        return _XdrIdentifier(token.value, line=token.line, column=token.column)
 
     def value(self, children):
         """Instantiate one _XdrValue object"""
@@ -573,84 +584,103 @@ class ParseToAst(Transformer):
 
     def constant_def(self, children):
         """Instantiate one _XdrConstant object"""
-        name = children[0].symbol
+        ident = children[0]
         value = children[1].value
-        return _XdrConstant(name, value)
+        return _XdrConstant(ident.symbol, value, line=ident.line, column=ident.column)
 
     def enum(self, children):
         """Instantiate one _XdrEnum object"""
-        enum_name = children[0].symbol
+        name_ident = children[0]
 
         i = 0
         enumerators = []
         body = children[1]
         while i < len(body.children):
-            name = body.children[i].symbol
+            ident = body.children[i]
             value = body.children[i + 1].value
-            enumerators.append(_XdrEnumerator(name, value))
+            enumerators.append(
+                _XdrEnumerator(
+                    ident.symbol, value, line=ident.line, column=ident.column
+                )
+            )
             i = i + 2
 
-        return _XdrEnum(enum_name, enumerators)
+        return _XdrEnum(
+            name_ident.symbol,
+            enumerators,
+            line=name_ident.line,
+            column=name_ident.column,
+        )
 
     def fixed_length_opaque(self, children):
         """Instantiate one _XdrFixedLengthOpaque declaration object"""
-        name = children[0].symbol
+        ident = children[0]
         size = children[1].value
 
-        return _XdrFixedLengthOpaque(name, size)
+        return _XdrFixedLengthOpaque(
+            ident.symbol, size, line=ident.line, column=ident.column
+        )
 
     def variable_length_opaque(self, children):
         """Instantiate one _XdrVariableLengthOpaque declaration object"""
-        name = children[0].symbol
+        ident = children[0]
         if children[1] is not None:
             maxsize = children[1].value
         else:
             maxsize = "0"
 
-        return _XdrVariableLengthOpaque(name, maxsize)
+        return _XdrVariableLengthOpaque(
+            ident.symbol, maxsize, line=ident.line, column=ident.column
+        )
 
     def string(self, children):
         """Instantiate one _XdrString declaration object"""
-        name = children[0].symbol
+        ident = children[0]
         if children[1] is not None:
             maxsize = children[1].value
         else:
             maxsize = "0"
 
-        return _XdrString(name, maxsize)
+        return _XdrString(ident.symbol, maxsize, line=ident.line, column=ident.column)
 
     def fixed_length_array(self, children):
         """Instantiate one _XdrFixedLengthArray declaration object"""
         spec = children[0]
-        name = children[1].symbol
+        ident = children[1]
         size = children[2].value
 
-        return _XdrFixedLengthArray(name, spec, size)
+        return _XdrFixedLengthArray(
+            ident.symbol, spec, size, line=ident.line, column=ident.column
+        )
 
     def variable_length_array(self, children):
         """Instantiate one _XdrVariableLengthArray declaration object"""
         spec = children[0]
-        name = children[1].symbol
+        ident = children[1]
         if children[2] is not None:
             maxsize = children[2].value
         else:
             maxsize = "0"
 
-        return _XdrVariableLengthArray(name, spec, maxsize)
+        return _XdrVariableLengthArray(
+            ident.symbol, spec, maxsize, line=ident.line, column=ident.column
+        )
 
     def optional_data(self, children):
         """Instantiate one _XdrOptionalData declaration object"""
         spec = children[0]
-        name = children[1].symbol
+        ident = children[1]
 
-        return _XdrOptionalData(name, spec)
+        return _XdrOptionalData(
+            ident.symbol, spec, line=ident.line, column=ident.column
+        )
 
     def basic(self, children):
         """Instantiate one _XdrBasic object"""
         spec = children[0]
-        name = children[1].symbol
+        ident = children[1]
 
-        return _XdrBasic(name, spec)
+        return _XdrBasic(ident.symbol, spec, line=ident.line, column=ident.column)
 
     def void(self, children):
         """Instantiate one _XdrVoid declaration object"""
@@ -659,17 +689,19 @@ class ParseToAst(Transformer):
 
     def struct(self, children):
         """Instantiate one _XdrStruct object"""
-        name = children[0].symbol
+        ident = children[0]
+        name = ident.symbol
         fields = children[1].children
+        pos = {"line": ident.line, "column": ident.column}
 
         last_field = fields[-1]
         if (
             isinstance(last_field, _XdrOptionalData)
             and name == last_field.spec.type_name
         ):
-            return _XdrPointer(name, fields)
+            return _XdrPointer(name, fields, **pos)
 
-        return _XdrStruct(name, fields)
+        return _XdrStruct(name, fields, **pos)
 
     def typedef(self, children):
         """Instantiate one _XdrTypedef object"""
@@ -694,39 +726,57 @@ class ParseToAst(Transformer):
 
     def union(self, children):
         """Instantiate one _XdrUnion object"""
-        name = children[0].symbol
+        ident = children[0]
 
         body = children[1]
         discriminant = body.children[0].children[0]
         cases = body.children[1:-1]
         default = body.children[-1]
 
-        return _XdrUnion(name, discriminant, cases, default)
+        return _XdrUnion(
+            ident.symbol,
+            discriminant,
+            cases,
+            default,
+            line=ident.line,
+            column=ident.column,
+        )
 
     def procedure_def(self, children):
         """Instantiate one _RpcProcedure object"""
         result = children[0]
-        name = children[1].symbol
+        ident = children[1]
         argument = children[2]
         number = children[3].value
 
-        return _RpcProcedure(name, number, argument, result)
+        return _RpcProcedure(
+            ident.symbol,
+            number,
+            argument,
+            result,
+            line=ident.line,
+            column=ident.column,
+        )
 
     def version_def(self, children):
         """Instantiate one _RpcVersion object"""
-        name = children[0].symbol
+        ident = children[0]
         number = children[-1].value
         procedures = children[1:-1]
 
-        return _RpcVersion(name, number, procedures)
+        return _RpcVersion(
+            ident.symbol, number, procedures, line=ident.line, column=ident.column
+        )
 
     def program_def(self, children):
         """Instantiate one _RpcProgram object"""
-        name = children[0].symbol
+        ident = children[0]
         number = children[-1].value
         versions = children[1:-1]
 
-        return _RpcProgram(name, number, versions)
+        return _RpcProgram(
+            ident.symbol, number, versions, line=ident.line, column=ident.column
+        )
 
     def pragma_def(self, children):
         """Instantiate one _Pragma object"""
-- 
2.54.0

[PATCH 3/5] xdrgen: Reject specifications that define a name twice

[Chuck Lever]

When an RPC specification defines the same type or constant name
more than once, currently xdrgen emits every definition without
complaint. The duplication surfaces later as a C compiler error
about a redefined struct or function that points at generated code
instead of the actual offending line in the .x source.

RFC 4506 Section 6.4 places constant and type identifiers in a
single name space that must be unique within a specification. Add
a semantic check that enforces this rule.

Signed-off-by: Chuck Lever <cel@kernel.org>
---
 .../net/sunrpc/xdrgen/subcmds/declarations.py | 10 +--
 .../net/sunrpc/xdrgen/subcmds/definitions.py  |  7 ++-
 tools/net/sunrpc/xdrgen/subcmds/lint.py       |  7 ++-
 tools/net/sunrpc/xdrgen/subcmds/source.py     |  7 ++-
 tools/net/sunrpc/xdrgen/xdr_ast.py            | 61 ++++++++++++++++++-
 tools/net/sunrpc/xdrgen/xdr_parse.py          | 20 ++++++
 6 files changed, 101 insertions(+), 11 deletions(-)

diff --git a/tools/net/sunrpc/xdrgen/subcmds/declarations.py b/tools/net/sunrpc/xdrgen/subcmds/declarations.py
index ed83d48d1f68..f187611466d7 100644
--- a/tools/net/sunrpc/xdrgen/subcmds/declarations.py
+++ b/tools/net/sunrpc/xdrgen/subcmds/declarations.py
@@ -21,16 +21,15 @@ from generators.union import XdrUnionGenerator
 
 from xdr_ast import transform_parse_tree, _RpcProgram, Specification
 from xdr_ast import _XdrEnum, _XdrPointer, _XdrTypedef, _XdrStruct, _XdrUnion
+from xdr_ast import XdrSemanticError
 from xdr_parse import xdr_parser, set_xdr_annotate
 from xdr_parse import make_error_handler, XdrParseError
-from xdr_parse import handle_transform_error
+from xdr_parse import handle_transform_error, handle_semantic_error
 
 logger.setLevel(logging.INFO)
 
 
-def emit_header_declarations(
-    root: Specification, language: str, peer: str
-) -> None:
+def emit_header_declarations(root: Specification, language: str, peer: str) -> None:
     """Emit header declarations"""
     for definition in root.definitions:
         if isinstance(definition.value, _XdrEnum):
@@ -68,6 +67,9 @@ def subcmd(args: Namespace) -> int:
         except VisitError as e:
             handle_transform_error(e, source, args.filename)
             return 1
+        except XdrSemanticError as e:
+            handle_semantic_error(e, source, args.filename)
+            return 1
 
         gen = XdrHeaderTopGenerator(args.language, args.peer)
         gen.emit_declaration(args.filename, ast)
diff --git a/tools/net/sunrpc/xdrgen/subcmds/definitions.py b/tools/net/sunrpc/xdrgen/subcmds/definitions.py
index a48ca0549382..77b666943a11 100644
--- a/tools/net/sunrpc/xdrgen/subcmds/definitions.py
+++ b/tools/net/sunrpc/xdrgen/subcmds/definitions.py
@@ -21,12 +21,12 @@ from generators.typedef import XdrTypedefGenerator
 from generators.struct import XdrStructGenerator
 from generators.union import XdrUnionGenerator
 
-from xdr_ast import transform_parse_tree, Specification
+from xdr_ast import transform_parse_tree, Specification, XdrSemanticError
 from xdr_ast import _RpcProgram, _XdrConstant, _XdrEnum, _XdrPassthru, _XdrPointer
 from xdr_ast import _XdrTypedef, _XdrStruct, _XdrUnion
 from xdr_parse import xdr_parser, set_xdr_annotate
 from xdr_parse import make_error_handler, XdrParseError
-from xdr_parse import handle_transform_error
+from xdr_parse import handle_transform_error, handle_semantic_error
 
 logger.setLevel(logging.INFO)
 
@@ -94,6 +94,9 @@ def subcmd(args: Namespace) -> int:
         except VisitError as e:
             handle_transform_error(e, source, args.filename)
             return 1
+        except XdrSemanticError as e:
+            handle_semantic_error(e, source, args.filename)
+            return 1
 
         gen = XdrHeaderTopGenerator(args.language, args.peer)
         gen.emit_definition(args.filename, ast)
diff --git a/tools/net/sunrpc/xdrgen/subcmds/lint.py b/tools/net/sunrpc/xdrgen/subcmds/lint.py
index e1da49632e62..b4ea0f55f079 100644
--- a/tools/net/sunrpc/xdrgen/subcmds/lint.py
+++ b/tools/net/sunrpc/xdrgen/subcmds/lint.py
@@ -11,8 +11,8 @@ from lark import logger
 from lark.exceptions import VisitError
 
 from xdr_parse import xdr_parser, make_error_handler, XdrParseError
-from xdr_parse import handle_transform_error
-from xdr_ast import transform_parse_tree
+from xdr_parse import handle_transform_error, handle_semantic_error
+from xdr_ast import transform_parse_tree, XdrSemanticError
 
 logger.setLevel(logging.DEBUG)
 
@@ -34,5 +34,8 @@ def subcmd(args: Namespace) -> int:
         except VisitError as e:
             handle_transform_error(e, source, args.filename)
             return 1
+        except XdrSemanticError as e:
+            handle_semantic_error(e, source, args.filename)
+            return 1
 
     return 0
diff --git a/tools/net/sunrpc/xdrgen/subcmds/source.py b/tools/net/sunrpc/xdrgen/subcmds/source.py
index 27e8767b1b58..56eba34d8eb3 100644
--- a/tools/net/sunrpc/xdrgen/subcmds/source.py
+++ b/tools/net/sunrpc/xdrgen/subcmds/source.py
@@ -21,11 +21,11 @@ from generators.union import XdrUnionGenerator
 
 from xdr_ast import transform_parse_tree, _RpcProgram, Specification
 from xdr_ast import _XdrAst, _XdrEnum, _XdrPassthru, _XdrPointer
-from xdr_ast import _XdrStruct, _XdrTypedef, _XdrUnion
+from xdr_ast import _XdrStruct, _XdrTypedef, _XdrUnion, XdrSemanticError
 
 from xdr_parse import xdr_parser, set_xdr_annotate, set_xdr_enum_validation
 from xdr_parse import make_error_handler, XdrParseError
-from xdr_parse import handle_transform_error
+from xdr_parse import handle_transform_error, handle_semantic_error
 
 logger.setLevel(logging.INFO)
 
@@ -123,6 +123,9 @@ def subcmd(args: Namespace) -> int:
         except VisitError as e:
             handle_transform_error(e, source, args.filename)
             return 1
+        except XdrSemanticError as e:
+            handle_semantic_error(e, source, args.filename)
+            return 1
         match args.peer:
             case "server":
                 generate_server_source(args.filename, ast, args.language)
diff --git a/tools/net/sunrpc/xdrgen/xdr_ast.py b/tools/net/sunrpc/xdrgen/xdr_ast.py
index 15d2c6c40dd9..cf68ff5dfe17 100644
--- a/tools/net/sunrpc/xdrgen/xdr_ast.py
+++ b/tools/net/sunrpc/xdrgen/xdr_ast.py
@@ -814,7 +814,9 @@ def _merge_consecutive_passthru(definitions: List[Definition]) -> List[Definitio
             lines = [definitions[i].value.content]
             meta = definitions[i].meta
             j = i + 1
-            while j < len(definitions) and isinstance(definitions[j].value, _XdrPassthru):
+            while j < len(definitions) and isinstance(
+                definitions[j].value, _XdrPassthru
+            ):
                 lines.append(definitions[j].value.content)
                 j += 1
             merged = _XdrPassthru("\n".join(lines))
@@ -826,10 +828,67 @@ def _merge_consecutive_passthru(definitions: List[Definition]) -> List[Definitio
     return result
 
 
+def _meta_line(meta) -> int:
+    """Return the 1-based source line for a node's meta, or 0 if unknown"""
+    try:
+        return meta.line
+    except AttributeError:
+        return 0
+
+
+class XdrSemanticError(Exception):
+    """A specification that parses but violates an XDR semantic rule.
+
+    Detection lives in the language-independent front end because a
+    duplicate name is malformed XDR regardless of the output language.
+    """
+
+    def __init__(self, message: str, meta):
+        super().__init__(message)
+        self.message = message
+        self.line = _meta_line(meta)
+        self.column = getattr(meta, "column", 0)
+
+
+def _introduced_names(value):
+    """Yield (name, node) for each identifier a definition introduces."""
+    if isinstance(value, (_XdrStruct, _XdrUnion, _XdrPointer)):
+        yield value.name, value
+    elif isinstance(value, _XdrEnum):
+        yield value.name, value
+        for enumerator in value.enumerators:
+            yield enumerator.name, enumerator
+    elif isinstance(value, _XdrTypedef):
+        yield value.declaration.name, value.declaration
+    elif isinstance(value, _XdrConstant):
+        yield value.name, value
+
+
+def check_duplicate_definitions(root: "Specification") -> None:
+    """Reject a spec that declares an identifier more than once.
+
+    RFC 4506 Section 6.4 places constant and type identifiers in a
+    single name space that must be unique within a specification.
+    """
+    seen = {}
+    for definition in root.definitions:
+        for name, node in _introduced_names(definition.value):
+            where = node if node.line else definition.meta
+            first = seen.get(name)
+            if first is not None:
+                raise XdrSemanticError(
+                    f"duplicate identifier '{name}'"
+                    f" (first declared at line {_meta_line(first)})",
+                    where,
+                )
+            seen[name] = where
+
+
 def transform_parse_tree(parse_tree):
     """Transform productions into an abstract syntax tree"""
     ast = transformer.transform(parse_tree)
     ast.definitions = _merge_consecutive_passthru(ast.definitions)
+    check_duplicate_definitions(ast)
     return ast
 
 
diff --git a/tools/net/sunrpc/xdrgen/xdr_parse.py b/tools/net/sunrpc/xdrgen/xdr_parse.py
index 3e76717d2f85..78298553ee78 100644
--- a/tools/net/sunrpc/xdrgen/xdr_parse.py
+++ b/tools/net/sunrpc/xdrgen/xdr_parse.py
@@ -169,6 +169,26 @@ def handle_transform_error(e: VisitError, source: str, filename: str) -> None:
     sys.stderr.write("\n".join(msg_parts) + "\n")
 
 
+def handle_semantic_error(e, source: str, filename: str) -> None:
+    """Report a semantic error (e.g., a duplicate name) with context.
+
+    Args:
+        e: The XdrSemanticError carrying message and source position
+        source: The XDR source text being parsed
+        filename: The name of the file being parsed
+    """
+    lines = source.splitlines()
+    line_num = getattr(e, "line", 0)
+    column = getattr(e, "column", 0)
+    line_text = lines[line_num - 1] if 0 < line_num <= len(lines) else ""
+
+    msg_parts = [f"{filename}:{line_num}:{column}: semantic error", e.message]
+    if line_text:
+        msg_parts.extend(format_source_caret(line_text, column))
+
+    sys.stderr.write("\n".join(msg_parts) + "\n")
+
+
 def xdr_parser() -> Lark:
     """Return a Lark parser instance configured with the XDR language grammar"""
 
-- 
2.54.0

[PATCH 4/5] xdrgen: Enforce RFC 5531 name and number scoping for RPC programs

[Chuck Lever]

The duplicate-identifier check enforces the RFC 4506 name space
for XDR type and constant identifiers but ignores what an RPC
program definition adds. RFC 5531 Section 12.3 completes the
model: a program identifier shares the specification-wide name
space with constant and type identifiers, a version name and
number are unique within their program, and a procedure name and
number are unique within their version.

xdrgen currently accepts a specification that breaks any of these
rules, and the symptom depends on which rule. A duplicate procedure
name reaches the generated header as a redeclared enumerator,
which the C compiler rejects. A duplicate procedure number is
more dangerous because it is silent: the two procedures emit
enumerators of equal value -- valid C that compiles cleanly --
leaving a dispatch collision to surface only at run time. A
duplicate program name shares the specification-wide name space
with constants and types and is caught alongside them.

Extend the check to enforce RFC 5531 scoping in full.

Signed-off-by: Chuck Lever <cel@kernel.org>
---
 tools/net/sunrpc/xdrgen/xdr_ast.py | 49 ++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/tools/net/sunrpc/xdrgen/xdr_ast.py b/tools/net/sunrpc/xdrgen/xdr_ast.py
index cf68ff5dfe17..ec48506b239a 100644
--- a/tools/net/sunrpc/xdrgen/xdr_ast.py
+++ b/tools/net/sunrpc/xdrgen/xdr_ast.py
@@ -862,6 +862,50 @@ def _introduced_names(value):
         yield value.declaration.name, value.declaration
     elif isinstance(value, _XdrConstant):
         yield value.name, value
+    elif isinstance(value, _RpcProgram):
+        yield value.name, value
+
+
+def _check_rpc_scope_names(program: "_RpcProgram") -> None:
+    """Enforce RFC 5531 Section 12.3 scoping within an RPC program.
+
+    A version name and number are unique within the program and a
+    procedure name and number are unique within its version.
+    """
+    version_names = set()
+    version_numbers = set()
+    for version in program.versions:
+        if version.name in version_names:
+            raise XdrSemanticError(
+                f"duplicate version name '{version.name}'"
+                f" in program '{program.name}'",
+                version,
+            )
+        version_names.add(version.name)
+        if version.number in version_numbers:
+            raise XdrSemanticError(
+                f"duplicate version number {version.number}"
+                f" in program '{program.name}'",
+                version,
+            )
+        version_numbers.add(version.number)
+        procedure_names = set()
+        procedure_numbers = set()
+        for procedure in version.procedures:
+            if procedure.name in procedure_names:
+                raise XdrSemanticError(
+                    f"duplicate procedure name '{procedure.name}'"
+                    f" in version '{version.name}'",
+                    procedure,
+                )
+            procedure_names.add(procedure.name)
+            if procedure.number in procedure_numbers:
+                raise XdrSemanticError(
+                    f"duplicate procedure number {procedure.number}"
+                    f" in version '{version.name}'",
+                    procedure,
+                )
+            procedure_numbers.add(procedure.number)
 
 
 def check_duplicate_definitions(root: "Specification") -> None:
@@ -869,6 +913,9 @@ def check_duplicate_definitions(root: "Specification") -> None:
 
     RFC 4506 Section 6.4 places constant and type identifiers in a
     single name space that must be unique within a specification.
+    RFC 5531 Section 12.3 adds RPC program names to that name space
+    and scopes version names and numbers to their program and
+    procedure names and numbers to their version.
     """
     seen = {}
     for definition in root.definitions:
@@ -882,6 +929,8 @@ def check_duplicate_definitions(root: "Specification") -> None:
                     where,
                 )
             seen[name] = where
+        if isinstance(definition.value, _RpcProgram):
+            _check_rpc_scope_names(definition.value)
 
 
 def transform_parse_tree(parse_tree):
-- 
2.54.0

[PATCH 5/5] xdrgen: Reject out-of-range program, version, and procedure numbers

[Chuck Lever]

RFC 5531 assigns only unsigned constants to program, version, and
procedure numbers (Section 12.3) and encodes each as an unsigned
32-bit integer (Section 9), so a valid number falls within
[0, 2**32 - 1]. RFC 4506 Section 6.2 permits a signed decimal constant
for XDR constants in general and sets no ceiling on magnitude, so the
grammar accepts an out-of-range value without complaint. It reaches
generated code -- a negative procedure number emerges as an enumerator
such as "FOO = -5", valid C that compiles cleanly even though the wire
field is an unsigned 32-bit integer. Thus the xdrgen front end is the
only place that can reject the malformed value.

Extend the semantic checks to require each program, version, and
procedure number to fall within [0, 2**32 - 1].

Signed-off-by: Chuck Lever <cel@kernel.org>
---
 .../tests/bad-procedure-number-negative.x     | 20 ++++++
 .../tests/bad-procedure-number-too-large.x    | 20 ++++++
 .../tests/bad-program-number-negative.x       | 19 ++++++
 .../tests/bad-program-number-too-large.x      | 19 ++++++
 .../tests/bad-version-number-negative.x       | 19 ++++++
 .../tests/bad-version-number-too-large.x      | 19 ++++++
 tools/net/sunrpc/xdrgen/xdr_ast.py            | 64 ++++++++++++++++++-
 7 files changed, 177 insertions(+), 3 deletions(-)
 create mode 100644 tools/net/sunrpc/xdrgen/tests/bad-procedure-number-negative.x
 create mode 100644 tools/net/sunrpc/xdrgen/tests/bad-procedure-number-too-large.x
 create mode 100644 tools/net/sunrpc/xdrgen/tests/bad-program-number-negative.x
 create mode 100644 tools/net/sunrpc/xdrgen/tests/bad-program-number-too-large.x
 create mode 100644 tools/net/sunrpc/xdrgen/tests/bad-version-number-negative.x
 create mode 100644 tools/net/sunrpc/xdrgen/tests/bad-version-number-too-large.x

diff --git a/tools/net/sunrpc/xdrgen/tests/bad-procedure-number-negative.x b/tools/net/sunrpc/xdrgen/tests/bad-procedure-number-negative.x
new file mode 100644
index 000000000000..33ed272c3ce2
--- /dev/null
+++ b/tools/net/sunrpc/xdrgen/tests/bad-procedure-number-negative.x
@@ -0,0 +1,20 @@
+/*
+ * NEGATIVE TEST CASE -- xdrgen must REJECT this specification.
+ *
+ * RFC 5531 assigns only unsigned constants to program, version, and
+ * procedure numbers (Section 12.3). This spec gives a procedure a
+ * negative number, which the front end must reject.
+ *
+ * Expected diagnostic:
+ *   negative procedure number -5 in version 'BADVERS'
+ *
+ * The tests directory has no automated runner; exercise by hand:
+ *   ./xdrgen definitions tests/bad-procedure-number-negative.x   (must fail)
+ */
+
+program BADPROG {
+	version BADVERS {
+		void BADPROC_NULL(void) = 0;
+		void BADPROC_FOO(void)  = -5;
+	} = 1;
+} = 100000;
diff --git a/tools/net/sunrpc/xdrgen/tests/bad-procedure-number-too-large.x b/tools/net/sunrpc/xdrgen/tests/bad-procedure-number-too-large.x
new file mode 100644
index 000000000000..521581c57358
--- /dev/null
+++ b/tools/net/sunrpc/xdrgen/tests/bad-procedure-number-too-large.x
@@ -0,0 +1,20 @@
+/*
+ * NEGATIVE TEST CASE -- xdrgen must REJECT this specification.
+ *
+ * RFC 5531 encodes program, version, and procedure numbers as unsigned
+ * 32-bit integers (Section 9). This spec gives a procedure a number one
+ * past the 32-bit maximum, which the front end must reject.
+ *
+ * Expected diagnostic:
+ *   procedure number 4294967296 in version 'BADVERS' exceeds 4294967295
+ *
+ * The tests directory has no automated runner; exercise by hand:
+ *   ./xdrgen definitions tests/bad-procedure-number-too-large.x   (must fail)
+ */
+
+program BADPROG {
+	version BADVERS {
+		void BADPROC_NULL(void) = 0;
+		void BADPROC_FOO(void)  = 4294967296;
+	} = 1;
+} = 100000;
diff --git a/tools/net/sunrpc/xdrgen/tests/bad-program-number-negative.x b/tools/net/sunrpc/xdrgen/tests/bad-program-number-negative.x
new file mode 100644
index 000000000000..f7b71ee07f6c
--- /dev/null
+++ b/tools/net/sunrpc/xdrgen/tests/bad-program-number-negative.x
@@ -0,0 +1,19 @@
+/*
+ * NEGATIVE TEST CASE -- xdrgen must REJECT this specification.
+ *
+ * RFC 5531 assigns only unsigned constants to program, version, and
+ * procedure numbers (Section 12.3). This spec gives the program a
+ * negative number, which the front end must reject.
+ *
+ * Expected diagnostic:
+ *   negative program number -100000 in program 'BADPROG'
+ *
+ * The tests directory has no automated runner; exercise by hand:
+ *   ./xdrgen definitions tests/bad-program-number-negative.x   (must fail)
+ */
+
+program BADPROG {
+	version BADVERS {
+		void BADPROC_NULL(void) = 0;
+	} = 1;
+} = -100000;
diff --git a/tools/net/sunrpc/xdrgen/tests/bad-program-number-too-large.x b/tools/net/sunrpc/xdrgen/tests/bad-program-number-too-large.x
new file mode 100644
index 000000000000..c761584e712f
--- /dev/null
+++ b/tools/net/sunrpc/xdrgen/tests/bad-program-number-too-large.x
@@ -0,0 +1,19 @@
+/*
+ * NEGATIVE TEST CASE -- xdrgen must REJECT this specification.
+ *
+ * RFC 5531 encodes program, version, and procedure numbers as unsigned
+ * 32-bit integers (Section 9). This spec gives the program a number one
+ * past the 32-bit maximum, which the front end must reject.
+ *
+ * Expected diagnostic:
+ *   program number 4294967296 in program 'BADPROG' exceeds 4294967295
+ *
+ * The tests directory has no automated runner; exercise by hand:
+ *   ./xdrgen definitions tests/bad-program-number-too-large.x   (must fail)
+ */
+
+program BADPROG {
+	version BADVERS {
+		void BADPROC_NULL(void) = 0;
+	} = 1;
+} = 4294967296;
diff --git a/tools/net/sunrpc/xdrgen/tests/bad-version-number-negative.x b/tools/net/sunrpc/xdrgen/tests/bad-version-number-negative.x
new file mode 100644
index 000000000000..dd9c773435c0
--- /dev/null
+++ b/tools/net/sunrpc/xdrgen/tests/bad-version-number-negative.x
@@ -0,0 +1,19 @@
+/*
+ * NEGATIVE TEST CASE -- xdrgen must REJECT this specification.
+ *
+ * RFC 5531 assigns only unsigned constants to program, version, and
+ * procedure numbers (Section 12.3). This spec gives the version a
+ * negative number, which the front end must reject.
+ *
+ * Expected diagnostic:
+ *   negative version number -1 in program 'BADPROG'
+ *
+ * The tests directory has no automated runner; exercise by hand:
+ *   ./xdrgen definitions tests/bad-version-number-negative.x   (must fail)
+ */
+
+program BADPROG {
+	version BADVERS {
+		void BADPROC_NULL(void) = 0;
+	} = -1;
+} = 100000;
diff --git a/tools/net/sunrpc/xdrgen/tests/bad-version-number-too-large.x b/tools/net/sunrpc/xdrgen/tests/bad-version-number-too-large.x
new file mode 100644
index 000000000000..dd44f6eed564
--- /dev/null
+++ b/tools/net/sunrpc/xdrgen/tests/bad-version-number-too-large.x
@@ -0,0 +1,19 @@
+/*
+ * NEGATIVE TEST CASE -- xdrgen must REJECT this specification.
+ *
+ * RFC 5531 encodes program, version, and procedure numbers as unsigned
+ * 32-bit integers (Section 9). This spec gives the version a number one
+ * past the 32-bit maximum, which the front end must reject.
+ *
+ * Expected diagnostic:
+ *   version number 4294967296 in program 'BADPROG' exceeds 4294967295
+ *
+ * The tests directory has no automated runner; exercise by hand:
+ *   ./xdrgen definitions tests/bad-version-number-too-large.x   (must fail)
+ */
+
+program BADPROG {
+	version BADVERS {
+		void BADPROC_NULL(void) = 0;
+	} = 4294967296;
+} = 100000;
diff --git a/tools/net/sunrpc/xdrgen/xdr_ast.py b/tools/net/sunrpc/xdrgen/xdr_ast.py
index ec48506b239a..9dab8bc545b0 100644
--- a/tools/net/sunrpc/xdrgen/xdr_ast.py
+++ b/tools/net/sunrpc/xdrgen/xdr_ast.py
@@ -498,7 +498,7 @@ class _RpcProcedure(_XdrAst):
     """RPC procedure definition"""
 
     name: str
-    number: str
+    number: int
     argument: _XdrTypeSpecifier
     result: _XdrTypeSpecifier
 
@@ -508,7 +508,7 @@ class _RpcVersion(_XdrAst):
     """RPC version definition"""
 
     name: str
-    number: str
+    number: int
     procedures: List[_RpcProcedure]
 
 
@@ -517,7 +517,7 @@ class _RpcProgram(_XdrAst):
     """RPC program definition"""
 
     name: str
-    number: str
+    number: int
     versions: List[_RpcVersion]
 
 
@@ -933,11 +933,69 @@ def check_duplicate_definitions(root: "Specification") -> None:
             _check_rpc_scope_names(definition.value)
 
 
+# RFC 5531 (Section 9) encodes program, version, and procedure numbers
+# as unsigned 32-bit integers, so each must fall within [0, 2**32 - 1].
+_RPC_NUMBER_MAX = 2**32 - 1
+
+
+def _check_rpc_number(kind: str, number: int, scope: str, meta) -> None:
+    """Reject one RPC number that is negative or wider than 32 bits."""
+    if number < 0:
+        raise XdrSemanticError(
+            f"negative {kind} number {number} {scope}",
+            meta,
+        )
+    if number > _RPC_NUMBER_MAX:
+        raise XdrSemanticError(
+            f"{kind} number {number} {scope} exceeds {_RPC_NUMBER_MAX}",
+            meta,
+        )
+
+
+def check_rpc_number_range(root: "Specification") -> None:
+    """Reject an out-of-range program, version, or procedure number.
+
+    RFC 5531 assigns only unsigned constants to program, version, and
+    procedure numbers (Section 12.3) and encodes each as an unsigned
+    32-bit integer (Section 9). RFC 4506 Section 6.2 permits a signed
+    decimal constant for XDR constants in general and sets no ceiling on
+    magnitude, so the grammar accepts an out-of-range value; the range
+    is enforced here instead. The parser retains no per-version or
+    per-procedure source location, so a violation is reported against the
+    program definition.
+    """
+    for definition in root.definitions:
+        program = definition.value
+        if not isinstance(program, _RpcProgram):
+            continue
+        _check_rpc_number(
+            "program",
+            program.number,
+            f"in program '{program.name}'",
+            definition.meta,
+        )
+        for version in program.versions:
+            _check_rpc_number(
+                "version",
+                version.number,
+                f"in program '{program.name}'",
+                definition.meta,
+            )
+            for procedure in version.procedures:
+                _check_rpc_number(
+                    "procedure",
+                    procedure.number,
+                    f"in version '{version.name}'",
+                    definition.meta,
+                )
+
+
 def transform_parse_tree(parse_tree):
     """Transform productions into an abstract syntax tree"""
     ast = transformer.transform(parse_tree)
     ast.definitions = _merge_consecutive_passthru(ast.definitions)
     check_duplicate_definitions(ast)
+    check_rpc_number_range(ast)
     return ast
 
 
-- 
2.54.0