Address binskim flag for compiler in driver build #99

ShaojieZhuIntel · 2025-02-11T10:38:33Z

Summary

Since the compiler in the driver build cannot pass the Binskim scan and shows [Explicitly disabled warnings: 4146;4244;4267], it is necessary to remove the suppression of these three warnings. LLVM code like:

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

Line 692 in dfee500

-wd4146 # Suppress 'unary minus operator applied to unsigned type, result still unsigned'
     -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'
-wd4146 means disable the /w4146 warning
-wd4244 means disable the /w4244 warning
-wd4267 means disable the /w4267 warning

So we need enable these three warnings to pass the Binskim

Since it is not possible to directly enable specific warnings for the LLVM repo within the compiler repo, we use an option here to control whether the specified warnings are suppressed.

JIRA ticket

EISW-130780

Related PR in NPU Compiler and/or OpenVINO repository with sub-module update

PR-205

Other related tickets

List tickets for additional work, eg, something was found during review but you agreed to address it in another Jira

E-xxxxx

ShaojieZhuIntel · 2025-02-13T05:25:53Z

Apply the current changes from PR99 in PR15919, and perform compiler in driver build with PR205. The build was successful and passed the binskim scan.
The CiD build job is recorded in PR15919.

https://github.com/intel-innersource/applications.ai.vpu-accelerators.vpux-plugin/pull/15919 PR-checker passed

AlexandruLorinti · 2025-02-14T11:19:21Z

I confirm there are no errors regarding suppressed warnings in BinSkim scan

ggladilo · 2025-02-18T10:11:38Z

llvm/cmake/modules/HandleLLVMOptions.cmake

@@ -767,6 +767,15 @@ if (MSVC)
  foreach(flag ${msvc_warning_flags})
    append("${flag}" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
  endforeach(flag)
+
+  if (BUILD_COMPILER_FOR_DRIVER)
+    string(REPLACE "-wd4146" "" CMAKE_C_FLAGS "${CMAKE_C_FLAGS}")


nitpick: I'm not quite familiar with LLVM CMake code involved here, but I'd consider updating global CMAKE variables, especially CMAKE_C/CXX_* not the best practice, reason is there's a chance it'd affect other targets you didn't intended to change
I saw that case in our project already, when OpenVINO did this and then propagated not always desired compilation options to our targets

If you stick with modifying these variables for some reason, could you please clarify why and confirm no unintended target is changed (presumably nothing outside of llvm-project folder)?

Alternative would be just working with target properties, you can specify compilation options per target such it's not intrusive

Try to modify and test based on your suggestion.

ggladilo · 2025-02-20T10:34:38Z

llvm/lib/Support/CMakeLists.txt

+if(MSVC)
+  if(BUILD_COMPILER_FOR_DRIVER)


nitpick: if (MSVC AND BUILD_COMPILER_FOR_DRIVER)
Also, since we have to modified LLVM's codebase, I suppose we should have some kind tracker of all such changes

@nikita-kud do we have anything like this already?

Fixed, thank you!

@nikita-kud do we have anything like this already?

Yes, we have: E-101222
But I think it can't be upstreamed. This change looks sad. Why we can't suppress these warnings scanner?

@nikita-kud do we have anything like this already?

Yes, we have: E-101222 But I think it can't be upstreamed. This change looks sad. Why we can't suppress these warnings scanner?

Because we need to pass binskim. @AlexandruLorinti We are currently fix the binskim scan failures by applying a patch during the CiD build. Since it cannot be upstreamed, can we continue using this method?

@nikita-kud do we have anything like this already?

Yes, we have: E-101222 But I think it can't be upstreamed. This change looks sad. Why we can't suppress these warnings scanner?

Because we need to pass binskim. @AlexandruLorinti We are currently fix the binskim scan failures by applying a patch during the CiD build. Since it cannot be upstreamed, can we continue using this method?

any method would be ok, you can keep the patch in CiD build

ggladilo

Consider adding a comment to each modification of compile options
Maybe makes sense to create a CMake function that accepts target and silences all needed warnings, then you could add explanation to just function implementation
In this case all the logic for making binskim happy is reused

lmielick · 2025-02-28T15:25:44Z

llvm/lib/Analysis/CMakeLists.txt

+
+if(MSVC AND BUILD_COMPILER_FOR_DRIVER)
+  target_compile_options(LLVMAnalysis PRIVATE
+    /w34146 /w34244 /w34267


We should be able to do the same in NPU Cmake which includes LLVM, avoiding a patch.

Yea, I guess it's true, we could write CMake code/function in our project that processes pre-defined set of targets (from LLVM repo that we know are problematic to one our jobs/processes) and for each given target modifies a property of it that corresponds to compile options used to build the target accordingly
This way we get clean separation - this warnings disablement is not a problem of LLVM (since they can build their targets they probably just do not provide support for these-warnings-clean-build), but our problem since we seem to place more requirements on LLVM codebase than its authors/maintainers

ggladilo · 2025-03-04T11:53:31Z

llvm/lib/Analysis/CMakeLists.txt

+# Due to this target cannot pass the binskim scan after the compiler in the driver build,
+# it is necessary to remove the suppression of these three warnings(/wd4146 /wd4244 /wd4267).
+enable_msvc_warnings_on_compiler_for_driver(LLVMAnalysis /w3 4146 4244 4267)


Move comments to the function implementation, no need to repeat every time you call the function
Function name is confusing, do you disable or enable warnings here? From the values looks like you disable them
Also, I'd keep function specific to your use case, instead of accepting warnings as arguments, just "hardcode" them internally and name the function something along the lines as fix_binksim or something (choose better name than mine!)
Also consider @lmielick's comment to implement solution in compiler repo

Ok, thank you for your suggestions! Here we are enabling warnings, and I will modify the function according to your advice. Additionally, I am trying to modify only the compiler repo to make it pass binskim.

Signed-off-by: Zhu, Shaojie <[email protected]>

ShaojieZhuIntel · 2025-03-27T03:56:12Z

Firstly, can not directly enable warnings for LLVM within the compiler repo. Secondly, I tried creating a function in the compiler repo and calling this function for each target in LLVM that fails the binskim check. This approach reduces the binskim errors, but it involves modifying many targets. Additionally, if the same errors appear in other targets in the future, further modifications to the LLVM code will be necessary. Therefore, I believe it is more appropriate to add an option in LLVM that allows controlling the enabling of relevant warnings from the compiler repo.

ShaojieZhuIntel · 2025-03-27T08:59:00Z

Test job: https://jenkins-npu-compiler.ir.intel.com/job/pub/job/IE-Packages/job/Release/job/CiD-Windows10/15966/ passed
binskim passed
Hi @ggladilo @lmielick @nikita-kud @AlexandruLorinti Could you please help to review this change?

andrey-golubev · 2025-03-27T09:19:38Z

Why do we need it in LLVM? Can we instead do it at the compiler's side around add_subdirectory(LLVM)?
Then the change is local to our stack and doesn't have to be forever cherry-picked between LLVM releases.

Also, removing warnings doesn't look good to me. Are there issues in LLVM otherwise? Can we fix them instead and push that to upstream?

lmielick · 2025-03-27T10:33:56Z

This approach reduces the binskim errors, but it involves modifying many targets.

@ShaojieZhuIntel did you try using directory level CMAKE variables? I would presume we can append these flags into CFLAGS on directory level. Even if this is applied eagerly for all LLVM targets it should be fine:
a) there is no incentive to fix warnings on LLVM side
b) if it's contingent on BINSKIM flag we can still keep the LLVM warnings on in developer build or so

