[SYCL] Null VarDecl dereference when a binding declaration is captured #6924
Conversation
After the support for capturing structured bindings in lambdas, variables captured in BuildCaptureField need not be VarDecls. A previous patch added needs to now account for a possible null pointer before dereferencing the pointer to get its name string.
|
|
||
| // CHECK: expected-no-diagnostics | ||
|
|
||
| template <typename T> |
There was a problem hiding this comment.
Can the test be simpler? Something like the following should be enough to test this right?
int main() {
int a[2] = {1, 2};
auto [x, y] = a;
auto L = [=]() {x = 10;};
L();
return 0;
}
There was a problem hiding this comment.
Also can these kind of captures happen inside SYCL kernel and if so should we add a test to see what metadata is generated?
There was a problem hiding this comment.
Can the test be simpler? Something like the following should be enough to test this right?
int main() { int a[2] = {1, 2}; auto [x, y] = a; auto L = [=]() {x = 10;}; L(); return 0; }
Done.
There was a problem hiding this comment.
Also can these kind of captures happen inside SYCL kernel and if so should we add a test to see what metadata is generated?
I have added a CodeGen test.
clang/lib/Sema/SemaLambda.cpp
Outdated
| @@ -1723,7 +1723,7 @@ FieldDecl *Sema::BuildCaptureField(RecordDecl *RD, | |||
| // For SYCL compilations, save user specified names for | |||
| // lambda capture. | |||
| if (getLangOpts().SYCLIsDevice || getLangOpts().SYCLIsHost) { | |||
| StringRef CaptureName = Var->getName(); | |||
| StringRef CaptureName = Var ? Var->getName() : ""; | |||
There was a problem hiding this comment.
So binding declarations are just null VarDecls?
There was a problem hiding this comment.
So binding declarations are just null VarDecls?
Yeah, that feels weird to me too.
Also, they seem to have a name.
There was a problem hiding this comment.
So binding declarations are just null VarDecls?
No. Originally this routine had a VarDecl return type. This was changed to return a ValueDecl as part of the support for lambdas capturing BindingDecls. So now, in this routine, when we dynamic cast to VarDecl, there is a possibility that Var could be NULL. That is the bad null-dereference that this PR is fixing.
There was a problem hiding this comment.
A ValueDecl is a NamedDecl. Is there a reason why we can't retain the name for this as well? The reason we retained names for captures is so that the metadata generated for openCL kernel arguments retained use specified names - which we are now not doing for this case.
There was a problem hiding this comment.
I have added the name of the BindingDecl.
|
The failure of lsc/lsc_surf_load_u32.cpp is unrelated to this change. |
After the support for capturing structured bindings in lambdas, variables captured in BuildCaptureField
need not be VarDecls. A previous patch needs to now account for a possible null pointer before
dereferencing the pointer to get its name string.