KhronosGroup / NNEF-Tools

The NNEF Tools repository contains tools to generate and consume NNEF documents
https://www.khronos.org/nnef
222 stars 57 forks source link

Memory leaks in arrayType and tupleType functions #60

Closed asdor closed 5 years ago

asdor commented 5 years ago

In this functions static map with value type of const Type* is created. New element is added with calling new operator. It is bad, because this pointer will not be released and we will get memory leak. Instead of this, we need to use smart pointers.

gyenesvi commented 5 years ago

I believe a static object is never released, only when a program terminates, so there is no need to release that memory, when the program terminates it is released anyway, so this is not a memory leak.

rgolovanov commented 5 years ago

I believe a static object is never released, only when a program terminates, so there is no need to release that memory, when the program terminates it is released anyway, so this is not a memory leak.

@gyenesvi you are right. There is no functional bug in the generated code,of course all memory will be released, but it won't be graceful object release and this is bad in common case. Any memory leak detector will point to this place, e.g. valgrind:

{
   <insert_a_suppression_name_here>
   Memcheck:Leak
   match-leak-kinds: reachable
   fun:_Znwm
   fun:_ZN4nnef9arrayTypeEPKNS_4TypeE
   fun:_ZN4nnefL16stdlibPrototypesEv
   fun:_ZN4nnef10FlatParser15buildPrototypesEv
   fun:_ZN4nnef10FlatParser5parseERSiPKcRNS_6Parser8CallbackE
   fun:main
}

In this particular case we implemented our own parser NNEF->OpenVX based on your code. Since our development process have high requirements to the code quality we usually use all reasonable tools to proof and support code quality (profilers, leak detectors, static analyzers, etc.).

We could skip this problem (e.g. suppress in valgrind) but decided to report and provide the fix to make open source code a bit better.

gyenesvi commented 5 years ago

Okay, then it makes sense, I will review the PR.

rgolovanov commented 5 years ago

@gyenesvi thank you!