ShaojieZhuIntel · 2025-03-27T11:32:01Z

Why do we need it in LLVM? Can we instead do it at the compiler's side around add_subdirectory(LLVM)? Then the change is local to our stack and doesn't have to be forever cherry-picked between LLVM releases.

Also, removing warnings doesn't look good to me. Are there issues in LLVM otherwise? Can we fix them instead and push that to upstream?

We are not removing warnings; we are enabling warnings because some warnings are suppressed in LLVM, which is not allowed by binskim. Like:

  AliasAnalysis.cpp.obj (LLVMAnalysis.lib) [Explicitly disabled warnings: 4146;4244;4267]
  AliasSetTracker.cpp.obj (LLVMAnalysis.lib) [Explicitly disabled warnings: 4146;4244;4267]
  AssumeBundleQueries.cpp.obj (LLVMAnalysis.lib) [Explicitly disabled warnings: 4146;4244;4267]
  AssumptionCache.cpp.obj (LLVMAnalysis.lib) [Explicitly disabled warnings: 4146;4244;4267]
  BasicAliasAnalysis.cpp.obj (LLVMAnalysis.lib) [Explicitly disabled warnings: 4146;4244;4267]
  BlockFrequencyInfo.cpp.obj (LLVMAnalysis.lib) [Explicitly disabled warnings: 4146;4244;4267]
  BlockFrequencyInfoImpl.cpp.obj (LLVMAnalysis.lib) [Explicitly disabled warnings: 4146;4244;4267]
  BranchProbabilityInfo.cpp.obj (LLVMAnalysis.lib) [Explicitly disabled warnings: 4146;4244;4267]

Even if we handle this around add_subdirectory(LLVM), it will be overridden by the actions in LLVM that disable warnings.

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

Line 692 in dfee500

    
           -wd4146 # Suppress 'unary minus operator applied to unsigned type, result still unsigned'

      -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'

ShaojieZhuIntel · 2025-03-27T11:37:21Z

This approach reduces the binskim errors, but it involves modifying many targets.

