diff --git a/.github/workflows/premerge.yml b/.github/workflows/premerge.yml index 736fa91d..c468d8cc 100644 --- a/.github/workflows/premerge.yml +++ b/.github/workflows/premerge.yml @@ -20,10 +20,19 @@ jobs: - name: Install LLVM run: | - wget https://apt.llvm.org/llvm.sh - chmod +x llvm.sh - sudo ./llvm.sh 23 - sudo apt install libmlir-23-dev mlir-23-tools + # LLVM 23 has branched, so it lives in its own apt suite now. Upstream + # llvm.sh still maps 23 to the unversioned suite, which serves LLVM 24 + # snapshots, so add the release repository directly instead. + CODENAME=$(lsb_release -cs) + sudo mkdir -p /etc/apt/keyrings + wget -qO- https://apt.llvm.org/llvm-snapshot.gpg.key \ + | sudo tee /etc/apt/keyrings/apt.llvm.org.asc > /dev/null + echo "deb [signed-by=/etc/apt/keyrings/apt.llvm.org.asc] https://apt.llvm.org/${CODENAME}/ llvm-toolchain-${CODENAME}-23 main" \ + | sudo tee /etc/apt/sources.list.d/llvm-23.list > /dev/null + sudo apt-get update + # The snapshot packaging pulled in llvm-23-dev via libmlir-23-dev; the + # release packaging does not, and MLIRConfig.cmake needs LLVMConfig.cmake. + sudo apt-get install -y clang-23 lld-23 llvm-23-dev libmlir-23-dev mlir-23-tools - name: ccache uses: hendrikmuhs/ccache-action@v1.2 diff --git a/CLAUDE.md b/CLAUDE.md index cd406ece..2a8a229f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -15,12 +15,18 @@ This is an experimental Python 3.9-compatible interpreter implementation in C++. - GMP (GNU Multiple Precision library) - ICU (International Components for Unicode) -Install LLVM/MLIR on Ubuntu: +Install LLVM/MLIR on Ubuntu. LLVM 23 has branched, so it has its own apt suite; +`llvm.sh` still maps 23 to the unversioned suite (LLVM 24 snapshots), so add the +release repository directly: ```bash -wget https://apt.llvm.org/llvm.sh -chmod +x llvm.sh -sudo ./llvm.sh 23 all -sudo apt install libmlir-23-dev mlir-23-tools +CODENAME=$(lsb_release -cs) +sudo mkdir -p /etc/apt/keyrings +wget -qO- https://apt.llvm.org/llvm-snapshot.gpg.key \ + | sudo tee /etc/apt/keyrings/apt.llvm.org.asc > /dev/null +echo "deb [signed-by=/etc/apt/keyrings/apt.llvm.org.asc] https://apt.llvm.org/${CODENAME}/ llvm-toolchain-${CODENAME}-23 main" \ + | sudo tee /etc/apt/sources.list.d/llvm-23.list > /dev/null +sudo apt-get update +sudo apt-get install -y clang-23 lld-23 llvm-23-dev libmlir-23-dev mlir-23-tools ``` ### Build Commands diff --git a/integration/tests/class_scope_isolation.py b/integration/tests/class_scope_isolation.py new file mode 100644 index 00000000..68c0b75f --- /dev/null +++ b/integration/tests/class_scope_isolation.py @@ -0,0 +1,352 @@ +# A class body is outlined into its own function during lowering, so it must +# never reference an SSA value from the enclosing scope. Every class body ends +# by returning the __class__ cell, which is carried as a None constant in the +# Python dialect -- structurally identical to a module-level `x = None`. Before +# py.class was marked IsolatedFromAbove, CSE merged the two whenever the +# module-level constant dominated the class body, and the outlined function +# ended up returning a value defined in its parent: +# error: 'func.return' op using value defined outside the region +# which failed lowering and then crashed. The giveaway was that only the +# *second* class broke -- the first one's constant precedes the module-level +# one, so nothing dominates it. + + +class A: + pass + + +a = None + + +class B: + pass + + +assert a is None +assert A().__class__ is A +assert B().__class__ is B +assert A is not B + +# A function definition between the two behaves the same way: what matters is +# the module-level None, not what kind of definition precedes it. + + +def sandwiched(): + return 1 + + +b = None + + +class C: + pass + + +assert b is None +assert sandwiched() == 1 +assert C().__class__ is C + +# Several None-valued names interleaved with class definitions: each class body +# must still return its own class, not whichever constant happened to dominate. + +d = None + + +class D: + def which(self): + return "D" + + +e = None + + +class E: + def which(self): + return "E" + + +f = None + +assert d is None and e is None and f is None +assert D().which() == "D" +assert E().which() == "E" + +# Other constants that are equally shareable across regions. `True`/`0`/`""` +# never triggered the original crash, but they exercise the same merge path. + +g = True +h = 0 +i = "" + + +class F: + value = 1 + + +assert g is True and h == 0 and i == "" +assert F.value == 1 +assert F().__class__ is F + +# Class bodies that legitimately close over an outer name resolve it by name +# (load_deref/load_closure), never by SSA value, so isolation must not break +# inheritance or references to earlier module-level bindings. + +base_marker = None + + +class Base: + marker = "base" + + +class Derived(Base): + pass + + +assert base_marker is None +assert Derived.marker == "base" +assert issubclass(Derived, Base) +assert Derived().__class__ is Derived + +# The rest of this file pins down what IsolatedFromAbove does *not* forbid. +# The trait constrains the MLIR region (no SSA values from an enclosing region), +# not Python scoping: everything the class body needs from outside arrives +# either as an operand of py.class, evaluated in the enclosing scope before the +# body runs (decorators, bases, metaclass kwargs), or by *name* through +# $captures + load_deref/load_closure (free variables), exactly as CPython's +# separate class code object does it. + + +def free_variable_read(): + captured = "from-outer" + + class C: + value = captured + + return C + + +assert free_variable_read().value == "from-outer" + + +def method_default_from_enclosing_local(): + d = 42 + + class C: + def m(self, x=d): + return x + + return C + + +assert method_default_from_enclosing_local()().m() == 42 + + +def decorator_built_from_outer_value(): + tag = "tagged" + + def deco(cls): + cls.tag = tag + return cls + + @deco + class C: + pass + + return C + + +assert decorator_built_from_outer_value().tag == "tagged" + + +def base_computed_from_enclosing_local(): + class Base: + marker = "b" + + chosen = Base + + class Derived(chosen): + pass + + return Derived + + +assert base_computed_from_enclosing_local().marker == "b" + + +def metaclass_from_enclosing_local(): + class Meta(type): + pass + + m = Meta + + class C(metaclass=m): + pass + + return C + + +assert type(metaclass_from_enclosing_local()).__name__ == "Meta" + + +def comprehension_reading_enclosing_local(): + n = 3 + + class C: + items = [i for i in range(n)] + + return C + + +assert comprehension_reading_enclosing_local().items == [0, 1, 2] + + +def classes_capturing_a_loop_variable(): + out = [] + for i in range(3): + + class C: + idx = i + + out.append(C.idx) + return out + + +assert classes_capturing_a_loop_variable() == [0, 1, 2] + + +GLOBAL = "glob" + + +class UsesGlobal: + v = GLOBAL + + +assert UsesGlobal.v == "glob" + + +# The local names here deliberately avoid colliding with any module-level name +# in this file -- see the note below about the global-vs-captured-free-variable +# bug, which is unrelated to region isolation but would otherwise mask this case. +def two_levels_of_nesting(): + outer_word = "one" + + def inner(): + inner_word = "two" + + class C: + joined = outer_word + inner_word + + return C + + return inner() + + +assert two_levels_of_nesting().joined == "onetwo" + + +# A class nested directly inside another class body. Each py.class is lowered +# by its own run of ClassDefinitionOpLowering, and each body keeps its own +# py.class_return until then. The outer class used to rewrite *every* +# py.class_return in its subtree to func.return -- including the inner class's +# -- so by the time the inner class was lowered its terminator was gone and it +# tripped ASSERT(return_op) at FunctionPatterns.cpp. Every class here carries +# __class__ in cellvars, which is what selects that code path. + + +class Outer: + class Inner: + b = 1 + + +assert Outer.Inner.b == 1 +assert Outer.Inner().__class__ is Outer.Inner + + +def nested_class_in_function(): + n = 7 + + class Outer: + a = n + + class Inner: + b = n + 1 + + return Outer + + +assert nested_class_in_function().a == 7 +assert nested_class_in_function().Inner.b == 8 + + +class ThreeDeep: + class Middle: + class Innermost: + v = "deep" + + +assert ThreeDeep.Middle.Innermost.v == "deep" + + +# The same shape where the bodies actually use the __class__ cell, so the +# LoadClosureOp rewrite of the class_return operand runs for both classes +# rather than only being selected by the cellvars check. + + +class OuterSuper: + def who(self): + return "outer" + + class InnerSuper: + class Base: + def who(self): + return "base" + + class Derived(Base): + def who(self): + return "derived+" + super().who() + + +assert OuterSuper().who() == "outer" +assert OuterSuper.InnerSuper.Derived().who() == "derived+base" + + +# Sibling nested classes: the outer body holds more than one py.class_return +# in its subtree, so the walk has to skip each of them independently. + + +class TwoChildren: + class First: + tag = "first" + + class Second: + tag = "second" + + +assert TwoChildren.First.tag == "first" +assert TwoChildren.Second.tag == "second" + +# NOTE: one more case belongs here but hits a separate, pre-existing bug that +# reproduces identically on builds from before py.class was marked +# IsolatedFromAbove, so it is not a regression from region isolation: +# - a lambda in a class body closing over an enclosing function local +# (`k = 7; class C: f = lambda self: k`) raises NameError: name 'k' is not +# defined -- the free variable is not threaded through the class scope to +# the nested lambda. +# Add it here once it is fixed. +# +# A third, also pre-existing: when a name is *both* a module-level global and a +# captured free variable read by a nested function's class body, codegen aborts +# on TODO() at MLIRGenerator.cpp:327 (the store-name visibility lookup finds the +# symbol in neither the hidden nor the visible map). Minimal repro: +# b = None +# def f(): +# a = "one" +# def inner(): +# b = "two" +# class C: +# joined = a + b +# return C +# return inner() +# That is why two_levels_of_nesting() above uses distinctive local names. + +print("class_scope_isolation: ok") diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index e05609fa..2afd2319 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -302,7 +302,10 @@ target_link_libraries(python-cpp ) # LLVM backend -find_package(LLVM CONFIG 23) +# No version is requested here for the same reason as in executable/mlir: LLVM's +# config-version file wants an exact major.minor match. The MLIR subdirectory has +# already validated the major version by this point. +find_package(LLVM CONFIG) if(ENABLE_LLVM_BACKEND AND NOT LLVM_FOUND) message(FATAL_ERROR "Could not find LLVM in the local environment") elseif(ENABLE_LLVM_BACKEND AND LLVM_FOUND) diff --git a/src/executable/mlir/CMakeLists.txt b/src/executable/mlir/CMakeLists.txt index 3888d355..8e633477 100644 --- a/src/executable/mlir/CMakeLists.txt +++ b/src/executable/mlir/CMakeLists.txt @@ -1,5 +1,15 @@ -find_package(LLVM 23 REQUIRED CONFIG) -message(STATUS "Using LLVMConfig.cmake in: ${LLVM_DIR}") +# LLVMConfigVersion.cmake only accepts an exact major.minor match, so asking for +# "23" means "23.0" and rejects the 23.1.x releases. Search without a version, +# newest install first, and enforce the major version ourselves. +set(CMAKE_FIND_PACKAGE_SORT_ORDER NATURAL) +set(CMAKE_FIND_PACKAGE_SORT_DIRECTION DEC) +find_package(LLVM REQUIRED CONFIG) +if(LLVM_VERSION_MAJOR LESS 23) + message(FATAL_ERROR + "LLVM 23 or newer is required, found ${LLVM_PACKAGE_VERSION} in ${LLVM_DIR}. " + "Point CMake at a newer install with -DLLVM_DIR=/lib/cmake/llvm") +endif() +message(STATUS "Using LLVMConfig.cmake in: ${LLVM_DIR} (${LLVM_PACKAGE_VERSION})") find_package(MLIR CONFIG REQUIRED HINTS ${LLVM_DIR}/../lib/cmake/mlir) diff --git a/src/executable/mlir/Conversion/PythonToPythonBytecode/FunctionPatterns.cpp b/src/executable/mlir/Conversion/PythonToPythonBytecode/FunctionPatterns.cpp index 357426d5..232fc2a1 100644 --- a/src/executable/mlir/Conversion/PythonToPythonBytecode/FunctionPatterns.cpp +++ b/src/executable/mlir/Conversion/PythonToPythonBytecode/FunctionPatterns.cpp @@ -126,6 +126,23 @@ namespace { } }; + // Collects the py.class_return ops that terminate `body` itself, skipping + // nested func.func / py.class subtrees. + std::vector collect_own_class_returns(mlir::Region &body) + { + std::vector returns; + body.walk([&returns](mlir::Operation *child_op) { + if (mlir::isa(child_op)) { + return WalkResult::skip(); + } + if (auto cr = mlir::dyn_cast(child_op)) { + returns.push_back(cr); + } + return WalkResult::advance(); + }); + return returns; + } + struct ClassDefinitionOpLowering : public mlir::OpRewritePattern { using OpRewritePattern::OpRewritePattern; @@ -146,6 +163,8 @@ namespace { class_fn_definition->setAttr("is_class", rewriter.getBoolAttr(true)); + auto class_returns = collect_own_class_returns(op.getBody()); + if (auto cellvars = op->getAttrOfType("cellvars")) { auto cell_names = cellvars.getValue(); if (std::find_if(cell_names.begin(), @@ -155,18 +174,8 @@ namespace { }) != cell_names.end()) { - mlir::py::ClassReturnOp return_op; - op.getBody().walk([&return_op](mlir::Operation *child_op) { - if (mlir::isa(child_op)) { - return WalkResult::skip(); - } - if (auto cr = mlir::dyn_cast(child_op)) { - return_op = cr; - return WalkResult::interrupt(); - } - return WalkResult::advance(); - }); - ASSERT(return_op); + ASSERT(!class_returns.empty()); + auto return_op = class_returns.front(); ASSERT(return_op->getParentOp() == op.getOperation()); ASSERT(return_op.getValue().getDefiningOp()); rewriter.setInsertionPoint(return_op.getValue().getDefiningOp()); @@ -181,14 +190,14 @@ namespace { attr.insert(attr.end(), op->getAttrs().begin(), op->getAttrs().end()); class_fn_definition->setAttrs(attr); - // Convert all py.class_return ops in the class body to - // func.return so that the body, once inlined into the - // synthesised func.func, has a valid terminator. - op.getBody().walk([&rewriter](mlir::py::ClassReturnOp cr) { + // Convert this class's own py.class_return ops to func.return so + // that the body, once inlined into the synthesised func.func, has + // a valid terminator. Nested classes are left alone. + for (auto cr : class_returns) { rewriter.setInsertionPoint(cr); rewriter.replaceOpWithNewOp( cr, mlir::ValueRange{ cr.getValue() }); - }); + } auto *end = class_fn_definition.addEntryBlock(); rewriter.setInsertionPointToStart(end); diff --git a/src/executable/mlir/Dialect/Python/IR/PythonOps.td b/src/executable/mlir/Dialect/Python/IR/PythonOps.td index ec9d83aa..88449fda 100644 --- a/src/executable/mlir/Dialect/Python/IR/PythonOps.td +++ b/src/executable/mlir/Dialect/Python/IR/PythonOps.td @@ -585,7 +585,8 @@ def ImportAllOp : Python_Op<"import_all"> { let arguments = (ins Python_PyObjectType:$module); } -def ClassDefinitionOp : Python_Op<"class", [AttrSizedOperandSegments]> { +def ClassDefinitionOp : Python_Op<"class", [AttrSizedOperandSegments, + IsolatedFromAbove]> { let summary = "Class definition"; // captures is a list of free-variable names — no tensor semantics,