[SYCL][NFC] Avoid nullptr dereferencing of YDimExpr and ZDimExpr #4367

schittir · 2021-08-18T23:20:31Z

Add assert that arguments of work_group_size attributes cannot be nullptr

Klocwork exposed a (false positive) bug that nullptr derferencing of
YDimExpr and ZDimExpr is possible for non sycl:: usage.
This is not practically possible because such (OpenCL and cl::) spellings
of the attribute require three arguments.

This patch

Removes the ability of SetDefaultValue lambda to return nullptr
Asserts in SetDefaultValue that it is not possible for sycl:: case
to have NULL arguments
Moves error checking for three arguments for OpenCL and cl:: cases
ahead of the assert
Removes check for intel::reqd_work_group_size spelling

Fix the Klocworks exposed bug for non sycl:: or intel:: usage by refactoring and adding checks to avoid nullptr dereferencing

clang/lib/Sema/SemaDeclAttr.cpp

Fix whitespace

clang/lib/Sema/SemaDeclAttr.cpp

premanandrao

LGTM

schittir · 2021-08-24T18:28:01Z

Fixing commit message now.

AaronBallman

LGTM! Can you also add "NFC" to the review title as this has no functional change?

schittir · 2021-08-24T18:40:06Z

Thank you for the review, @erichkeane @AaronBallman - I updated the description and title.

Thanks @premanandrao

schittir · 2021-08-25T05:35:56Z

Working on reproducing the test failures locally.

clang/lib/Sema/SemaDeclAttr.cpp

schittir · 2021-08-25T18:03:18Z

Working on reproducing the test failures locally.

Still unable to reproduce the failure locally. Machine ran out of space meanwhile. Will try a diferent macine.

Do it after checking for 3 argument requirement for OpenCL and cl:: spellings

clang/lib/Sema/SemaDeclAttr.cpp

Assert in SetDefaultValue that it is not possible for sycl:: or intel:: cases to have NULL arguments Move error checking of three arguments for OpenCL and cl:: cases ahead of the assert in SetDefaultValue

clang/lib/Sema/SemaDeclAttr.cpp

…DimExpr_nullptr

AaronBallman

LGTM aside from a microscopic commenting nit, thanks!

clang/lib/Sema/SemaDeclAttr.cpp

schittir · 2021-09-08T16:22:16Z

Removed extra // and updated description. @AaronBallman Could you please take a look and approve again? Thank you.

smanna12

Thanks for the patch.

AaronBallman

LGTM!

schittir · 2021-09-08T16:47:20Z

LGTM!

Thank you for all the reviews!

dm-vodopyanov · 2021-09-08T19:46:12Z

@schittir, can you add a test for these changes?

schittir · 2021-09-08T20:16:39Z

@schittir, can you add a test for these changes?

I don't think there is a real case where the assert is false. This is a non-functional change made to appease Klocwork.

MrSidims

Post-factum LGTM

…el#4367) Add assert that arguments of work_group_size attributes cannot be nullptr Klocwork exposed a (false positive) bug that nullptr derferencing of YDimExpr and ZDimExpr is possible for non sycl:: usage. This is not practically possible because such (OpenCL and cl::) spellings of the attribute require three arguments. This patch 1. Removes the ability of SetDefaultValue lambda to return nullptr 2. Asserts in SetDefaultValue that it is not possible for sycl:: case to have NULL arguments 3. Moves error checking for three arguments for OpenCL and cl:: cases ahead of the assert 4. Removes check for intel::reqd_work_group_size spelling

[SYCL] Avoid nullptr dereferencing of YDimExpr and ZDimExpr

adf91fa

Fix the Klocworks exposed bug for non sycl:: or intel:: usage by refactoring and adding checks to avoid nullptr dereferencing

schittir requested review from AaronBallman, elizabethandrews and premanandrao as code owners August 18, 2021 23:20

schittir requested a review from erichkeane August 18, 2021 23:27

AaronBallman reviewed Aug 19, 2021

View reviewed changes

clang/lib/Sema/SemaDeclAttr.cpp Show resolved Hide resolved

clang/lib/Sema/SemaDeclAttr.cpp Outdated Show resolved Hide resolved

clang/lib/Sema/SemaDeclAttr.cpp Outdated Show resolved Hide resolved

