Skip to content
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

Introduce new SDL compile flags on Windows and fix warnings. #1035

Merged
merged 13 commits into from Mar 30, 2023
Merged

Introduce new SDL compile flags on Windows and fix warnings. #1035

merged 13 commits into from Mar 30, 2023

Conversation

sarathnandu
Copy link
Contributor

@sarathnandu sarathnandu commented Feb 28, 2023 •
edited

Description

Add SDL related compiler flags for MSVC and fix warnings in Windows build.

Fixes # - issue number(s) if exists

  • - git commit message contains an appropriate signed-off-by string (see CONTRIBUTING.md for details)

Type of change

Choose one or multiple, leave empty if none of the other choices apply

Add a respective label(s) to PR if you have permissions

  • bug fix - change that fixes an issue
  • new feature - change that adds functionality
  • tests - change in tests
  • infrastructure - change in infrastructure and CI
  • documentation - documentation update

Tests

  • added - required for new features and some bug fixes
  • not needed

Documentation

  • updated in # - add PR number
  • needs to be updated
  • not needed

Breaks backward compatibility

  • Yes
  • No
  • Unknown

Notify the following users

List users with @ to send notifications

Other information

@sarathnandu
Add Windows compiler flags and fix TBB Malloc Warnings.
06f603b 
Signed-off-by: sarathnandu <sarath.nandu.ramachandran.nair@intel.com>
@sarathnandu
Update Indentation on frontend.cpp
eb8da39 
Signed-off-by: sarathnandu <sarath.nandu.ramachandran.nair@intel.com>
@sarathnandu
Update Indentation on frontend.cpp
6edcb23 
Signed-off-by: sarathnandu <sarath.nandu.ramachandran.nair@intel.com>
@sarathnandu
Merge branch 'dev/sarathna/win_flags' of https://github.com/oneapi-sr…
67d2741 
…c/oneTBB into dev/sarathna/win_flags

Signed-off-by: sarathnandu <sarath.nandu.ramachandran.nair@intel.com>
@sarathnandu
Remove INTEGRITYCHECK flag from MSVC.cmake
676cc23 
Signed-off-by: sarathnandu <sarath.nandu.ramachandran.nair@intel.com>
dnmokhov
dnmokhov reviewed Mar 4, 2023
View reviewed changes
cmake/compilers/Intel.cmake Outdated
Comment on lines 18 to 19
set(TBB_OPENMP_FLAG /Qopenmp)
set(TBB_LIB_LINK_FLAGS ${TBB_LIB_LINK_FLAGS} /Qcf-protection:full)
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not too familiar with the purpose of each of these flags variables, but isn't /Qcf-protection a code generation instead of a linker option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/Qcf-protection as a code-generation option has performance impact. And I think it does not have any impact as linker option. I'll remove this flag for now.

cmake/compilers/MSVC.cmake Outdated
@@ -35,6 +35,14 @@ endif()
set(TBB_LIB_COMPILE_FLAGS -D_CRT_SECURE_NO_WARNINGS /GS)
set(TBB_COMMON_COMPILE_FLAGS /volatile:iso /FS /EHsc)

set(TBB_COMMON_COMPILE_FLAGS ${TBB_COMMON_COMPILE_FLAGS} /GS /W4)
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't these already set on lines 22 and 35 above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe in line 22 :
set(TBB_WARNING_LEVEL $&lt;$&lt;NOT:$<CXX_COMPILER_ID:Intel>>:/W4> $<$BOOL:${TBB_STRICT}:/WX>), the TBB_WARNING_LEVEL is set if /W4 is set for compilers other than Intel. Is my assumption right ?
But /GS seems to be set in line 35 for TBB_LIB_COMPILE_FLAGS and maybe that's good enough - we dont need for common compile flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The warnings in TBB-malloc starts showing for MSVC build only when /W4 is added.

Copy link
Contributor

Choose a reason for hiding this comment

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

There appear to be some TODOs in src/tbbmalloc/CMakeLists.txt with regards to that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion!. Removed w4 warning level from common compile flags and added to TBB_WARNING_LEVEL in tbbmalloc. Verified all the warnings are fixed.

@sarathnandu
Address review comments.
479565a 
Signed-off-by: sarathnandu <sarath.nandu.ramachandran.nair@intel.com>
@sarathnandu sarathnandu marked this pull request as draft March 15, 2023 18:30
@sarathnandu
Fix compiler warnings for gcc.
73d13ed 
Signed-off-by: sarathnandu <sarath.nandu.ramachandran.nair@intel.com>
@sarathnandu
Fix icx compiler warning on tbbmalloc
da482da 
Signed-off-by: sarathnandu <sarath.nandu.ramachandran.nair@intel.com>
@sarathnandu
Update Copyright for tbbmalloc src
916c756 
Signed-off-by: sarathnandu <sarath.nandu.ramachandran.nair@intel.com>
dnmokhov
dnmokhov reviewed Mar 21, 2023
View reviewed changes
src/tbbmalloc/large_objects.cpp Outdated Show resolved Hide resolved
src/tbbmalloc/CMakeLists.txt Outdated Show resolved Hide resolved
cmake/compilers/MSVC.cmake Outdated Show resolved Hide resolved
@sarathnandu
Fix more compiler warnings for tbbmalloc.
9e72d25
@sarathnandu
Suppress /wd4267 related warnings
da40304 
Signed-off-by: sarathnandu <sarath.nandu.ramachandran.nair@intel.com>
@sarathnandu sarathnandu marked this pull request as ready for review March 27, 2023 22:17
@sarathnandu
Fix the pointer cast.
daf267e 
Signed-off-by: sarathnandu <sarath.nandu.ramachandran.nair@intel.com>
dnmokhov
dnmokhov previously approved these changes Mar 28, 2023
View reviewed changes
Copy link
Contributor

@dnmokhov dnmokhov left a comment

Choose a reason for hiding this comment

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

LGTM

isaevil
isaevil reviewed Mar 28, 2023
View reviewed changes
cmake/compilers/MSVC.cmake Outdated Show resolved Hide resolved
pavelkumbrasev
pavelkumbrasev reviewed Mar 28, 2023
View reviewed changes
cmake/compilers/MSVC.cmake Outdated Show resolved Hide resolved
src/tbbmalloc/Customize.h Outdated Show resolved Hide resolved
src/tbbmalloc/backref.cpp Outdated Show resolved Hide resolved
src/tbbmalloc/backref.cpp Outdated Show resolved Hide resolved
src/tbbmalloc/backref.cpp Outdated Show resolved Hide resolved
src/tbbmalloc/frontend.cpp Outdated Show resolved Hide resolved
src/tbbmalloc/frontend.cpp Outdated Show resolved Hide resolved
src/tbbmalloc/large_objects.h Outdated Show resolved Hide resolved
@sarathnandu sarathnandu changed the title Dev/sarathna/win flags Introduce new SDL compile flags on Windows and fix warnings. Mar 28, 2023
@sarathnandu @pavelkumbrasev
Apply suggestions from code review
f6aee25 
Fix review comments.

Co-authored-by: Pavel Kumbrasev <pavel.kumbrasev@intel.com>
@sarathnandu sarathnandu merged commit 2110128 into master Mar 30, 2023
18 checks passed
@sarathnandu sarathnandu deleted the dev/sarathna/win_flags branch March 30, 2023 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@isaevil isaevil isaevil left review comments

@pavelkumbrasev pavelkumbrasev pavelkumbrasev approved these changes

@dnmokhov dnmokhov dnmokhov approved these changes

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@sarathnandu @pavelkumbrasev @dnmokhov @isaevil