Skip to content

Address binskim flag for compiler in driver build #99

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

Open
wants to merge 1 commit into
base: npu/release/19.x
Choose a base branch
from
Open

Address binskim flag for compiler in driver build #99

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions llvm/cmake/modules/HandleLLVMOptions.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -765,6 +765,16 @@ if (MSVC)

# Enable warnings
if (LLVM_ENABLE_WARNINGS)
# Remove all -wd flag to enable warnings
if (NOT CLANG_CL)
set(msvc_warning_flags
# Promoted warnings.
-w14062 # Promote 'enumerator in switch of enum is not handled' to level 1 warning.

# Promoted warnings to errors.
-we4238 # Promote 'nonstandard extension used : class rvalue used as lvalue' to error.
)
endif(NOT CLANG_CL)
Copy link
Contributor

@andrey-golubev andrey-golubev Apr 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

can we add these in NPU CMake to CMAKE_CXX_FLAGS instead?
something tells me it should be possible to do. and we won't need the patch here.

Copy link
Contributor

Choose a reason for hiding this comment

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

(now that we know that ENABLE_WARNINGS works, perhaps we could move it outside?)

Copy link
Author

Choose a reason for hiding this comment

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

-w14062 and -we4238 are not the key to solving this issue. The key is to clear the -wdxxxx flags in ${msvc_warning_flags}.
Only enabling LLVM_ENABLE_WARNINGS does not work. We must reset ${msvc_warning_flags} in this code because

npu-plugin-llvm/llvm/cmake/modules/HandleLLVMOptions.cmake

Lines 777 to 780 in 13000db

foreach(flag ${msvc_warning_flags})
append("${flag}" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
endforeach(flag)
endif (MSVC)
writes ${msvc_warning_flags} into CMAKE_CXX_FLAGS.
Our goal is to remove all -wdxxxx flags set in

npu-plugin-llvm/llvm/cmake/modules/HandleLLVMOptions.cmake

Lines 689 to 746 in 13000db

set(msvc_warning_flags
# Disabled warnings.
-wd4141 # Suppress ''modifier' : used more than once' (because of __forceinline combined with inline)
-wd4146 # Suppress 'unary minus operator applied to unsigned type, result still unsigned'
-wd4244 # Suppress ''argument' : conversion from 'type1' to 'type2', possible loss of data'
-wd4267 # Suppress ''var' : conversion from 'size_t' to 'type', possible loss of data'
-wd4291 # Suppress ''declaration' : no matching operator delete found; memory will not be freed if initialization throws an exception'
-wd4351 # Suppress 'new behavior: elements of array 'array' will be default initialized'
-wd4456 # Suppress 'declaration of 'var' hides local variable'
-wd4457 # Suppress 'declaration of 'var' hides function parameter'
-wd4458 # Suppress 'declaration of 'var' hides class member'
-wd4459 # Suppress 'declaration of 'var' hides global declaration'
-wd4503 # Suppress ''identifier' : decorated name length exceeded, name was truncated'
-wd4624 # Suppress ''derived class' : destructor could not be generated because a base class destructor is inaccessible'
-wd4722 # Suppress 'function' : destructor never returns, potential memory leak
-wd4100 # Suppress 'unreferenced formal parameter'
-wd4127 # Suppress 'conditional expression is constant'
-wd4512 # Suppress 'assignment operator could not be generated'
-wd4505 # Suppress 'unreferenced local function has been removed'
-wd4610 # Suppress '<class> can never be instantiated'
-wd4510 # Suppress 'default constructor could not be generated'
-wd4702 # Suppress 'unreachable code'
-wd4245 # Suppress ''conversion' : conversion from 'type1' to 'type2', signed/unsigned mismatch'
-wd4706 # Suppress 'assignment within conditional expression'
-wd4310 # Suppress 'cast truncates constant value'
-wd4701 # Suppress 'potentially uninitialized local variable'
-wd4703 # Suppress 'potentially uninitialized local pointer variable'
-wd4389 # Suppress 'signed/unsigned mismatch'
-wd4611 # Suppress 'interaction between '_setjmp' and C++ object destruction is non-portable'
-wd4805 # Suppress 'unsafe mix of type <type> and type <type> in operation'
-wd4204 # Suppress 'nonstandard extension used : non-constant aggregate initializer'
-wd4577 # Suppress 'noexcept used with no exception handling mode specified; termination on exception is not guaranteed'
-wd4091 # Suppress 'typedef: ignored on left of '' when no variable is declared'
# C4592 is disabled because of false positives in Visual Studio 2015
# Update 1. Re-evaluate the usefulness of this diagnostic with Update 2.
-wd4592 # Suppress ''var': symbol will be dynamically initialized (implementation limitation)
-wd4319 # Suppress ''operator' : zero extending 'type' to 'type' of greater size'
# C4709 is disabled because of a bug with Visual Studio 2017 as of
# v15.8.8. Re-evaluate the usefulness of this diagnostic when the bug
# is fixed.
-wd4709 # Suppress comma operator within array index expression
# We'd like this warning to be enabled, but it triggers from code in
# WinBase.h that we don't have control over.
-wd5105 # Suppress macro expansion producing 'defined' has undefined behavior
# Ideally, we'd like this warning to be enabled, but even MSVC 2019 doesn't
# support the 'aligned' attribute in the way that clang sources requires (for
# any code that uses the LLVM_ALIGNAS macro), so this is must be disabled to
# avoid unwanted alignment warnings.
-wd4324 # Suppress 'structure was padded due to __declspec(align())'
# Promoted warnings.
-w14062 # Promote 'enumerator in switch of enum is not handled' to level 1 warning.
# Promoted warnings to errors.
-we4238 # Promote 'nonstandard extension used : class rvalue used as lvalue' to error.
)
, such as -wd4141, -wd4146, and -wd4244. These flags disable some warnings. By removing them, we can enable the corresponding warnings. Removing these -wdxxxx flags while enabling LLVM_ENABLE_WARNINGS aligns with the purpose of this option.

Copy link

@ggladilo ggladilo May 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Changing CMAKE_CXX_FLAGS is not a good idea since it's global property and will impact all targets built, which is not what we want probably

Copy link
Author

Choose a reason for hiding this comment

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

Changing CMAKE_CXX_FLAGS is not a idea since it's global property and will impact all targets built, which is not what we want probably

In fact, I have already tried enabling the specified warnings for each target that reported an error, but this approach has two drawbacks. First, there are too many targets that need modification. Second, if there are other BinSkim errors in the future, further modifications will be required. Additionally, I have tried enabling warnings around add_subdirectory(LLVM), but LLVM suppresses some warnings again, causing the enabling of warnings to be ineffective.
Changing CMAKE_CXX_FLAGS is only to re-enable the previously suppressed warnings, which aligns with the purpose of the LLVM_ENABLE_WARNINGS option. This option is off by default, and we only turn it on during CiD build, so other builds won't be affected.

Copy link

Choose a reason for hiding this comment

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

To clarify, my original concern was that with this change we disable some warnings for potentially a lot of targets, but now I see we actually enable warnings, so I'm fine with the change

# Put /W4 in front of all the -we flags. cl.exe doesn't care, but for
# clang-cl having /W4 after the -we flags will re-enable the warnings
# disabled by -we.
Expand Down