Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Anderson2021 autoscheduler plays dubious games with unique_ptr<> #7703

Closed
steven-johnson opened this issue Jul 24, 2023 · 0 comments · Fixed by #7706
Closed

Anderson2021 autoscheduler plays dubious games with unique_ptr<> #7703

steven-johnson opened this issue Jul 24, 2023 · 0 comments · Fixed by #7706
Assignees

Comments

@steven-johnson
Copy link
Contributor

While investigating #7606 I noticed this code in the Anderson2021 codebase:

std::unique_ptr<ThreadInfo> GPULoopInfo::create_thread_info() {
    internal_assert(at_or_inside_block());
    internal_assert(at_or_inside_thread());

    auto max_thread_counts = current_block_loop->get_union_thread_counts(nullptr);
    std::unique_ptr<ThreadInfo> new_thread_info = std::make_unique<ThreadInfo>(
        current_thread_loop->vectorized_loop_index,
        current_thread_loop->size,
        current_thread_loop->stage->loop,
        max_thread_counts);
    thread_info = new_thread_info.get();
    return new_thread_info;
}

since thread_info is a member of this, and declared as a plain const ThreadInfo*, after this call, it refers to an unowned pointer... the ownership is in the returned unique_ptr. Since the only two call sites immediately assign the result to a local unique_ptr, this member var is guaranteed to be stale at the end of the calling routine.

I'm not sure what the right fix is here because I don't know the intended semantics -- shared_ptr might be appropriate, but it's not clear to me whether the returned result is intended to be mutable or not, or what constraints there are on mutability.

The current code looks absolutely unsafe, however, and needs fixing; if there are similar code patterns elsewhere in this codebase, they need examination as well.

steven-johnson added a commit that referenced this issue Jul 25, 2023
steven-johnson added a commit that referenced this issue Aug 1, 2023
* Attempt to fix #7703

* fixes

* Update LoopNest.cpp

* Update GPULoopInfo.h

* Fixes.

* clang-tidy
ardier pushed a commit to ardier/Halide-mutation that referenced this issue Mar 3, 2024
* Attempt to fix halide#7703

* fixes

* Update LoopNest.cpp

* Update GPULoopInfo.h

* Fixes.

* clang-tidy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants