From 87e6a26c5d99db784b8df4d76a85fba6ac344eb4 Mon Sep 17 00:00:00 2001 From: Changming Sun Date: Wed, 23 Nov 2022 09:25:02 -0800 Subject: [PATCH] Enforce Prefast check in Windows CPU CI pipeline (#13735) Right now we fix the warnings in an ad-hoc way. We run static analysis in nightly builds, then create work items for the finding it found. Our CI build pipelines run the same scan but do not break the build. So, this PR will fix the remaining findings in the CPU EP(including the training part) and enforce the check. Later on we can continue to expand the scope. We still have some warnings left in the JNI part. I will try to address them later in the next month. --- .../onnxruntime/core/common/profiler_common.h | 2 +- onnxruntime/core/platform/windows/env.cc | 2 +- onnxruntime/core/session/onnxruntime_c_api.cc | 9 +++------ .../contrib_ops/remove_padding_op_test.cc | 6 +++--- .../contrib_ops/restore_padding_op_test.cc | 6 +++--- onnxruntime/test/framework/tunable_op_test.cc | 5 ++--- .../cpu/activation/activation_op_test.cc | 20 ++++++++++++++++--- .../templates/win-ci-vs-2019.yml | 4 ++-- .../azure-pipelines/win-ci-pipeline.yml | 6 +++--- 9 files changed, 35 insertions(+), 25 deletions(-) diff --git a/include/onnxruntime/core/common/profiler_common.h b/include/onnxruntime/core/common/profiler_common.h index 07d6cc101b..305e4385b4 100644 --- a/include/onnxruntime/core/common/profiler_common.h +++ b/include/onnxruntime/core/common/profiler_common.h @@ -59,7 +59,7 @@ struct EventRecord { args(event_args) {} EventRecord(const EventRecord& other) = default; - EventRecord(EventRecord&& other) = default; + EventRecord(EventRecord&& other) noexcept = default; EventRecord& operator=(const EventRecord& other) = default; EventRecord& operator=(EventRecord&& other) = default; diff --git a/onnxruntime/core/platform/windows/env.cc b/onnxruntime/core/platform/windows/env.cc index a96b1d1608..e0524e1734 100644 --- a/onnxruntime/core/platform/windows/env.cc +++ b/onnxruntime/core/platform/windows/env.cc @@ -508,7 +508,7 @@ class WindowsEnv : public Env { 0, static_cast(mapped_offset), mapped_length); - + GSL_SUPPRESS(r .11) mapped_memory = MappedMemoryPtr{reinterpret_cast(mapped_base) + offset_to_page, OrtCallbackInvoker{OrtCallback{UnmapFile, new UnmapFileParam{mapped_base, mapped_length}}}}; diff --git a/onnxruntime/core/session/onnxruntime_c_api.cc b/onnxruntime/core/session/onnxruntime_c_api.cc index c308a553cb..8781694518 100644 --- a/onnxruntime/core/session/onnxruntime_c_api.cc +++ b/onnxruntime/core/session/onnxruntime_c_api.cc @@ -1989,8 +1989,7 @@ ORT_API_STATUS_IMPL(OrtApis::CreateOpaqueValue, _In_z_ const char* domain_name, "Specified domain and type names combination does not refer to a registered opaque type"); const auto* non_tensor_base = ml_type->AsNonTensorType(); ORT_ENFORCE(non_tensor_base != nullptr, "Opaque type is not a non_tensor type!!!"); - GSL_SUPPRESS(r .11) - std::unique_ptr ort_val(new OrtValue); + std::unique_ptr ort_val = std::make_unique(); non_tensor_base->FromDataContainer(data_container, data_container_size, *ort_val); *out = ort_val.release(); API_IMPL_END @@ -2012,6 +2011,7 @@ ORT_API_STATUS_IMPL(OrtApis::GetOpaqueValue, _In_ const char* domain_name, _In_ return nullptr; } +GSL_SUPPRESS(r .11) ORT_API_STATUS_IMPL(OrtApis::GetAvailableProviders, _Outptr_ char*** out_ptr, _In_ int* providers_length) { API_IMPL_BEGIN @@ -2021,11 +2021,9 @@ ORT_API_STATUS_IMPL(OrtApis::GetAvailableProviders, _Outptr_ char*** out_ptr, constexpr size_t MAX_LEN = 30; const auto& available_providers = GetAvailableExecutionProviderNames(); const int available_count = narrow(available_providers.size()); - GSL_SUPPRESS(r .11) char** const out = new char*[available_count]; if (out) { for (int i = 0; i < available_count; i++) { - GSL_SUPPRESS(r .11) out[i] = new char[MAX_LEN + 1]; #ifdef _MSC_VER strncpy_s(out[i], MAX_LEN, available_providers[i].c_str(), MAX_LEN); @@ -2195,8 +2193,7 @@ ORT_API(void, OrtApis::ReleaseArenaCfg, _Frees_ptr_opt_ OrtArenaCfg* ptr) { ORT_API_STATUS_IMPL(OrtApis::CreatePrepackedWeightsContainer, _Outptr_ OrtPrepackedWeightsContainer** out) { API_IMPL_BEGIN - GSL_SUPPRESS(r .11) - std::unique_ptr container(new PrepackedWeightsContainer()); + std::unique_ptr container = std::make_unique(); *out = reinterpret_cast(container.release()); return nullptr; API_IMPL_END diff --git a/onnxruntime/test/contrib_ops/remove_padding_op_test.cc b/onnxruntime/test/contrib_ops/remove_padding_op_test.cc index 0ef004907f..d1a189de9a 100644 --- a/onnxruntime/test/contrib_ops/remove_padding_op_test.cc +++ b/onnxruntime/test/contrib_ops/remove_padding_op_test.cc @@ -94,9 +94,9 @@ static void RunRemovePaddingTests( int hidden_size, int total_tokens) { bool use_float16 = false; - const bool disable_cpu = true; - const bool disable_cuda = false; - const bool disable_rocm = true; + constexpr bool disable_cpu = true; + constexpr bool disable_cuda = false; + constexpr bool disable_rocm = true; RunRemovePadding(input_data, sequence_token_count_data, output_data, token_offset_data, cumulated_seq_len_data, max_token_count, batch_size, sequence_length, hidden_size, total_tokens, use_float16, disable_cpu, disable_cuda, disable_rocm); diff --git a/onnxruntime/test/contrib_ops/restore_padding_op_test.cc b/onnxruntime/test/contrib_ops/restore_padding_op_test.cc index 22aae1d71a..c8d49ce465 100644 --- a/onnxruntime/test/contrib_ops/restore_padding_op_test.cc +++ b/onnxruntime/test/contrib_ops/restore_padding_op_test.cc @@ -77,9 +77,9 @@ static void RunRestorePaddingTests( int hidden_size, int total_tokens) { bool use_float16 = false; - const bool disable_cpu = true; - const bool disable_cuda = false; - const bool disable_rocm = true; + constexpr bool disable_cpu = true; + constexpr bool disable_cuda = false; + constexpr bool disable_rocm = true; RunRestorePadding(input_data, output_data, token_offset_data, batch_size, sequence_length, hidden_size, total_tokens, use_float16, disable_cpu, disable_cuda, disable_rocm); diff --git a/onnxruntime/test/framework/tunable_op_test.cc b/onnxruntime/test/framework/tunable_op_test.cc index e8022bb335..43f46d4578 100644 --- a/onnxruntime/test/framework/tunable_op_test.cc +++ b/onnxruntime/test/framework/tunable_op_test.cc @@ -426,10 +426,9 @@ class TunableVecAddHandleInplaceUpdate : public TunableOp { const VecAddParams* PreTuning(const VecAddParams* params) override { if (params->beta != 0) { is_proxy_params_used = true; - GSL_SUPPRESS(i .11) - VecAddParams* proxy = new VecAddParams(*params); + std::unique_ptr proxy = std::make_unique(*params); proxy->c = new int[params->num_elem]; - return proxy; + return proxy.release(); } is_proxy_params_used = false; return params; diff --git a/orttraining/orttraining/test/training_ops/cpu/activation/activation_op_test.cc b/orttraining/orttraining/test/training_ops/cpu/activation/activation_op_test.cc index fc551ae75b..52c01da54b 100644 --- a/orttraining/orttraining/test/training_ops/cpu/activation/activation_op_test.cc +++ b/orttraining/orttraining/test/training_ops/cpu/activation/activation_op_test.cc @@ -211,10 +211,16 @@ TEST(QuickGeluGradTest, Basic) { // Positive alpha. { - const float alpha = 1.702f; + constexpr float alpha = 1.702f; TestElementwiseGradientOp( "QuickGeluGrad", {{"dY", dY}, {"X", x_vals}}, + // The ifdef is to suppress a warning: "lambda capture 'alpha' is not required to be captured for this use." + // But on Windows it is required. +#ifdef __clang__ + [](const std::vector& params) { +#else [alpha](const std::vector& params) { +#endif ORT_ENFORCE(params.size() == 2); const auto dy = params[0], x = params[1]; return QuickGeluGrad(dy, x, alpha); @@ -224,10 +230,14 @@ TEST(QuickGeluGradTest, Basic) { // Silu = x*sigmoid(x), i.e., alpha = 1.0f. { - const float alpha = 1.0f; + constexpr float alpha = 1.0f; TestElementwiseGradientOp( "QuickGeluGrad", {{"dY", dY}, {"X", x_vals}}, +#ifdef __clang__ + [](const std::vector& params) { +#else [alpha](const std::vector& params) { +#endif ORT_ENFORCE(params.size() == 2); const auto dy = params[0], x = params[1]; return QuickGeluGrad(dy, x, alpha); @@ -237,10 +247,14 @@ TEST(QuickGeluGradTest, Basic) { // Negative alpha. { - const float alpha = -1.702f; + constexpr float alpha = -1.702f; TestElementwiseGradientOp( "QuickGeluGrad", {{"dY", dY}, {"X", x_vals}}, +#ifdef __clang__ + [](const std::vector& params) { +#else [alpha](const std::vector& params) { +#endif ORT_ENFORCE(params.size() == 2); const auto dy = params[0], x = params[1]; return QuickGeluGrad(dy, x, alpha); diff --git a/tools/ci_build/github/azure-pipelines/templates/win-ci-vs-2019.yml b/tools/ci_build/github/azure-pipelines/templates/win-ci-vs-2019.yml index 21551243d0..1db620cc5a 100644 --- a/tools/ci_build/github/azure-pipelines/templates/win-ci-vs-2019.yml +++ b/tools/ci_build/github/azure-pipelines/templates/win-ci-vs-2019.yml @@ -236,7 +236,7 @@ jobs: msBuildArchitecture: amd64 setupCommandlines: 'python $(Build.SourcesDirectory)\tools\ci_build\build.py --config RelWithDebInfo --build_dir $(Build.SourcesDirectory)\b --skip_submodule_sync --build_shared_lib --update --cmake_generator "Visual Studio 16 2019" --build_shared_lib --enable_onnx_tests ${{ parameters.additionalBuildFlags }} --cmake_extra_defines onnxruntime_ENABLE_STATIC_ANALYSIS=ON' msBuildCommandline: '"C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\MSBuild\Current\Bin\amd64\msbuild.exe" "$(Build.SourcesDirectory)\b\RelWithDebInfo\onnxruntime.sln" /p:RunCodeAnalysis=true /p:platform=${{ parameters.msbuildPlatform }} /p:configuration=RelWithDebInfo /p:VisualStudioVersion="16.0" /m /p:PreferredToolArchitecture=x64' - excludedPaths: '$(Build.SourcesDirectory)\b#$(Build.SourcesDirectory)\cmake#C:\program files (x86)' + excludedPaths: '$(Build.SourcesDirectory)\b#$(Build.SourcesDirectory)\cmake#C:\program files#C:\program files (x86)' rulesetName: Custom customRuleset: $(Build.SourcesDirectory)\cmake\Sdl.ruleset publishXML: true @@ -253,7 +253,7 @@ jobs: - task: PostAnalysis@2 displayName: 'Guardian Break v2' inputs: - GdnBreakGdnToolSDLNativeRulesSeverity: Warning + GdnBreakGdnToolSDLNativeRulesSeverity: Note GdnBreakGdnToolSDLNativeRules: true diff --git a/tools/ci_build/github/azure-pipelines/win-ci-pipeline.yml b/tools/ci_build/github/azure-pipelines/win-ci-pipeline.yml index 85de2c2241..f2d8853371 100644 --- a/tools/ci_build/github/azure-pipelines/win-ci-pipeline.yml +++ b/tools/ci_build/github/azure-pipelines/win-ci-pipeline.yml @@ -39,7 +39,7 @@ stages: isX86: false job_name_suffix: x64_release RunOnnxRuntimeTests: ${{ parameters.RunOnnxRuntimeTests }} - RunStaticCodeAnalysis: true + RunStaticCodeAnalysis: false isTraining: false ORT_EP_NAME: CPU GenerateDocumentation: false @@ -117,7 +117,7 @@ stages: isX86: true job_name_suffix: x86_release RunOnnxRuntimeTests: ${{ parameters.RunOnnxRuntimeTests }} - RunStaticCodeAnalysis: true + RunStaticCodeAnalysis: false isTraining: false ORT_EP_NAME: CPU GenerateDocumentation: false @@ -174,7 +174,7 @@ stages: isX86: false job_name_suffix: training_on_device_x64_release RunOnnxRuntimeTests: ${{ parameters.RunOnnxRuntimeTests }} - RunStaticCodeAnalysis: true + RunStaticCodeAnalysis: false EnablePython: false isTraining: true ORT_EP_NAME: CPU