From 060841927ea665024c754d0525f745b50e1e2b0b Mon Sep 17 00:00:00 2001 From: Tim Grant Date: Wed, 4 Mar 2026 10:32:29 -0700 Subject: [PATCH 1/3] [PATCH] Implement += and friends so that they evaluate both arguments exactly once. This fix was aided by claude-4.6-opus. --- src/liboslcomp/ast.cpp | 24 ++++++++++++++++++------ src/liboslcomp/ast.h | 6 ++++++ src/liboslcomp/codegen.cpp | 28 +++++++++++++++++++++++----- src/liboslcomp/typecheck.cpp | 10 +++++++++- 4 files changed, 56 insertions(+), 12 deletions(-) diff --git a/src/liboslcomp/ast.cpp b/src/liboslcomp/ast.cpp index 6d0e4a8580..ff82393b49 100644 --- a/src/liboslcomp/ast.cpp +++ b/src/liboslcomp/ast.cpp @@ -1044,12 +1044,6 @@ ASTassign_expression::ASTassign_expression(OSLCompilerImpl* comp, ASTNode* var, Operator op, ASTNode* expr) : ASTNode(assign_expression_node, comp, op, var, expr) { - if (op != Assign) { - // Rejigger to straight assignment and binary op - m_op = Assign; - m_children[1] = new ASTbinary_expression(comp, op, var, expr); - } - check_symbol_writeability(var); } @@ -1178,6 +1172,24 @@ ASTbinary_expression::ASTbinary_expression(OSLCompilerImpl* comp, Operator op, +ASTbinary_expression::ASTbinary_expression(OSLCompilerImpl* comp, Operator op, + ASTNode::ref left, + ASTNode::ref right) + : ASTNode(binary_expression_node, comp, op) +{ + addchild(left); + addchild(right); + // Check for a user-overloaded function for this operator. + // Disallow a few ops from overloading. + if (op != And && op != Or) { + ustring funcname = ustring::fmtformat("__operator__{}__", opword()); + Symbol* sym = comp->symtab().find(funcname); + if (sym && sym->symtype() == SymTypeFunction) + m_function_overload = (FunctionSymbol*)sym; + } +} + + ASTNode* ASTbinary_expression::make(OSLCompilerImpl* comp, Operator op, ASTNode* left, ASTNode* right) diff --git a/src/liboslcomp/ast.h b/src/liboslcomp/ast.h index b9f94a8da8..bcb1c676bd 100644 --- a/src/liboslcomp/ast.h +++ b/src/liboslcomp/ast.h @@ -827,6 +827,12 @@ class ASTbinary_expression final : public ASTNode { ASTbinary_expression(OSLCompilerImpl* comp, Operator op, ASTNode* left, ASTNode* right); + // this constructor is used to typecheck the compound assignent (+= and friends) + // it's almost identical to the normal one, except that because it takes ref's + // it doesn't destroy the children unless it's the right thing to do + ASTbinary_expression(OSLCompilerImpl* comp, Operator op, ASTNode::ref left, + ASTNode::ref right); + // Special consructor wrapper that can collapse ops between literals static ASTNode* make(OSLCompilerImpl* comp, Operator op, ASTNode* left, ASTNode* right); diff --git a/src/liboslcomp/codegen.cpp b/src/liboslcomp/codegen.cpp index 1c3be53c2b..68b56b0889 100644 --- a/src/liboslcomp/codegen.cpp +++ b/src/liboslcomp/codegen.cpp @@ -465,9 +465,9 @@ ASTcompound_initializer::codegen(Symbol* sym) Symbol* ASTassign_expression::codegen(Symbol* dest) { - OSL_DASSERT(m_op == Assign); // all else handled by binary_op - ASTindex* index = NULL; + Symbol *ind = NULL, *ind2 = NULL, *ind3 = NULL; + if (var()->nodetype() == index_node) { // Assigning to an individual component or array element index = (ASTindex*)var().get(); @@ -482,7 +482,24 @@ ASTassign_expression::codegen(Symbol* dest) dest = var()->codegen(); } - Symbol* operand = expr()->codegen(dest); + // For compound assignments (+=, *=, etc.), read the current LHS value, + // apply the binary op, and use the result as the operand for plain assign. + // Index expressions are evaluated once and reused for the write because they + // are the only ones at the moment that could have side effects. + Symbol* operand; + if (m_op != Assign) { + Symbol* lhs_read; + if (index) { + lhs_read = index->codegen(NULL, ind, ind2, ind3); + } else { + lhs_read = var()->codegen(); + } + Symbol* rhs = expr()->codegen(); + operand = m_compiler->make_temporary(typespec()); + emitcode(opword(), operand, lhs_read, rhs); + } else { + operand = expr()->codegen(dest); + } OSL_DASSERT(operand != NULL); if (typespec().is_structure_array()) { @@ -500,7 +517,8 @@ ASTassign_expression::codegen(Symbol* dest) // Assignment of struct copies each element individually if (operand != dest) { StructSpec* structspec = typespec().structspec(); - Symbol* arrayindex = index ? index->index()->codegen() : NULL; + Symbol* arrayindex = index ? (ind ? ind : index->index()->codegen()) + : NULL; if (arrayindex) { // Special case -- assignment to a element of an array of // structs. Beware the temp that may have been created above, @@ -528,7 +546,7 @@ ASTassign_expression::codegen(Symbol* dest) } if (index) { - index->codegen_assign(operand); + index->codegen_assign(operand, ind, ind2, ind3); dest = operand; // so transitive assignment works for array refs } else if (operand != dest) { emitcode(typespec().is_array() ? "arraycopy" : "assign", dest, operand); diff --git a/src/liboslcomp/typecheck.cpp b/src/liboslcomp/typecheck.cpp index 919266647f..f770e1928a 100644 --- a/src/liboslcomp/typecheck.cpp +++ b/src/liboslcomp/typecheck.cpp @@ -300,7 +300,15 @@ ASTassign_expression::typecheck(TypeSpec /*expected*/) return TypeSpec(); } - OSL_DASSERT(m_op == Assign); // all else handled by binary_op + // For compound assignments (+=, *=, etc. call it @=) we need to + // validate that the binary op @ is applicable to var() and expr(), + // and then that its result can be assigned into var(). + + if (m_op != Assign) { + ASTbinary_expression bin(m_compiler, Operator(m_op), var(), expr()); + et = bin.typecheck(vt); + } + ustring varname; if (var()->nodetype() == variable_ref_node) varname = ((ASTvariable_ref*)var().get())->name(); From f341c8db9c2c65a2f012fa0e7e0762dbbc5d9d7a Mon Sep 17 00:00:00 2001 From: Tim Grant Date: Wed, 4 Mar 2026 10:33:39 -0700 Subject: [PATCH 2/3] [PATCH] Update the reference output for the debug-uninit test. --- testsuite/debug-uninit/ref/out.txt | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/testsuite/debug-uninit/ref/out.txt b/testsuite/debug-uninit/ref/out.txt index a0908b811a..36a42543c0 100644 --- a/testsuite/debug-uninit/ref/out.txt +++ b/testsuite/debug-uninit/ref/out.txt @@ -5,8 +5,8 @@ ERROR: Detected possible use of uninitialized value in int i_uninit at test.osl: ERROR: Detected possible use of uninitialized value in float f_uninit at test.osl:14 (group unnamed_group_1, layer 0 test_0, shader test, op 5 'color', arg 1) ERROR: Detected possible use of uninitialized value in string s_uninit at test.osl:15 (group unnamed_group_1, layer 0 test_0, shader test, op 6 'texture', arg 1) ERROR: [RendererServices::texture] ImageInput::create() called with no filename -ERROR: Detected possible use of uninitialized value in float[3] A at test.osl:22 (group unnamed_group_1, layer 0 test_0, shader test, op 12 'aref', arg 1) -ERROR: Detected possible use of uninitialized value in color C at test.osl:28 (group unnamed_group_1, layer 0 test_0, shader test, op 17 'compref', arg 1) -ERROR: Detected possible use of uninitialized value in matrix M at test.osl:34 (group unnamed_group_1, layer 0 test_0, shader test, op 22 'mxcompref', arg 1) -ERROR: Detected possible use of uninitialized value in float[20] knots3 at test.osl:54 (group unnamed_group_1, layer 0 test_0, shader test, op 49 'spline', arg 4) -ERROR: Detected possible use of uninitialized value in float[20] knots4 at test.osl:55 (group unnamed_group_1, layer 0 test_0, shader test, op 51 'splineinverse', arg 4) +ERROR: Detected possible use of uninitialized value in float[3] A at test.osl:22 (group unnamed_group_1, layer 0 test_0, shader test, op 14 'aref', arg 1) +ERROR: Detected possible use of uninitialized value in color C at test.osl:28 (group unnamed_group_1, layer 0 test_0, shader test, op 21 'compref', arg 1) +ERROR: Detected possible use of uninitialized value in matrix M at test.osl:34 (group unnamed_group_1, layer 0 test_0, shader test, op 28 'mxcompref', arg 1) +ERROR: Detected possible use of uninitialized value in float[20] knots3 at test.osl:54 (group unnamed_group_1, layer 0 test_0, shader test, op 58 'spline', arg 4) +ERROR: Detected possible use of uninitialized value in float[20] knots4 at test.osl:55 (group unnamed_group_1, layer 0 test_0, shader test, op 61 'splineinverse', arg 4) From e1a011392ce4362efde2a9604cf46583cb8984d2 Mon Sep 17 00:00:00 2001 From: Tim Grant Date: Wed, 4 Mar 2026 11:09:10 -0700 Subject: [PATCH 3/3] [PATCH] Add a new test for compound assignment. --- src/cmake/testing.cmake | 1 + testsuite/compound-assignment/BATCHED | 0 testsuite/compound-assignment/OPTIX | 0 testsuite/compound-assignment/ref/out.txt | 7 +++++++ testsuite/compound-assignment/run.py | 7 +++++++ testsuite/compound-assignment/test.osl | 23 +++++++++++++++++++++++ 6 files changed, 38 insertions(+) create mode 100644 testsuite/compound-assignment/BATCHED create mode 100644 testsuite/compound-assignment/OPTIX create mode 100644 testsuite/compound-assignment/ref/out.txt create mode 100755 testsuite/compound-assignment/run.py create mode 100644 testsuite/compound-assignment/test.osl diff --git a/src/cmake/testing.cmake b/src/cmake/testing.cmake index 5b5b4327d7..02b3a34c8a 100644 --- a/src/cmake/testing.cmake +++ b/src/cmake/testing.cmake @@ -281,6 +281,7 @@ macro (osl_add_all_tests) color color2 color4 color-reg colorspace comparison complement-reg compile-buffer compassign-bool compassign-reg component-range + compound-assignment control-flow-reg connect-components const-array-params const-array-fill debugnan debug-uninit diff --git a/testsuite/compound-assignment/BATCHED b/testsuite/compound-assignment/BATCHED new file mode 100644 index 0000000000..e69de29bb2 diff --git a/testsuite/compound-assignment/OPTIX b/testsuite/compound-assignment/OPTIX new file mode 100644 index 0000000000..e69de29bb2 diff --git a/testsuite/compound-assignment/ref/out.txt b/testsuite/compound-assignment/ref/out.txt new file mode 100644 index 0000000000..12ebc3fc86 --- /dev/null +++ b/testsuite/compound-assignment/ref/out.txt @@ -0,0 +1,7 @@ +Compiled test.osl -> test.oso +i: 0, j: 0 +i: 1, j: 1 +i: 2, j: 2 +i: 3, j: 3 +a: {2, 4, 6, 8, 0, 0, 0, 0} + diff --git a/testsuite/compound-assignment/run.py b/testsuite/compound-assignment/run.py new file mode 100755 index 0000000000..6836d5554b --- /dev/null +++ b/testsuite/compound-assignment/run.py @@ -0,0 +1,7 @@ +#!/usr/bin/env python + +# Copyright Contributors to the Open Shading Language project. +# SPDX-License-Identifier: BSD-3-Clause +# https://github.com/AcademySoftwareFoundation/OpenShadingLanguage + +command = testshade("test") diff --git a/testsuite/compound-assignment/test.osl b/testsuite/compound-assignment/test.osl new file mode 100644 index 0000000000..eca77fcfac --- /dev/null +++ b/testsuite/compound-assignment/test.osl @@ -0,0 +1,23 @@ +// Copyright Contributors to the Open Shading Language project. +// SPDX-License-Identifier: BSD-3-Clause +// https://github.com/AcademySoftwareFoundation/OpenShadingLanguage + +// Test assigning to float from int without temporary works properly. +// Should really validate the generated oso too. + +shader +test() +{ + int a[8] = { 1, 2, 3, 4, 0, 0, 0, 0 }; + int b[4] = { 1, 2, 3, 4 }; + int i = 0, j = 0; + for (int k = 0; k < 4; ++k) { + printf("i: %d, j: %d\n", i, j); + a[i++] += b[j++]; + } + + // Print all elements of 'a' to make sure we didn't write + // outside of the scan + printf("a: {%d, %d, %d, %d, %d, %d, %d, %d}\n", a[0], a[1], a[2], a[3], + a[4], a[5], a[6], a[7]); +}