No special case for unannotated params

[llvm-beanz]

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

[bogner]

lg pending some editing in the comments and maybe another test case

[bogner]

This comment is confusing. Isn't the reason there's no diagnostic because it is annotated with parameter annotations?

[llvm-beanz]

Confusingly and probably incorrectly it isn’t annotated. My comment lacks sufficient details here. This change ignores unannotated parameters and instead leaves it up to the rules for C/C++, which ignores values used as pass-by-reference parameters.

[tex3d]

Yeah, that is confusing. I would think that the reason there's no diagnostic is that the third parameter is annotated with out, like an ordinary case where no warning should be emitted (and where we should have a test for that as you pointed out above). Here's the intrinsic definition:

void [[]] InterlockedAdd64(in uint byteOffset, in u64 value, out any_int64 original) : interlockedadd_immediate;

Or is it the case that even though this has out in the intrinsic definition, it doesn't add the HLSLOutAttr to the parameter decl like an ordinary HLSL function would, yet we still need to test to ensure that it doesn't think this is a use of the uninitialized value?

[llvm-beanz]

Yes, we don’t translate the annotations from the intrinsic definitions to the annotations in the AST, we just use them to generate the qual types for the arguments. InterlockedAdd ends up with a signature something like InterlockedAdd(int, int, int && __restrict), with no HLSL annotations on the ParamVarDecls

I don’t think many of these are generating correct AST representations, but since they’re completely missing the HLSL annotations the best thing we can do is rely on C/C++ rules.

[bogner]

Is this comment stale? I don't think it describes what's happening accurately even before your change.

[tex3d]

Is in added elsewhere by default for user function params?

[llvm-beanz]

No, this only covers explicit annotations. Pass-by-value parameters (including explicitly annotated in) should automatically be treated as uses because they’ll pretty much always have an LValueToRValue cast which is a use.

Thinking on it we need the in check here is to cover the case of in out vs inout, but otherwise we wouldn’t need to check in at all.

[AppVeyorBot]

✅ Build DirectXShaderCompiler 1.0.2930 completed (commit d5beffb9a2 by @llvm-beanz)

[AppVeyorBot]

✅ Build DirectXShaderCompiler 1.0.2931 completed (commit 18d547e86f by @llvm-beanz)

[MarijnS95]

Many thanks, this solves the issue on our end! However, it also surfaces a bunch of issues on the following pattern (that I discarded yesterday by writing in #5093 that the out parameter should/could be relied on to initialize the argument at the call site):

bool GetOptionalThing(out float result) {
    if (...) {
        result = ...;
        return true;
    }
    return false;
}

with DXC obviously complaining that result is left uninitialized at return false;.

How should this pattern be implemented in HLSL? I worked around the warning by adding result = result; but that seems wrong given that it's an out, not an inout. In most of these cases I think we expect result to already contain a sensible value that we do not want to overwrite, perhaps it should just be an inout EDIT: that doesn't apply everywhere, inout causes more/different problems for us.

[llvm-beanz]

Many thanks, this solves the issue on our end!

Glad to be helpful 😄

However, it also surfaces a bunch of issues on the following pattern (that I discarded yesterday by writing in #5093 that the out parameter should/could be relied on to initialize the argument at the call site)

Now you're hitting the new warning exactly as intended.

How should this pattern be implemented in HLSL?

This is the fun bit. The reason we introduced this issue is because people were misusing out parameters and relying on undefined behavior in DXC. HLSL parameter passing is copy-in/copy-out. Input parameters (either marked with no keyword, the in keyword or the inout keyword) are initialized into the function with the value provided from the call site. Output parameters (either marked inout or out are copied back the the variable provided at the call site after the function executes. For a code illustration of how inout parameter passing works in HLSL, take a look at this example (spoiler alert: it probably isn't what you expect). The only difference between inout and out is that for inout parameters the value from the call site is propagated into the function.

DXC in order to avoid redundant copies tries to optimize the code by passing in the address of the variable you want to store the end result to. We found cases where DXC was doing that unsafely, which I fixed back in January. Fixing those cases surfaced bugs in shaders where they were relying either on the unsafe optimization or outright undefined behavior in DXC. This code illustrates a (trivial) case where the optimization is unsafe (because the memory aliases), and the exposed undefined behavior made an invalid shader seem valid.

These new warnings surface the undefined behavior to the user in a more actionable and identifiable way. Rather than a validation error, or a rendering glitch, we tell you exactly where the uninitialized value is propagating from.

The important point here for out parameters is that out parameters are not initialized with the value from the variable at the call site, and they are defined to always copy back on top of the provided variable. Because copying back an uninitialized value is undefined behavior in HLSL, the compiler will sometimes (but not always) give you the behavior you might be expecting because we optimize away the copy.

I worked around the warning by adding result = result; but that seems wrong given that it's an out, not an inout.

For an out parameter this will silence the warning, but not correct the undefined behavior, so be careful.

In most of these cases I think we expect result to already contain a sensible value that we do not want to overwrite, perhaps it should just be an inout EDIT: that doesn't apply everywhere, inout causes more/different problems for us.

If you expect the value to have a sensible result and to not overwrite it, you should make it an inout parameter in order to preserve the initial state value. If it is not, you should initialize the value in all control flow paths for a function otherwise you risk propagating undefined behavior out of the function.

If you just want the warning to go away and are willing to get surprised along the way you have three options:

1. Self-assigning the variable tells the analysis to ignore the variable (as you discovered).
2. If an out variable is completely unused in a function, but needs to exist in the signature for... some reason... (there are some patterns with macros, templates, or shader entry functions where that is required), you can mark the parameter [maybe_unused] which will silence only the "always uninitialized" warning, not the "sometimes uninitialized" warning.
3. You can pass -Wno-parameter-usage to silence the whole category of warnings.

[llvm-beanz]

One last note worth sharing, these warning conditions are hard errors in FXC (see: FXC example).

Because this behavior has been allowed in DXC forever, we decided to make it a default-enabled warning instead of an error.

[MarijnS95]

@llvm-beanz thanks for clarifying that in such a super-detailed manner 🤩! I don't want the warnings to go away at all but rather solve them correctly, less UB means less time spent debugging (we code in Rust for a reason, too). Sounds like I'll be determining on a case-by-case basis whether to use inout or rearchitect the APIs a bit.

[tex3d]

Looks good!

[MarijnS95]

Self-assigning the variable tells the analysis to ignore the variable (as you discovered).

In hindsight, is this intentional? I'd have a similar "uninitialized" error when using the out parameter on the right of =, and this seems like a poor/cheaty way to get out of the undefined-behaviour check without actually addressing undefined behaviour.