Assert that YDimExpr and ZDimExpr can't practically be nullptr

e9f8624

erichkeane previously approved these changes Aug 24, 2021

View reviewed changes

AaronBallman reviewed Aug 24, 2021

View reviewed changes

clang/lib/Sema/SemaDeclAttr.cpp Outdated Show resolved Hide resolved

clang/lib/Sema/SemaDeclAttr.cpp Show resolved Hide resolved

Remove local variable E; return literal directly

5dbf9d5

Fix whitespace

schittir dismissed erichkeane’s stale review via 5dbf9d5 August 24, 2021 18:19

AaronBallman reviewed Aug 24, 2021

View reviewed changes

clang/lib/Sema/SemaDeclAttr.cpp Outdated Show resolved Hide resolved

Fix parenthesis

690d6ce

premanandrao previously approved these changes Aug 24, 2021

View reviewed changes

AaronBallman previously approved these changes Aug 24, 2021

View reviewed changes

schittir changed the title ~~[SYCL] Avoid nullptr dereferencing of YDimExpr and ZDimExpr~~ [SYCL][NFC] Avoid nullptr dereferencing of YDimExpr and ZDimExpr Aug 24, 2021

MrSidims reviewed Aug 25, 2021

View reviewed changes

clang/lib/Sema/SemaDeclAttr.cpp Outdated Show resolved Hide resolved

Assert that YDimExpr or ZDimExpr cannot be null at a different place

11f0a79

Do it after checking for 3 argument requirement for OpenCL and cl:: spellings

schittir dismissed stale reviews from AaronBallman and premanandrao via 11f0a79 September 2, 2021 22:14

Remove whitespace

3d4000a

AaronBallman reviewed Sep 3, 2021

View reviewed changes

clang/lib/Sema/SemaDeclAttr.cpp Outdated Show resolved Hide resolved

schittir requested a review from AaronBallman September 3, 2021 18:10

Remove the ability of SetDefaultValue lambda to return nullptr

7f49a73

Assert in SetDefaultValue that it is not possible for sycl:: or intel:: cases to have NULL arguments Move error checking of three arguments for OpenCL and cl:: cases ahead of the assert in SetDefaultValue

smanna12 reviewed Sep 7, 2021

View reviewed changes

clang/lib/Sema/SemaDeclAttr.cpp Outdated Show resolved Hide resolved

Merge remote-tracking branch 'intel_llvm_remote/sycl' into YDimExpr_Z…

4e4008a

…DimExpr_nullptr

Rebase and remove checks for intel::reqd_work_group_size spelling

5ae220f

AaronBallman previously approved these changes Sep 8, 2021

View reviewed changes

clang/lib/Sema/SemaDeclAttr.cpp Outdated Show resolved Hide resolved

Remove extra //

e114ef6

schittir dismissed AaronBallman’s stale review via e114ef6 September 8, 2021 16:20

smanna12 approved these changes Sep 8, 2021

View reviewed changes

bader requested review from AaronBallman, MrSidims and premanandrao September 8, 2021 16:31

AaronBallman approved these changes Sep 8, 2021

View reviewed changes

dm-vodopyanov merged commit 6e9fa89 into intel:sycl Sep 8, 2021

MrSidims reviewed Sep 8, 2021

View reviewed changes

[SYCL][NFC] Avoid nullptr dereferencing of YDimExpr and ZDimExpr #4367

[SYCL][NFC] Avoid nullptr dereferencing of YDimExpr and ZDimExpr #4367

Uh oh!

Conversation

schittir commented Aug 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

premanandrao left a comment

Choose a reason for hiding this comment

Uh oh!

schittir commented Aug 24, 2021

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

schittir commented Aug 24, 2021

Uh oh!

schittir commented Aug 25, 2021

Uh oh!

Uh oh!

schittir commented Aug 25, 2021

Uh oh!

Uh oh!

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

schittir commented Sep 8, 2021

Uh oh!

smanna12 left a comment

Choose a reason for hiding this comment

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

schittir commented Sep 8, 2021

Uh oh!

dm-vodopyanov commented Sep 8, 2021

Uh oh!

schittir commented Sep 8, 2021

Uh oh!

MrSidims left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

schittir commented Aug 18, 2021 •

edited

Loading