Assert in template + out functions when it has local variables

[MarijnS95]

Unlike #5273 this seems much stranger in origin. The following sample fails to compile with the error below:

dxc -E main -T cs_6_6 repro.hlsl -HV 2021

template <typename R>
void test(R x, out uint result) {
    uint repro = 0;
    result = 10;
}

[numthreads(32, 32, 1)] void main(uint2 threadId: SV_DispatchThreadID) {
    uint x;
    test(10, x);
}

dxc: tools/clang/lib/Analysis/UninitializedValues.cpp:232: ValueVector::reference (anonymous namespace)::CFGBlockValues::operator[](const clang::VarDecl *): Assertion `idx.hasValue()' failed.

And this assert is "solved" by either:

1. Removing and inlining the template (replacing R with uint);
2. Removing the unused uint repro = 0; variable. Note that using it in e.g. result = 10 + repro; does not get rid of the assert!;
3. Replacing out with inout.

Keep in mind that I did hack in a:

  if (!idx.hasValue()) {
    const auto &astContext = vd->getASTContext();
    auto str = vd->getLocation().printToString(astContext.getSourceManager());
    printf("Here: %s\n", str.c_str());
  }

bit of code to get the compiler to tell me where in our massive shader + #include chain I had to start looking to provide you with a repro (similar to the request for emitError() in #5273). Is there anything I/we can do to streamline this process?

Likewise I wouldn't have found this error without creating a release build of the compiler with -DLLVM_ENABLE_ASSERTIONS=ON, and would like to discuss enabling assertions by default. I have no clue what kind of valid/invalid shader the compiler might have emitted otherwise (or is already emitting, since I am updating our shader compiler and enabling this as a safety guard, but our current older compiler without assertions is accepting this shader just fine: more details in the first paragraph of #5271).

[llvm-beanz]

So, I think this issue is more of an argument about why we shouldn’t enable assertions by default than an argument that we should.

The assert is revealing a bug, but it isn’t a bug that prevents correct code generation. With asserts disabled the shader compiles to correct output.

The problem with blanket enabling asserts in the DXC (or LLVM) codebase is that it enables a bunch of really expensive verification steps that are extremely useful for testing, but not for end users. My anecdotal test on DXC is that costs us about 15% on compile time.

[MarijnS95]

@llvm-beanz I've not validated that for larger HLSL source the output is indeed a valid shader. It is not my code but who knows if this is the source of vague/obscure bugs.

In my mind all bets are off when we ignore useful asserts, otherwise the assert shouldn't have been there.

[llvm-beanz]

@llvm-beanz I've not validated that for larger HLSL source the output is indeed a valid shader. It is not my code but who knows if this is the source of vague/obscure bugs.

The location of the assert you reported here is during diagnostic construction, and the code still does something correct even if the assert is false. The assert does signify a faulty assumption in the code, which I'll look into, but it does not result in any impact on AST formation or code generation.

In my mind all bets are off when we ignore useful asserts, otherwise the assert shouldn't have been there.

This is okay if that's how you use asserts in your codebase. That is not how asserts are used in LLVM, and applying that assumption to LLVM is not necessarily helpful.

LLVM generally uses asserts to verify assumptions but still expects the code to behave correctly if the assert is removed. Cases that are irrecoverable use report_fatal_error or exit in LLVM to end execution (note: in DXC exit calls have been replaced with exception throws).

[MarijnS95]

That constraint seems to have been loosened as LLVM transformed into DXC then :)

(Not necessarily speaking about this specific assert)

[llvm-beanz]

DXC's use of asserts is inconsistent at best, but since most of DXC's code is inherited from LLVM & Clang, I don't think you can safely dismiss how that code was written.

[MarijnS95]

Sure, let's settle that discussion there then, and I'll just have to be mindful of our rather uncommon use of many threads poking dxcompiler.dll at the same time (as threading bugs that end up being caught by asserts seem to pop up every once in a while).

Let's instead leave this issue to clear out the diagnostic assert, that'd unblock me from at least being able to run an assertion-enabled release build in case something else goes wrong :)

[simontaylor81]

We (Frostbite) just hit this issue as well. I wanted to clear a few things up since I think from reading the above there's a bit of a misunderstanding about the nature of this issue.

This is the code that is asserting:

ValueVector::reference CFGBlockValues::operator[](const VarDecl *vd) {
  const Optional<unsigned> &idx = declToIndex.getValueIndex(vd);
  assert(idx.hasValue());
  return scratch[idx.getValue()];
}

So, if the assert fires, then idx is an Optional with no value. If the assert is "ignored" (i.e. compiled out in Release) then the value of the Optional will be fetched anyway. This will just returned uninitialized memory. This is then used to index into a vector, which unless you are very lucky will be an out-of-bounds access.

Now, it appears that in very simple situations you "get away with it", but this is not true in general -- in our case it just crashes 🙂. We are compiling multiple shaders in parallel, and it sounds like this is what @MarijnS95 is doing too, so it sounds like that's one way to be unlucky 🙂.

So, based on this:

LLVM generally uses asserts to verify assumptions but still expects the code to behave correctly if the assert is removed.

This is definitely not true in this particular instance -- accessing unitialised data is not behaving correctly 🙂.

With asserts disabled the shader compiles to correct output.

IMO this is also not true in this instance -- at best the diagnostic analysis is wrong, attributing the assignment to the wrong variable. At worst it crashes, as we have seen 🙂.

Anyway, that's a very long winded way to say that this is a pretty bad bug, and prevents our use of out params in template functions 😄.

[MarijnS95]

I wanted to clear a few things up since I think from reading the above there's a bit of a misunderstanding about the nature of this issue.

After trying very hard to parse your reply, I couldn't find anything that is different in your report/reply/findings about the nature of this issue (maybe out the way it was written off as a diagnostics bug that still produces "correct" code?). Can you elaborate what this misunderstanding is?

I think we both found out doesn't work in templates, and that we just got lucky that our shader produces a working / "correct" output?

---

We are compiling multiple shaders in parallel, and it sounds like this is what @MarijnS95 is doing too, so it sounds like that's one way to be unlucky 🙂.

Yes, that's exactly what we're doing. On the same library and I think even compiler COM instance.

[simontaylor81]

I think we both found out doesn't work in templates, and that we just got lucky that our shader produces a working / "correct" output?

Yes, exactly, I believe we both hit the exact same issue. We weren't "lucky" though -- the compiler crashes for us 🙂.

My reference to a misunderstanding is the implication that this is an unimportant issue, because it "works" in Release. For us it very much does not, and is unlikely for most people (given that the code will index into an array with uninitialised memory, some that is very likely to cause a crash).