Fix memory leak reported by ASAN in NNVM to ONNX conversion

[kevinzh92]

Description

Fix memory leak reported by ASAN in NNVM to ONNX conversion of a constant.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

• The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
• Changes are complete (i.e. I finished coding on this PR)
• All changes have test coverage:
• Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
• Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
• Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
• Code is well-documented:
• For user-facing API changes, API doc string has been updated.
• For new C++ functions in header files, their functionalities and arguments are documented.
• For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
• Check the API doc at http:https://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
• To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

• Fixed a memory leak in NNVM to ONNX constants conversion code. The shared_ptr, as written, does not call delete[] on a dynamically allocated array.

Comments

[wkcn]

LGTM. Thank you for the fix!

[larroy]

Thanks for your PR @kevinzh92 I agree with the fix. May I suggest to change this to a vector of float and rename the variable to a more semantic name?

[ChaiBapchya]

@larroy isn't it consistent with the existing naming system?
That is, to call the instance of that class as the corresponding data pointer.

[larroy]

Not really, in those cases you want a semantic name, even if you are wrapping your Cat in a pointer you want to name it:

unique_ptr<Cat> cat = make_unique<Cat>()

And not

unique_ptr<Cat> unique_ptr = make_unique<Cat>()

Semantic naming is important, we have already type information from the compiler and we are in a statically typed language, no need to name a pointer _ptr. And definitely not "data" for the name of a variable, this is a code smell.

[ChaiBapchya]

Gotcha! Makes sense!

[karan6181]

@kevinzh92 Could you please address the review comments?

[kevinzh92]

Sorry guys. Things have been busy at work and this fell off my radar. @larroy, I hope I addressed all of your concerns.

[larroy]

🙅‍♂️this is not correct. You are taking address to the vector itself, you want to take address to the data.

Please use .data() member or the address of the first element.

[KellenSunderland]

Looks like there's still a compiler issue:

/work/mxnet/src/operator/subgraph/tensorrt/nnvm_to_onnx.cc:567:54: error: reinterpret_cast from type 'const short unsigned int*' to type 'int32_t* {aka int*}' casts away qualifiers

                   reinterpret_cast<int32_t&>(constant));

[larroy]

I also have concerns about this line:

                  reinterpret_cast<int32_t&>(constant));

[wkcn]

Hi, I update this code to use T[] as the template type of shared_ptr.

Thank @kevinzh92 for pointing out this issue.

[larroy]

Please rebase

[larroy]

LGTM

[wkcn]

Merged.
It is a tiny change to modify the template type, and keep the original variable name.
We can modify the semantic name in another PR : )
std::vector is not suitable in this PR, since it will allocate extra capacity.

Thank you!