Skip to content

[SYCL][NFC] Fix static code analysis concerns #2531

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Sep 25, 2020
Merged

[SYCL][NFC] Fix static code analysis concerns #2531

merged 2 commits into from
Sep 25, 2020

Conversation

smanna12
Copy link
Contributor

@smanna12 smanna12 commented Sep 24, 2020 •
edited
Loading

Found via a static-analysis tool:

1. Pointer 'NewDeclAttr' returned from call to function 'getAttr<clang::SYCLIntelMaxWorkGroupSizeAttr>'
   at line 3218 may be NULL and will be dereferenced at line 3220.

2. Pointer 'NewDeclAttr' returned from call to function 'getAttr<clang::ReqdWorkGroupSizeAttr>'
   at line 3218 may be NULL and will be dereferenced at line 3220.

3. Pointer 'OldDeclAttr' returned from call to function 'getAttr<clang::ReqdWorkGroupSizeAttr>'
   at line 3219 may be NULL and will be dereferenced at line 3220.

4. Pointer 'OldDeclAttr' returned from call to function 'getAttr<clang::SYCLIntelMaxWorkGroupSizeAttr>'
   at line 3219 may be NULL and will be dereferenced at line 3220.

This patch fixes null pointer dereference issues in SemaDecl.cpp file
by adding early return if (!NewDeclAttr || !OldDeclAttr)

Signed-off-by: Soumi Manna soumi.manna@intel.com

@smanna12
[SYCL][NFC] Fix static code analysis concerns
00693a6 
Found via a static-analysis tool:

    1. Pointer 'NewDeclAttr' returned from call to function 'getAttr<clang::SYCLIntelMaxWorkGroupSizeAttr>'
       at line 3218 may be NULL and will be dereferenced at line 3220.

    2. Pointer 'NewDeclAttr' returned from call to function 'getAttr<clang::ReqdWorkGroupSizeAttr>'
       at line 3218 may be NULL and will be dereferenced at line 3220.

    3. Pointer 'OldDeclAttr' returned from call to function 'getAttr<clang::ReqdWorkGroupSizeAttr>'
       at line 3219 may be NULL and will be dereferenced at line 3220.

    4. Pointer 'OldDeclAttr' returned from call to function 'getAttr<clang::SYCLIntelMaxWorkGroupSizeAttr>'
       at line 3219 may be NULL and will be dereferenced at line 3220.

This patch fixes null pointer dereference issues in SemaDecl.cpp file
by changing return type of pointers from AttributeType to const auto.

Signed-off-by: Soumi Manna <soumi.manna@intel.com>
@smanna12 smanna12 marked this pull request as ready for review September 24, 2020 10:21
@smanna12 smanna12 requested a review from erichkeane September 24, 2020 10:21
@erichkeane
Copy link
Contributor

This won't change the return type other than adding 'const', it won't change the fact that getAttr can return a nullptr.

This doesn't solve hte problem you're trying to solve.

elizabethandrews
elizabethandrews reviewed Sep 24, 2020
View reviewed changes
@smanna12
Update patch based on review comments
46ab806 
Signed-off-by: Soumi Manna <soumi.manna@intel.com>
erichkeane
erichkeane approved these changes Sep 24, 2020
View reviewed changes
Copy link
Contributor

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrm.... bonus points would be given for making this a variadic template, so that this would work:

checkDimensionsAndSetDiagnostics<ReqdWorkGroupSizeAttr, SYCLIntelMaxWorkGroupSizeAttr>(*this, New, Old);

That said, its more code at the moment to do so over just leaving 2 otherwise identical calls, so next time someone adds to this list perhaps we'll make them do it then :)

premanandrao
premanandrao reviewed Sep 24, 2020
View reviewed changes
clang/lib/Sema/SemaDecl.cpp
@@ -3302,12 +3306,8 @@ bool Sema::MergeFunctionDecl(FunctionDecl *New, NamedDecl *&OldD,
}
}

