fix(batched): Make sure to promote even zero-initialized closures to varying.

sfriedmapixar commented 3 months ago

Description

Fixed a bug in the batched analysis of varying vs uniform that could miss promoting zero-initialized closures to varying. Because the rest of the JIT code assumes closures are always varying, we simply mark all closures we see as varying in the analysis phase.

Tests

I encountered this in a complex production case, but couldn't manage to duplicate it with small changes to the closure-zero tests in the test-suite. So I know it resolves the issue, but would welcome input on how to craft a small test to trigger it.

Checklist:

[X] I have read the contribution guidelines.
[X] I have updated the documentation, if applicable.
[ ] I have ensured that the change is tested somewhere in the testsuite (adding new test cases if necessary).
[X] My code follows the prevailing code style of this project. If I haven't already run clang-format v17 before submitting, I definitely will look at the CI test that runs clang-format and fix anything that it highlights as being nonconforming.

AlexMWells commented 3 months ago

Lets hold this one until I review it more deeply. Not sure that is the right place to mark it varying, as all the dependencies are not setup yet. I think now we might rely on not triggering the recursive marking until all other analysis is complete.

AlexMWells commented 3 months ago

Doesn't the existing function cover this scenario already:


   OSL_NOINLINE void make_closures_varying()
    {
        OSL_DEV_ONLY(std::cout << "make_closures_varying begin" << std::endl);
        // We assume that closures are always stored as varying pointers
        FOREACH_SYM(Symbol & s, inst())
        {
            if (s.typespec().is_closure_based()) {
                OSL_DEV_ONLY(std::cout << "closure symbol " << s.name().c_str()
                                       << " marked varying." << std::endl);
                recursively_mark_varying(&s);
            }
        }
        OSL_DEV_ONLY(std::cout << "make_closures_varying end" << std::endl);
    }

sfriedmapixar commented 3 months ago

Hmmm...possibly. I tested on a production case and didn't notice you'd made that change upstream of what we have locally. I need to go dig up that case again and retry with this.

sfriedmapixar commented 3 months ago

OK, I was able to dig up my test cases, and trying again with the updated code Alex mentioned does indeed fix the crash. After discussing it with him, the current fix is much better than this pull request, so closing this pull request.

AcademySoftwareFoundation / OpenShadingLanguage