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

out parameter diagnostics affect uninitialized variables at the _call site_ #5093

Closed
MarijnS95 opened this issue Mar 9, 2023 · 2 comments · Fixed by #5094
Closed

out parameter diagnostics affect uninitialized variables at the _call site_ #5093

MarijnS95 opened this issue Mar 9, 2023 · 2 comments · Fixed by #5094

Comments

@MarijnS95
Copy link
Contributor

A recent PR to prevent leaving out parameters uninitialized in function calls (#5047) seems to have escaped its cage and is now also disallowing uninitialized arguments into functions with out paramameters. See for example this (IMO totally valid) Interlocked* invocation:

        uint original;
        buffer.InterlockedAdd(offset, value, original);
        return original;
bindless.hlsl:110:46: error: variable 'original' is uninitialized when used here [-Werror,-Wuninitialized]
        buffer.InterlockedAdd(offset, value, original);
                                             ^~~~~~~~
bindless.hlsl:109:22: note: initialize the variable 'original' to silence this warning
        uint original;
                     ^
                      = 0

(Compiler from 667fb77)

I thought the whole point of out parameters and by extension this diagnostic is to know and force that the function will initialize original here, and it should thus not need to be pre-initialized by the caller.

Originally posted by @MarijnS95 in #5047 (comment), cc @llvm-beanz

@llvm-beanz
Copy link
Collaborator

Just going to drop a full-source example here:

RWByteAddressBuffer buffer;

void interlockWrapper(out uint original) {
  buffer.InterlockedAdd(16, 1, original);
}

llvm-beanz added a commit to llvm-beanz/DirectXShaderCompiler that referenced this issue Mar 10, 2023
Initially the change here was forcing unannotated parameters to be
treated as uses (`in` parameters). Instead this changes to not handle
them explicitly.

Most `in` parameter uses should produce `LValueToRValue` casts which
will be identified as uses through other means, but by removing this
special case we can support unannotated AST nodes from generated ASTs
through this analysis based on their type and usage alone.

Fixes microsoft#5093
@llvm-beanz
Copy link
Collaborator

Something to also be aware of here. -Wuninitialized in DXC does cause spurious warnings around struct types too. We also have issue #2494, which is caused by our bad AST hygiene.

llvm-beanz added a commit that referenced this issue Mar 10, 2023
Initially the change here was forcing unannotated parameters to be
treated as uses (`in` parameters). Instead this changes to not handle
them explicitly.

Most `in` parameter uses should produce `LValueToRValue` casts which
will be identified as uses through other means, but by removing this
special case we can support unannotated AST nodes from generated ASTs
through this analysis based on their type and usage alone.

Fixes #5093
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