if (New->hasAttr<ReqdWorkGroupSizeAttr>() &&
Old->hasAttr<ReqdWorkGroupSizeAttr>())
checkDimensionsAndSetDiagnostics<ReqdWorkGroupSizeAttr>(*this, New, Old);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand this is what @erichkeane suggested earlier, but I am not sure I like this. Would we do this for every attribute we want to merge? Seems like a lot of wasted calls.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it ends up being basically the same runtime-cost here after optimization. It seems like an easy function to inline, and hasAttr and getAttr have identical runtime cost.

My fear here is that this ends up being REALLY error-prone as time goes on, we're going to be adding these for a handful of attributes and it becomes easy to copy/paste error the THREE uses of the attribute type.

@premanandrao
Copy link
Contributor

Hrm.... bonus points would be given for making this a variadic template, so that this would work:

checkDimensionsAndSetDiagnostics<ReqdWorkGroupSizeAttr, SYCLIntelMaxWorkGroupSizeAttr>(*this, New, Old);

That said, its more code at the moment to do so over just leaving 2 otherwise identical calls, so next time someone adds to this list perhaps we'll make them do it then :)

Okay, I can live with this. :-)

elizabethandrews
elizabethandrews reviewed Sep 24, 2020
View reviewed changes
@smanna12
Copy link
Contributor Author

Thank you everyone for the reviews.

@bader bader merged commit df45f00 into intel:sycl Sep 25, 2020
alexbatashev pushed a commit to alexbatashev/llvm that referenced this pull request Sep 29, 2020
Merge remote-tracking branch 'upstream/sycl' into global_constructors…
026ea4e 
…_wrapper

* upstream/sycl: (1533 commits)
  [SYCL] XFAIL sub_group shuffle tests on GPU
  [SYCL] Add support for L0 loader validation layer (intel#2520)
  [NFC][LIT] Temporary disable function pointers as they hang on L0 (intel#2544)
  [SYCL] use release version of OpenCL ICD loader
  [SYCL] Improve testing of host-task (intel#2540)
  Revert 1291215
  [SYCL] Fix warning caused by [[nodiscard]] attribute (intel#2545)
  [SYCL] Workaround windows build failure
  [SYCL] Remove kernel_signature_start from int header (intel#2537)
  [SYCL] Fix ABI tests in post-commit (intel#2539)
  [SYCL][DOC] Update C-CXX-StandardLibrary doc to align with latest status (intel#2529)
  [SYCL][NFC] Fix static code analysis concerns (intel#2531)
  [SYCL][NFC] Improve testing for accessor_property_list (intel#2532)
  [SYCL] Avoid overuse of CPU on wait read-write lock loop (intel#2525)
  [SYCL] Implement no-decomposition for kernel types that don't need it. (intel#2477)
  [SYCL] Add group algorithm constraints (intel#2462)
  [BuildBot] Uplift Windows GPU RT from 8673 to 8778 (intel#2533)
  [SYCL][LIT][NFC] Extend ABI test suite (intel#2522)
  [SYCL][DebugInfo] Reinstate source locations for some kernel instructions (intel#2527)
  [SYCL][NFC] Replace the deprecated VectorType::getNumElements() (intel#2524)
  ...
kbenzie pushed a commit to kbenzie/intel-llvm that referenced this pull request Feb 17, 2025
@pbalcer
Merge pull request intel#2531 from Xewar313/fix-ze-calls
cb74dc9 
Fix calls to ze destructors in level-zero adapter v2
Chenyang-L pushed a commit that referenced this pull request Feb 18, 2025
@pbalcer
Merge pull request #2531 from Xewar313/fix-ze-calls
052f18f 
Fix calls to ze destructors in level-zero adapter v2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erichkeane erichkeane erichkeane approved these changes

@premanandrao premanandrao premanandrao approved these changes

@elizabethandrews elizabethandrews elizabethandrews approved these changes

@Fznamznon Fznamznon Awaiting requested review from Fznamznon

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@smanna12 @erichkeane @premanandrao @elizabethandrews @bader