Bug 195794 – [WHLSL] Enforce variable lifetimes

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGPU

[Myles C. Maxfield]

Variables need to behave as-if they live forever.

[Radar WebKit Bug Importer]

<rdar://problem/50746293>

[Saam Barati]

Gonna start working on this.

[Saam Barati]

Thoughts:
- Let's do this before metal codegen. So let's have it be a modifying pass over AST. 
- Make all entry points declare a struct that has fields for each variable.
- Conservatively, each variable/argument would work.
- Conservatively, but more optimized, every variable that has its address taken should also work.
- Make each non-entrypoint function take a "struct*" as its first parameter
- Make the first statements of each entry point to declare and then take the address of this struct.
- Make the first, or in the case of an entry point, after struct declaration, statement of any function where the parameter ends up in the struct store into the struct as the first thing it does. Alternatively, each callsite could have a unique entry in the struct to represent such an argument. Calls would then store into the struct prior to call, and functions would remove said arguments from their argument list, and call sites would be modified to not pass them in. The latter sounds harder than the former.

[Saam Barati]

Created attachment 370539 [details]
WIP

it begins

[Saam Barati]

Created attachment 370620 [details]
WIP

It's starting to transform stuff in very reasonable ways. Not 100% complete yet though.

[Saam Barati]

Created attachment 370786 [details]
WIP

getting closer...

[Saam Barati]

Created attachment 370795 [details]
WIP

AST rewriting might be done

[Saam Barati]

Created attachment 370802 [details]
WIP

I think it's done. Need to rebase and make sure I have nothing else to do.

[Saam Barati]

Created attachment 370821 [details]
patch

If anyone is interested in looking, this should be just about done. Just need to make assigning through pointers work so I can test this.

[EWS Watchlist]

Attachment 370821 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/Modules/webgpu/WHLSL/Metal/WHLSLFunctionWriter.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 370821 [details]
patch

Attachment 370821 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12314321

New failing tests:
webgpu/whlsl-arbitrary-vertex-attribute-locations.html
webgpu/whlsl-dot-expressions.html
webgpu/whlsl-dont-crash-parsing-enum.html
webgpu/whlsl-nested-dot-expression-rvalue.html
webgpu/whlsl.html

[EWS Watchlist]

Created attachment 370827 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Saam Barati]

Created attachment 370915 [details]
patch

[Saam Barati]

Created attachment 370916 [details]
patch

[Myles C. Maxfield]

Comment on attachment 370916 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370916&action=review

> Source/WTF/wtf/PrintStream.h:135
> -        Name(const Type& value)                  \
> +        Name(Type value)                         \

Why?

> Source/WebCore/ChangeLog:19
> +        as ptrPtr(). So, the following would print "42":

This isn't right. All variables behave as if they had an initializer; either they have an explicit one, or an implicit one which behaves as a zero-fill.

Consider:
thread int* foo() {
    int x = 42;
    /* do something with x */;
    return &x;
}

and

thread int* bar = foo();
*bar = 100;
foo();

The code that is /* do something with x */ shouldn’t have to expect that x can be either 42 or 100. The code that is /* do something with x */ can assume that x is 42, because it is declared to be 42 directly above it. Therefore, initializers are run every time a variable declaration is encountered. So, this example should print 0.

[Saam Barati]

Comment on attachment 370916 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370916&action=review

>> Source/WTF/wtf/PrintStream.h:135
>> +        Name(Type value)                         \
> 
> Why?

So we can make Type a reference

>> Source/WebCore/ChangeLog:19
>> +        as ptrPtr(). So, the following would print "42":
> 
> This isn't right. All variables behave as if they had an initializer; either they have an explicit one, or an implicit one which behaves as a zero-fill.
> 
> Consider:
> thread int* foo() {
>     int x = 42;
>     /* do something with x */;
>     return &x;
> }
> 
> and
> 
> thread int* bar = foo();
> *bar = 100;
> foo();
> 
> The code that is /* do something with x */ shouldn’t have to expect that x can be either 42 or 100. The code that is /* do something with x */ can assume that x is 42, because it is declared to be 42 directly above it. Therefore, initializers are run every time a variable declaration is encountered. So, this example should print 0.

Right. Example should be:
p = ptrPtr()
*ptr() = 42
print *p

[Saam Barati]

