Fix SDL warnings in CPU EP #9975

snnn · 2021-12-08T20:40:56Z

Description:

Solve some SDL warnings
Refactor Training GPU Windows CI pipeline job, to make similar to the inference one. And I need to disable the static analysis for now. Fixing the warnings will need more changes, which will make this PR even bigger.
Add a customized SDL ruleset file, which is based on the VC++ default one with a few additional enabled warnings. The list of warnings is picked up from https://docs.microsoft.com/en-us/cpp/code-quality/code-analysis-for-cpp-corecheck?view=msvc-170 . Some of the warnings enforce using unique_ptr/shared_ptr instead of raw pointer to meet our compliance requirement.

When this PR is checked in, if you have code like:

char *buffer = nullptr;
if (useCache)
    buffer = GetCache();
else
    buffer = new char[bufferSize];  // C26400

It will be rejected by static analyzer. Because 26400 is in our SDL.rules file. You may convert it to:

std::unique_ptr<char[]> buffer;
if (useCache)
    buffer = GetCache();
else
    buffer = std::make_unique<char[]>(bufferSize);

We still have a lot C26451 warnings that I couldn't get them fixed this time. So I disabled them to make the CI pass. Next, I will add "C26406 DONT_ASSIGN_RAW_TO_OWNER" to the warning list.

To try the static analyzer locally, please open CMakeLists.txt and uncomment the following lines:

#if(WIN32 AND onnxruntime_ENABLE_STATIC_ANALYSIS)
  #  set_target_properties(${target_name} PROPERTIES VS_USER_PROPS ${PROJECT_SOURCE_DIR}/EnableVisualStudioCodeAnalysis.props)
#endif()

Then build it with "--extra_cmake_defines onnxruntime_ENABLE_STATIC_ANALYSIS=ON".
Motivation and Context

Why is this change required? What problem does it solve?

To meet compliance requirement and have better code quality. For example, C26478 is one of the warnings we will enable through this PR. And it helped me found a bug in conv_transpose_attributes.h

If it fixes an open issue, please link to the issue here.

cmake/onnxruntime_mlas.cmake

orttraining/orttraining/test/graph/optimizer_graph_builder_test.cc

snnn · 2021-12-17T08:22:47Z

include/onnxruntime/core/common/common.h

@@ -253,13 +254,6 @@ void LogRuntimeError(uint32_t session_id, const common::Status& status, const ch
    }                                  \
  } while (0)

-// C++ Core Guideline check suppression.
-#if defined(_MSC_VER) && !defined(__NVCC__) && !defined(__clang__)


Moved it to a new file because onnxruntime::Status also wants to use this macro. But this file includes "core/common/status.h"

snnn · 2021-12-17T19:26:35Z

include/onnxruntime/core/framework/kernel_registry.h

