[SYCL] Add a regression test for the crash during spec constants processing

[AlexeySotkin]

In case a SYCL kernel captures specialization constants and more than
one accessor it crashes. I think the crash happens due to an extra
increment of KernelFuncParam, in CreateOpenCLKernelBody() method.

Signed-off-by: Alexey Sotkin alexey.sotkin@intel.com

[erichkeane]

Please add a clang lit test for this that validates what the IR should look like in this case.

This ends up likely being solved in the SemaSYCL rewrite but I'd like to accept some sort of this fix with some good validation tests.

[erichkeane]

Rewrite here: #1517

[bader]

Please add a clang lit test for this that validates what the IR should look like in this case.

I would suggest "rewriting" the test. I.e. removing the existing test from the runtime library project.

[AlexeySotkin]

Please add a clang lit test for this that validates what the IR should look like in this case.

This ends up likely being solved in the SemaSYCL rewrite but I'd like to accept some sort of this fix with some good validation tests.

Do I really need to validate the IR if purpose of this patch is to fix a crash?

[AlexeySotkin]

Please add a clang lit test for this that validates what the IR should look like in this case.

I would suggest "rewriting" the test. I.e. removing the existing test from the runtime library project.

Where exactly do you suggest to move the test?

[bader]

Do I really need to validate the IR if purpose of this patch is to fix a crash?

No.

[bader]

Please add a clang lit test for this that validates what the IR should look like in this case.

I would suggest "rewriting" the test. I.e. removing the existing test from the runtime library project.

Where exactly do you suggest to move the test?

You seems to fix Sema component, so it should be in clang/test/Sema*

[erichkeane]

Please add a clang lit test for this that validates what the IR should look like in this case.
This ends up likely being solved in the SemaSYCL rewrite but I'd like to accept some sort of this fix with some good validation tests.

Do I really need to validate the IR if purpose of this patch is to fix a crash?

Yes, if the problem is that the parameters aren't being passed correctly, which causes the crash. We want our tests fine grained so that they can catch issues like this in the future while still being easy to reproduce.

[AlexeySotkin]

Please add a clang lit test for this that validates what the IR should look like in this case.
This ends up likely being solved in the SemaSYCL rewrite but I'd like to accept some sort of this fix with some good validation tests.

Do I really need to validate the IR if purpose of this patch is to fix a crash?

Yes, if the problem is that the parameters aren't being passed correctly, which causes the crash. We want our tests fine grained so that they can catch issues like this in the future while still being easy to reproduce.

I think that problem is not that the parameters are being passed incorrectly. This PR fixes the crash in the Sema itself (in the same method). Without the fix I've got an out of boundary access at line 703 here

llvm/clang/lib/Sema/SemaSYCL.cpp

Lines 701 to 707 in 6c88351

	auto KFP = KernelFuncParam;
	for (size_t I = 0; I < NumParams; ++KFP, ++I) {
	QualType ParamType = (*KFP)->getOriginalType();
	ParamDREs[I] = DeclRefExpr::Create(
	S.Context, NestedNameSpecifierLoc(), SourceLocation(), *KFP,
	false, DeclarationNameInfo(), ParamType, VK_LValue);
	}

[AlexeySotkin]

Thank you all for your review. I think I have resolved all your comments with my latest commit.

[bader]

Thank you all for your review. I think I have resolved all your comments with my latest commit.

Please, mark resolved comments using "Resolve conversation" button.

[bader]

@AlexeySotkin, I just merged PR conflicting with these changes. Could you resolve the merge conflicts, please?

[erichkeane]

@AlexeySotkin, I just merged PR conflicting with these changes. Could you resolve the merge conflicts, please?

Hopefully the solution to it is simply to 'delete' the changes in this patch in SemaSYCL and make sure that the tests still pass. If not, @Fznamznon and I can help.

[AlexeySotkin]

@AlexeySotkin, I just merged PR conflicting with these changes. Could you resolve the merge conflicts, please?

Hopefully the solution to it is simply to 'delete' the changes in this patch in SemaSYCL and make sure that the tests still pass. If not, @Fznamznon and I can help.

I have rebased my patch on top of the sycl branch. And now it seems that the test pass without my changes to the Clang.

Honestly, I don't see much sense in this PR anymore

[Fznamznon]

@AlexeySotkin, I just merged PR conflicting with these changes. Could you resolve the merge conflicts, please?

Hopefully the solution to it is simply to 'delete' the changes in this patch in SemaSYCL and make sure that the tests still pass. If not, @Fznamznon and I can help.

I have rebased my patch on top of the sycl branch. And now it seems that the test pass without my changes to the Clang.

Honestly, I don't see much sense in this PR anymore

We still need the test.

[erichkeane]

I would still like to have the test, if we broke it once, I can see us breaking it again. As I mentioned in my first response, I anticipated that this would be the case.