Memory leak in shaderc_compiler_initialize

[friesofdoom]

static shaderc_util::GlslangInitializer* initializer =
new shaderc_util::GlslangInitializer;

is never deleted.

This causes a whole chain reaction of memory leaks.

[zoddicus]

per libshaderc_util/include/libshaderc_util/compiler.h, GlslangInitializer is a Singleton, so though this is technically memory leak, it isn't a bug. Admittedly the Singleton pattern has not been correctly implemented here, so that should be corrected.

This all should in theory go away with resolving #945

I am going to confirm whether I should refactor this to be a proper Singleton, or if I should just rip it all out.

[friesofdoom]

Interesting. Either way, the singleton should be deleted (I use a "nifty counter" singleton for this)

I've noticed another memory leak deep in the shader compiler, but it seems to only happen in release mode so I've not had a chance to get a clear picture of what's causing it yet (It could just be an std artifact with my memory tracker). I would recommend you to run some tests with a memory leak tracker :)

This is the callstack of the allocation that's leaking (all the info I have):

>	appDemo.exe!std::num_put<char,class std::ostreambuf_iterator<char,struct std::char_traits<char> > >::_Getcat(class std::locale::facet const * *,class std::locale const *)	Unknown
 	appDemo.exe!std::use_facet<class std::num_put<char,class std::ostreambuf_iterator<char,struct std::char_traits<char> > > >(class std::locale const &)	Unknown
 	appDemo.exe!std::basic_ostream<char,struct std::char_traits<char> >::operator<<(bool)	C++
 	appDemo.exe!glslang::TPpContext::CPPinclude(class glslang::TPpToken *)	C++
 	appDemo.exe!glslang::TPpContext::readCPPline(class glslang::TPpToken *)	C++
 	appDemo.exe!glslang::TPpContext::tokenize(class glslang::TPpToken &)	C++
 	appDemo.exe!glslang::TScanContext::tokenize(class glslang::TPpContext *,class glslang::TParserToken &)	C++
 	appDemo.exe!yylex(union YYSTYPE *,class glslang::TParseContext &)	C++
 	appDemo.exe!yyparse(class glslang::TParseContext *)	C++
 	appDemo.exe!glslang::TParseContext::parseShaderStrings(class glslang::TPpContext &,class glslang::TInputScanner &,bool)	C++
 	appDemo.exe!shaderc_result_release�()	C++
 	appDemo.exe!glslang::TSymbolTable::`scalar deleting destructor'(unsigned int)	C++
 	appDemo.exe!glslang::TShader::parse(struct TBuiltInResource const *,int,enum EProfile,bool,bool,enum EShMessages,class glslang::TShader::Includer &)	C++
 	appDemo.exe!shaderc_util::Compiler::Compile(class shaderc_util::string_piece const &,enum EShLanguage,class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &,char const *,class std::function<enum EShLanguage > const &,class shaderc_util::CountingIncluder &,enum shaderc_util::Compiler::OutputType,class std::basic_ostream<char,struct std::char_traits<char> > *,unsigned __int64 *,unsigned __int64 *,class shaderc_util::GlslangInitializer *)	C++
 	appDemo.exe!shaderc_compilation_result_vector::`vector deleting destructor'(unsigned int)	C++
 	appDemo.exe!shaderc_compile_into_spv�()	C++
 	appDemo.exe!shaderc::Compiler::CompileGlslToSpv(class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> > const &,enum shaderc_shader_kind,char const *,char const *,class shaderc::CompileOptions const &)	Unknown

[zoddicus]

Just looked at the bot configs for what sanitizers we are running. There is an ASAN config that is part of kokoro:run, which wouldn't catch leaks, though will catch some other issues.

We should probably also add a LSAN bot at least to catch leaks. For other sanitizers, MSAN & UBSAN could potentially be useful, but if I recall correctly they are more difficult to setup/noisy.

I have filed #1053 for looking into increasing the sanitizer coverage.

[dneto0]

FYI. Years ago our team discussed the tradeoffs of leaking these singleton objects. At the time we chose to deliberately leak them rather than deal with the complexity of correctly releasing them, e.g. in the presence of multiple threads.

The nifty counter (just looked it up), relies C++ constructors and destructors for objects of static duration (so they're initialized before main() and destroyed after exit). That is against Google C++ style for important reasons. https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

In any case, I'm happy for @zoddicus to safely eliminate the mutex around Glslang, which will eliminate one singleton.

Looking at the stack trace, and drawing on experience from over a decade ago, text processing in the C++ standard IO stream uses allocated objects to manage locale translations, particularly for text and numbers. That may be what's leaking here. If that's the case, it may be out of our hands, and is really the responsibility of the underlying C++ runtime.

[zoddicus]

Marking this issue as closed, since #1059 reduces the leak to a single std::mutex, and we have filed other work items to improve sanitizer coverage.