@@ -37,7 +37,7 @@ class KernelRegistry {
  // TODO(Task:132) Make usage of unique_ptr/shared_ptr as out param consistent
  Status TryCreateKernel(const Node& node, const IExecutionProvider& execution_provider,
                         const std::unordered_map<int, OrtValue>& constant_initialized_tensors,
-                         const OrtValueNameIdxMap& mlvalue_name_idx_map, const FuncManager& funcs_mgr,
+                         const OrtValueNameIdxMap& mlvalue_name_idx_map, FuncManager& funcs_mgr,


Change funcs_mgr to non-const. Because this function may modify funcs_mgr's internal states. Previously we hided the fact.

snnn · 2021-12-17T19:27:59Z

include/onnxruntime/core/framework/op_kernel.h

-using KernelCreateFn = std::function<OpKernel*(const OpKernelInfo& info)>;
-using KernelCreatePtrFn = std::add_pointer<OpKernel*(const OpKernelInfo& info)>::type;
+class FuncManager;
+using KernelCreateFn = std::function<Status(FuncManager& func_mgr, const OpKernelInfo& info, std::unique_ptr<OpKernel>& out)>;


Use unique_ptr to replace raw pointer. And make it be more obvious that it depends on a non-const FuncManager.

snnn · 2021-12-17T19:29:41Z

include/onnxruntime/core/session/onnxruntime_cxx_api.h

@@ -950,8 +950,14 @@ struct CustomOpBase : OrtCustomOp {
    OrtCustomOp::GetOutputType = [](const OrtCustomOp* this_, size_t index) { return static_cast<const TOp*>(this_)->GetOutputType(index); };

    OrtCustomOp::KernelCompute = [](void* op_kernel, OrtKernelContext* context) { static_cast<TKernel*>(op_kernel)->Compute(context); };
+#if defined(_MSC_VER) && !defined(__clang__)


This file can not use GSL macros. Because it is directly used by outside users.

include/onnxruntime/core/session/environment.h

onnxruntime/core/session/provider_bridge_ort.cc

onnxruntime/core/util/thread_utils.cc

onnxruntime/core/session/allocator_adapters.cc

onnxruntime/core/providers/cpu/tensor/upsample.h

onnxruntime/core/providers/cpu/rnn/rnn_helpers.cc

onnxruntime/core/providers/cpu/rnn/deep_cpu_gru.cc

onnxruntime/core/providers/cpu/reduction/reduction_ops.cc

onnxruntime/core/providers/cpu/object_detection/roialign.cc

onnxruntime/core/providers/cpu/generator/range.cc

onnxruntime/core/platform/windows/env.cc

include/onnxruntime/core/graph/graph.h

snnn · 2021-12-18T04:05:26Z

onnxruntime/core/providers/cpu/nn/conv_transpose_attributes.h

@@ -50,7 +50,7 @@ struct ConvTransposeAttributes : public ConvAttributes {
    const TensorShape& F_Shape = (filter_shape != nullptr) ? *filter_shape : F->Shape();
    const Tensor* Pads = dynamic_padding ? context->Input<Tensor>(2) : nullptr;
    const Tensor* B = has_bias ? (dynamic_padding ? context->Input<Tensor>(3) : context->Input<Tensor>(2)) : nullptr;
-    const TensorShape& input_shape = X->Shape().Slice(2);
+    TensorShape input_shape = X->Shape().Slice(2);


This change fixed a bug.

The small program below reproduces the issue:

void BadCode(const std::string& s) { std::string p = std::move(s); std::cout << p << std::endl; } int _tmain(int argc, TCHAR* argv[]) { std::string s = "abc"; std::cout << s << std::endl; //now it is empty. return 0; }

Shouldn't the comment read - s "may" be empty after calling BadCode() ? I think the standard only expects that the source object is in a "valid" state after std::move is invoked (which may not necessarily be empty).

You're right.

snnn requested review from edgchen1 and pranavsharma December 8, 2021 20:41

edgchen1 reviewed Dec 8, 2021

View reviewed changes

cmake/onnxruntime_mlas.cmake Outdated Show resolved Hide resolved

edgchen1 reviewed Dec 8, 2021

View reviewed changes

orttraining/orttraining/test/graph/optimizer_graph_builder_test.cc Outdated Show resolved Hide resolved

snnn marked this pull request as draft December 9, 2021 20:04

RyanUnderhill reviewed Dec 10, 2021

View reviewed changes

orttraining/orttraining/test/graph/optimizer_graph_builder_test.cc Outdated Show resolved Hide resolved

snnn force-pushed the snnn/training_sdl branch 3 times, most recently from ed05fe4 to 02dfabb Compare December 14, 2021 03:12

snnn added 5 commits December 13, 2021 21:51

update

f638d7c

update

ed566f5

update

7b763ed

update

12ab030

update

5ae6b24

snnn force-pushed the snnn/training_sdl branch from 9ad19d4 to 5ae6b24 Compare December 14, 2021 05:51

snnn added 15 commits December 14, 2021 14:45

Merge remote-tracking branch 'origin/master' into snnn/training_sdl

2460f82

update

40ef6bf

revert

6973ca5

update

1e409ae

update

d50afad

update

e35319a

update

5f40fbe

Merge remote-tracking branch 'origin/master' into snnn/training_sdl

11a496c

update

aafcd8c

update

5adc41f

Merge remote-tracking branch 'origin/master' into snnn/training_sdl

5511142

update

d52ff30

Merge remote-tracking branch 'origin/master' into snnn/training_sdl

2f6bbc5

stash

f5d3b79

update

ed3ef97

snnn added 6 commits December 16, 2021 22:44

update

8179f49

revert

dde9f8d

update

fbdc03a

update

fdd98c5

update

b2d1238

update

29d1901

snnn commented Dec 17, 2021

View reviewed changes

snnn added 4 commits December 17, 2021 01:15

update

48d0d2a

update

11e0b36

update

2c1865d

Merge remote-tracking branch 'origin/master' into snnn/training_sdl

83ca999

snnn commented Dec 17, 2021

View reviewed changes

snnn added 4 commits December 17, 2021 12:13

update

a36cf83

update

a7ce037

update

c179156

Merge remote-tracking branch 'origin/master' into snnn/training_sdl

2c01a89

hariharans29 reviewed Dec 18, 2021

View reviewed changes

include/onnxruntime/core/session/environment.h Show resolved Hide resolved

RyanUnderhill reviewed Dec 18, 2021

View reviewed changes

update

d730618

pranavsharma previously approved these changes Dec 18, 2021

View reviewed changes

include/onnxruntime/core/graph/graph.h Show resolved Hide resolved

snnn commented Dec 18, 2021

View reviewed changes

update

172eb7f

snnn dismissed pranavsharma’s stale review via 172eb7f December 18, 2021 04:06

Add comment back

69c82f7

hariharans29 approved these changes Dec 20, 2021

View reviewed changes

snnn merged commit 4e9e01c into master Dec 20, 2021

snnn deleted the snnn/training_sdl branch December 20, 2021 04:54

snnn mentioned this pull request Dec 31, 2021

avoid using LocalFree on FormatMessageA buffer #3772

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix SDL warnings in CPU EP #9975

Fix SDL warnings in CPU EP #9975

snnn commented Dec 8, 2021 •

edited

Loading

snnn Dec 17, 2021

snnn Dec 17, 2021

snnn Dec 17, 2021

snnn Dec 17, 2021

snnn Dec 18, 2021

hariharans29 Dec 18, 2021

snnn Dec 18, 2021

Fix SDL warnings in CPU EP #9975

Fix SDL warnings in CPU EP #9975

Conversation

snnn commented Dec 8, 2021 • edited Loading

snnn Dec 17, 2021

Choose a reason for hiding this comment

snnn Dec 17, 2021

Choose a reason for hiding this comment

snnn Dec 17, 2021

Choose a reason for hiding this comment

snnn Dec 17, 2021

Choose a reason for hiding this comment

snnn Dec 18, 2021

Choose a reason for hiding this comment

hariharans29 Dec 18, 2021

Choose a reason for hiding this comment

snnn Dec 18, 2021

Choose a reason for hiding this comment

snnn commented Dec 8, 2021 •

edited

Loading