@ShaojieZhuIntel did you try using directory level CMAKE variables? I would presume we can append these flags into CFLAGS on directory level. Even if this is applied eagerly for all LLVM targets it should be fine: a) there is no incentive to fix warnings on LLVM side b) if it's contingent on BINSKIM flag we can still keep the LLVM warnings on in developer build or so

Yes, I have tried enabling warnings around add_subdirectory(LLVM), but LLVM suppresses some warnings again, causing the enabling of warnings to be ineffective. Like:

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

Line 692 in dfee500

    
           -wd4146 # Suppress 'unary minus operator applied to unsigned type, result still unsigned'

      -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'

XinWangIntel · 2025-04-01T02:43:56Z

@ggladilo @lmielick @andrey-golubev @AlexandruLorinti , could you help to review this again? The PR is to open suppressed warnings by remove some compiler flags (which make we can not see warnings in log). And we tried on vpux-plugin repo side, append cmake flag on that level can not work since llvm append flag after that again and invalid our new added flags.

ShaojieZhuIntel force-pushed the address-cid-binskim-patch branch from 0dbede6 to c5b379a Compare February 13, 2025 02:10

ShaojieZhuIntel marked this pull request as ready for review February 13, 2025 05:23

ShaojieZhuIntel requested a review from a team as a code owner February 13, 2025 05:23

hrotuna approved these changes Feb 13, 2025

View reviewed changes

hrotuna requested review from andrey-golubev and nikita-kud February 13, 2025 11:40

ggladilo reviewed Feb 18, 2025

View reviewed changes

ggladilo reviewed Feb 20, 2025

View reviewed changes

ShaojieZhuIntel force-pushed the address-cid-binskim-patch branch from 85abebf to cb1282d Compare February 20, 2025 11:57

ggladilo reviewed Feb 21, 2025

View reviewed changes

lmielick reviewed Feb 28, 2025

View reviewed changes

ggladilo reviewed Mar 4, 2025

View reviewed changes

enable some warnings to pass binskim

5ce36b2

Signed-off-by: Zhu, Shaojie <[email protected]>

ShaojieZhuIntel force-pushed the address-cid-binskim-patch branch from 26236fc to 5ce36b2 Compare March 27, 2025 03:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address binskim flag for compiler in driver build #99

Address binskim flag for compiler in driver build #99

ShaojieZhuIntel commented Feb 11, 2025 •

edited

Loading

ShaojieZhuIntel commented Feb 13, 2025 •

edited

Loading

AlexandruLorinti commented Feb 14, 2025

ggladilo Feb 18, 2025

ShaojieZhuIntel Feb 19, 2025

ggladilo Feb 20, 2025

ShaojieZhuIntel Feb 20, 2025

nikita-kud Feb 20, 2025 •

edited

Loading

ShaojieZhuIntel Feb 21, 2025

AlexandruLorinti Feb 21, 2025

ggladilo left a comment

lmielick Feb 28, 2025

ggladilo Feb 28, 2025

ggladilo Mar 4, 2025

ShaojieZhuIntel Mar 4, 2025

ShaojieZhuIntel commented Mar 27, 2025

ShaojieZhuIntel commented Mar 27, 2025

andrey-golubev commented Mar 27, 2025 •

edited

Loading

lmielick commented Mar 27, 2025 •

edited

Loading

ShaojieZhuIntel commented Mar 27, 2025

ShaojieZhuIntel commented Mar 27, 2025

XinWangIntel commented Apr 1, 2025

		if(MSVC)
		if(BUILD_COMPILER_FOR_DRIVER)

Address binskim flag for compiler in driver build #99

Are you sure you want to change the base?

Address binskim flag for compiler in driver build #99

Conversation

ShaojieZhuIntel commented Feb 11, 2025 • edited Loading

Summary

JIRA ticket

Related PR in NPU Compiler and/or OpenVINO repository with sub-module update

Other related tickets

ShaojieZhuIntel commented Feb 13, 2025 • edited Loading

AlexandruLorinti commented Feb 14, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikita-kud Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggladilo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShaojieZhuIntel commented Mar 27, 2025

ShaojieZhuIntel commented Mar 27, 2025

andrey-golubev commented Mar 27, 2025 • edited Loading

lmielick commented Mar 27, 2025 • edited Loading

ShaojieZhuIntel commented Mar 27, 2025

ShaojieZhuIntel commented Mar 27, 2025

XinWangIntel commented Apr 1, 2025

ShaojieZhuIntel commented Feb 11, 2025 •

edited

Loading

ShaojieZhuIntel commented Feb 13, 2025 •

edited

Loading

nikita-kud Feb 20, 2025 •

edited

Loading

andrey-golubev commented Mar 27, 2025 •

edited

Loading

lmielick commented Mar 27, 2025 •

edited

Loading