Page MenuHomePhabricator

[OpenCL] Warn about side effects for unevaluated vec_step arg
ClosedPublic

Authored by svenvh on Nov 12 2020, 6:30 AM.

Details

Summary

The argument to the vec_step builtin is not evaluated. Hoist the
diagnostic for this in Sema::CheckUnaryExprOrTypeTraitOperand such
that it comes before Sema::CheckVecStepTraitOperandType.

A minor side-effect of this change is that it also produces the
warning for co_await and co_yield now.

Associated spec clarification: https://github.com/KhronosGroup/OpenCL-Docs/pull/481

Diff Detail

Unit TestsFailed

TimeTest
400 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp

Event Timeline

svenvh created this revision.Nov 12 2020, 6:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 12 2020, 6:30 AM
Herald added a subscriber: yaxunl. · View Herald Transcript
svenvh requested review of this revision.Nov 12 2020, 6:30 AM
svenvh updated this revision to Diff 305058.Nov 13 2020, 1:49 AM

Add test case for OpenCL.

Anastasia accepted this revision.Dec 31 2020, 3:48 AM

LGTM, the change seems reasonable. Thanks!

A minor side-effect of this change is that it also produces the
warning for co_await and co_yield now.

This warning is produced for sizeof and it seems like a duplication of another diagnostic that occurs for this case. But this happens elsewhere and it is not harmful so it seems acceptable. Alternatively, we could try to move code that contains CheckVecStepTraitOperandType some line below instead of moving the warning to the top. But I don't find it necessary.

Btw just a suggestion - it is better to upload a full diff because the regular diff is not reliable in the Phabricator as line numbers change quickly in the repository and there is no information on the parent commit for the patch.

clang/lib/Sema/SemaExpr.cpp
4038

The comment could be updated to add vec_step. Although I think the comment has already changed in the repo so a rebase would be necessary.

This revision is now accepted and ready to land.Dec 31 2020, 3:48 AM

LGTM, the change seems reasonable. Thanks!

A minor side-effect of this change is that it also produces the
warning for co_await and co_yield now.

This warning is produced for sizeof and it seems like a duplication of another diagnostic that occurs for this case.

It aligns the note diagnostics for sizeof and vec_step for this case indeed.

Btw just a suggestion - it is better to upload a full diff because the regular diff is not reliable in the Phabricator as line numbers change quickly in the repository and there is no information on the parent commit for the patch.

Apologies, I normally do so but forgot to provide the full context this time.

This revision was landed with ongoing or failed builds.Jan 5 2021, 3:52 AM
This revision was automatically updated to reflect the committed changes.