Comment on attachment 370916 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370916&action=review

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:258
> +        expressions.append(WTFMove(dummyResult));

This is kind of unfortunate, but it was a straight forward way to appease the type checker and maintain the soundness of the IR. Anyone have any alternate suggestions or is this reasonable enough?

[Myles C. Maxfield]

Comment on attachment 370916 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370916&action=review

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:41
> +// legitimate and well specified thing to do.

Nit: well-specified

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:48
> +// - Each call is rewritten to pass a pointer to the struct as the last call argument.

Hopefully not Native function calls. Those which are implemented by the compiler itself shouldn't have to bother with this stuff.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:59
> +        if (!is<AST::VariableReference>(makePointerExpression.leftValue())) {

What about &(foo[3])?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:67
> +        auto* variable = variableReference.variable();

Can we ASSERT(variable) to get a crash in debug builds?

Or we could move the ASSERT() inside the body of .variable() if we audit all the call sites.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:69
> +        // If variable is unnamed, it means it's internal. We don't allow internal variables to escape.
> +        if (!variable->name().isEmpty())

I don't think this is intentional. Internal variables probably can be allowed to have names. Can we use another signal here instead?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:82
> +    return Lexer::Token { { }, 0, type };

Can we use some existing nearby token instead? It's not great but it's better than something explicitly meaningless.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:295
> +    String structName = "__WrapperStruct__"_s;

Nit: Why the underbars?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.h:36
> +void preserveVariableLifetimes(Program&);

👍

> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLExpression.h:44
> +    using Base = Value;

Is this really more clear than it was before?

[Saam Barati]

Comment on attachment 370916 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370916&action=review

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:59
>> +        if (!is<AST::VariableReference>(makePointerExpression.leftValue())) {
> 
> What about &(foo[3])?

Won't this be transformed into something like:
operator&[](&foo, 3)

[Saam Barati]

Comment on attachment 370916 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370916&action=review

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPrepare.cpp:60
> +static constexpr bool dumpASTAtEnd = true;

will revert to false

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:41
>> +// legitimate and well specified thing to do.
> 
> Nit: well-specified

will fix.

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:48
>> +// - Each call is rewritten to pass a pointer to the struct as the last call argument.
> 
> Hopefully not Native function calls. Those which are implemented by the compiler itself shouldn't have to bother with this stuff.

Yeah, we don't do this when calling native. I will clarify.

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:67
>> +        auto* variable = variableReference.variable();
> 
> Can we ASSERT(variable) to get a crash in debug builds?
> 
> Or we could move the ASSERT() inside the body of .variable() if we audit all the call sites.

I can assert.

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:69
>> +        if (!variable->name().isEmpty())
> 
> I don't think this is intentional. Internal variables probably can be allowed to have names. Can we use another signal here instead?

Right. This sounds like a good follow-up. Will remove this for now and we can do it in a follow up if we think it's necessary. FWIW, I think LLVM should reason all of this away. The main benefit is probably shrinking size of the struct.

https://bugs.webkit.org/show_bug.cgi?id=198383

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:82
>> +    return Lexer::Token { { }, 0, type };
> 
> Can we use some existing nearby token instead? It's not great but it's better than something explicitly meaningless.

I only use this when we synthesize types. I don't know what "nearby" means in such a scenario. Alternatively, we could just make the concept of an empty token. Maybe that's nicer.

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:295
>> +    String structName = "__WrapperStruct__"_s;
> 
> Nit: Why the underbars?

Just a quick way to visually denote it's an internal construct. But it's not needed. I just kinda like it.

>> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLExpression.h:44
>> +    using Base = Value;
> 
> Is this really more clear than it was before?

Not really in this patch. In general, I like using "Base" instead of the name of the parent class just to make re-parenting a class in the future require changing fewer lines of code.

[Saam Barati]

Created attachment 370959 [details]
patch

[Myles C. Maxfield]

(In reply to Saam Barati from comment #20)
> Comment on attachment 370916 [details]
> >> Source/WebCore/Modules/webgpu/WHLSL/AST/WHLSLExpression.h:44
> >> +    using Base = Value;
> > 
> > Is this really more clear than it was before?
> 
> Not really in this patch. In general, I like using "Base" instead of the
> name of the parent class just to make re-parenting a class in the future
> require changing fewer lines of code.

But we're writing these lines of code now anway, regardless of whether or not we end up actually reparenting...

[Myles C. Maxfield]

Comment on attachment 370916 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370916&action=review

>>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:59
>>> +        if (!is<AST::VariableReference>(makePointerExpression.leftValue())) {
>> 
>> What about &(foo[3])?
> 
> Won't this be transformed into something like:
> operator&[](&foo, 3)

Cool.

[Myles C. Maxfield]

Comment on attachment 370916 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370916&action=review

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:104
> +        bool isEntryPoint = !!functionDefinition.entryPointType();

Yuck, I hate "!!". I know that's the WebKit style, but I much prefer static_cast<bool>. Do you even need the !! if you're assigning to a bool?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:115
> +            AST::VariableDeclarations structVariableDeclarations;
> +            structVariableDeclarations.append(WTFMove(structVariableDeclaration));
> +            auto structDeclarationStatement = makeUniqueRef<AST::VariableDeclarationsStatement>(functionDefinition.origin(), WTFMove(structVariableDeclarations));

Can we put this in a helper?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:159
> +            auto structVariableReference = makeStructVariableReference();
> +            auto lhs = makeUniqueRef<AST::GlobalVariableReference>(parameter->origin(), WTFMove(structVariableReference), iter->value);
> +            lhs->setType(parameter->type()->clone());
> +            lhs->setTypeAnnotation(AST::LeftValue { AST::AddressSpace::Thread });
> +
> +            auto rhs = makeUniqueRef<AST::VariableReference>(AST::VariableReference::wrap(parameter.get()));
> +            rhs->setType(parameter->type()->clone());
> +            rhs->setTypeAnnotation(AST::RightValue());
> +
> +            auto assignment = makeUniqueRef<AST::AssignmentExpression>(parameter->origin(), WTFMove(lhs), WTFMove(rhs));
> +            assignment->setType(parameter->type()->clone());
> +            assignment->setTypeAnnotation(AST::RightValue());

Similar to below. It seems wasteful to have arguments that never get referenced. It's probably better to actually remove the parameters from the function entirely, and rewrite call expressions to do assignments into the global struct. This is a fairly substantial project though, and we might want to just file a bug about it and do it later. However, doing this same type of mutation for VariableDeclarationsStatement should be easier to implement so we should consider doing that in this patch.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:176
> +            functionDefinition.block().statements().insert(0,
> +                makeUniqueRef<AST::VariableDeclarationsStatement>(WTFMove(origin), WTFMove(declarations)));

Is there a reason not to join all these together into a single VariableDeclarationsStatement? Fewer mallocs()...

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:184
> +        RELEASE_ASSERT(m_structVariable);

I don't quite understand the philosophical difference between ASSERT() and RELEASE_ASSERT(). (I know the behavior difference, but not the logic the WebKit team uses when picking which one to use.) Why release here?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:200
> +        if (iter == m_variableMapping.end())
> +            return;

So this is if the EscapedVariableCollector proved that the variable doesn't need to lie within the global struct? I think a comment here would be helpful.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:204
> +        auto* internalField = AST::replaceWith<AST::GlobalVariableReference>(variableReference, variableReference.origin(), makeStructVariableReference(), iter->value);

origin() returns a reference, which I think will be UAF inside the body of replaceWith() because replaceWith()'s first argument is also a reference

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:215
> +        Vector<std::pair<AST::VariableDeclaration*, Optional<UniqueRef<AST::Expression>>>> variables;

I'd prefer replacing the pair with a struct for readability.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:225
> +            if (pair.second) {

Shouldn't we emit an assignment even if there is no initializer (for zero-filling)? If so, can we at least link to the bugzilla in a comment saying FIXME?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:228
> +                lhs->setTypeAnnotation(AST::LeftValue { AST::AddressSpace::Thread });

This is cool.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:252
> +            auto iter = m_variableMapping.find(pair.first);
> +            if (iter != m_variableMapping.end()) {
> +                auto lhs = makeUniqueRef<AST::GlobalVariableReference>(Lexer::Token(origin), makeStructVariableReference(), iter->value);
> +                lhs->setType(pair.first->type()->clone());
> +                lhs->setTypeAnnotation(AST::LeftValue { AST::AddressSpace::Thread });
> +
> +                auto rhs = makeUniqueRef<AST::VariableReference>(AST::VariableReference::wrap(*pair.first));
> +                rhs->setType(pair.first->type()->clone());
> +                rhs->setTypeAnnotation(AST::RightValue());
> +
> +                auto assignment = makeUniqueRef<AST::AssignmentExpression>(Lexer::Token(origin), WTFMove(lhs), WTFMove(rhs));
> +                assignment->setType(pair.first->type()->clone());
> +                assignment->setTypeAnnotation(AST::RightValue());
> +
> +                expressions.append(WTFMove(assignment));

I see what's happening. All variables get hoisted, regardless of whether or not they are eventually going to end up in the global struct. If they do end up in the global struct, you just copy from the variable into the global struct at initialization time.

Isn't that wasteful? Why not only hoist the variable declarations that don't end up in the global struct, and have all other variable declarations get destroyed? I believe the only inbound raw pointers to a variable declaration can be from a variable reference, which will get fixed up in this pass (though this claim should be verified). Doing this would allow you to unify this "if" statement and the one directly above it; the rhs is just the initializer, and the lhs is either the variable reference or the GlobalVariableReference, depending on whether the variable ended up living in the global struct.

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:258
>> +        expressions.append(WTFMove(dummyResult));
> 
> This is kind of unfortunate, but it was a straight forward way to appease the type checker and maintain the soundness of the IR. Anyone have any alternate suggestions or is this reasonable enough?

This pass runs after the type checker, so I don't quite understand this comment.

What happens if you delete these three lines?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:297
> +    auto wrapperStruct = makeUniqueRef<AST::StructureDefinition>(anonymousToken(Lexer::Token::Type::Struct), String(structName), WTFMove(elements));

Colloquially we often refer to the instance as "the struct". I'd recommend using more precise variable names like "wrapperStructDefinition"

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:304
> +    auto structType = makeUniqueRef<AST::TypeReference>(anonymousToken(Lexer::Token::Type::Identifier), String(structName), AST::TypeArguments());

structTypeReference

I think there's a TypeReference::wrap() convenience function.

> LayoutTests/webgpu/whlsl-ensure-proper-variable-lifetime.html:47
> +    thread float* ptrToShade = firstPtr(shade);
> +    thread float* ptrToShade2 = secondPtr(1.0);
> +
> +    float dummy;
> +    thread float* dummyPtr = &dummy;
> +
> +    dummy = assignToPtr(ptrToShade2, 0.0);
> +    dummy = assignToPtr(ptrToShade, shade);
> +
> +    result.position = position;
> +
> +    result.shade = *ptrToShade2;
> +
> +    return result;

Can we also add a test that makes sure that just regular local variables w/o pointers work? like int foo() { int x = 42; return x; } Or, if it's covered by existing tests, noting that in the ChangeLog would be good.

[Saam Barati]

Comment on attachment 370916 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370916&action=review

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:104
>> +        bool isEntryPoint = !!functionDefinition.entryPointType();
> 
> Yuck, I hate "!!". I know that's the WebKit style, but I much prefer static_cast<bool>. Do you even need the !! if you're assigning to a bool?

I like "!!". But to answer your question, we don't need it. However, I think !! makes the code clearer. So does static_cast<bool>, but that's a lot more characters, and not any clearer.

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:115
>> +            auto structDeclarationStatement = makeUniqueRef<AST::VariableDeclarationsStatement>(functionDefinition.origin(), WTFMove(structVariableDeclarations));
> 
> Can we put this in a helper?

Why?

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:159
>> +            assignment->setTypeAnnotation(AST::RightValue());
> 
> Similar to below. It seems wasteful to have arguments that never get referenced. It's probably better to actually remove the parameters from the function entirely, and rewrite call expressions to do assignments into the global struct. This is a fairly substantial project though, and we might want to just file a bug about it and do it later. However, doing this same type of mutation for VariableDeclarationsStatement should be easier to implement so we should consider doing that in this patch.

This isn't really hard to do, but it's slightly annoying to do, since it requires traversing the call graph. I think the analysis is, "function f take this parameter if it has any variables that end up in the struct or if it calls a function that has any variables that end up in the struct". I can file a follow up.

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:176
>> +                makeUniqueRef<AST::VariableDeclarationsStatement>(WTFMove(origin), WTFMove(declarations)));
> 
> Is there a reason not to join all these together into a single VariableDeclarationsStatement? Fewer mallocs()...

Nope. I just didn't think of that. Will do.

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:184
>> +        RELEASE_ASSERT(m_structVariable);
> 
> I don't quite understand the philosophical difference between ASSERT() and RELEASE_ASSERT(). (I know the behavior difference, but not the logic the WebKit team uses when picking which one to use.) Why release here?

I like RELEASE_ASSERT when the assert itself  isn't expensive.

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:204
>> +        auto* internalField = AST::replaceWith<AST::GlobalVariableReference>(variableReference, variableReference.origin(), makeStructVariableReference(), iter->value);
> 
> origin() returns a reference, which I think will be UAF inside the body of replaceWith() because replaceWith()'s first argument is also a reference

This patch changes this to:
Lexer::Token origin() const { return m_origin; }

on Value

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:225
>> +            if (pair.second) {
> 
> Shouldn't we emit an assignment even if there is no initializer (for zero-filling)? If so, can we at least link to the bugzilla in a comment saying FIXME?

Nah. That pass should be able to handle that.

>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:252
>> +                expressions.append(WTFMove(assignment));
> 
> I see what's happening. All variables get hoisted, regardless of whether or not they are eventually going to end up in the global struct. If they do end up in the global struct, you just copy from the variable into the global struct at initialization time.
> 
> Isn't that wasteful? Why not only hoist the variable declarations that don't end up in the global struct, and have all other variable declarations get destroyed? I believe the only inbound raw pointers to a variable declaration can be from a variable reference, which will get fixed up in this pass (though this claim should be verified). Doing this would allow you to unify this "if" statement and the one directly above it; the rhs is just the initializer, and the lhs is either the variable reference or the GlobalVariableReference, depending on whether the variable ended up living in the global struct.

I'm just going to introduce that new AST node we talked about. We're just dancing around it at this point. It'll be a list of statements (like a block), but won't be a scope (unlike a block).

>>> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:258
>>> +        expressions.append(WTFMove(dummyResult));
>> 
>> This is kind of unfortunate, but it was a straight forward way to appease the type checker and maintain the soundness of the IR. Anyone have any alternate suggestions or is this reasonable enough?
> 
> This pass runs after the type checker, so I don't quite understand this comment.
> 
> What happens if you delete these three lines?

You crash somewhere in metal code generation IIRC

>> LayoutTests/webgpu/whlsl-ensure-proper-variable-lifetime.html:47
>> +    return result;
> 
> Can we also add a test that makes sure that just regular local variables w/o pointers work? like int foo() { int x = 42; return x; } Or, if it's covered by existing tests, noting that in the ChangeLog would be good.

I can add a test

[Saam Barati]

Created attachment 370999 [details]
patch

[Saam Barati]

Comment on attachment 370999 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370999&action=review

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLPreserveVariableLifetimes.cpp:224
> +                auto lhs = makeUniqueRef<AST::GlobalVariableReference>(variable.origin(), makeStructVariableReference(), iter->value);
> +                lhs->setType(variable.type()->clone());
> +                lhs->setTypeAnnotation(AST::LeftValue { AST::AddressSpace::Thread });
> +
> +                auto rhs = makeUniqueRef<AST::VariableReference>(AST::VariableReference::wrap(variable));
> +                rhs->setType(variable.type()->clone());
> +                rhs->setTypeAnnotation(AST::RightValue());
> +
> +                auto assignment = makeUniqueRef<AST::AssignmentExpression>(variable.origin(), WTFMove(lhs), WTFMove(rhs));
> +                assignment->setType(variable.type()->clone());
> +                assignment->setTypeAnnotation(AST::RightValue());

I'm going to make a helper for this, since we also emit this code for the prologue of function definitions.

[Saam Barati]

Created attachment 371004 [details]
patch

[EWS Watchlist]

Comment on attachment 371004 [details]
patch

Attachment 371004 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/12334885

New failing tests:
http/wpt/service-workers/service-worker-networkprocess-crash.html

[EWS Watchlist]

Created attachment 371027 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.14.5

[WebKit Commit Bot]

Comment on attachment 371004 [details]
patch

Clearing flags on attachment: 371004

Committed r245945: <https://trac.webkit.org/changeset/245945>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.