[webkit-reviews] review granted: [Bug 195794] [WHLSL] Enforce variable lifetimes : [Attachment 370916] patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu May 30 14:28:26 PDT 2019


Myles C. Maxfield <mmaxfield at apple.com> has granted  review:
Bug 195794: [WHLSL] Enforce variable lifetimes
https://bugs.webkit.org/show_bug.cgi?id=195794

Attachment 370916: patch

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




--- Comment #24 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 370916
  --> https://bugs.webkit.org/attachment.cgi?id=370916
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.ge
t()));
> +	       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::Stru
ct), 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.


More information about the webkit-reviews mailing list