Thank you for the analysis and the proposed solution! I can reproduce the issue (with any q.cpp that is not clang-tidy clean):
$ clang-tidy q.cpp -- --serialize-diagnostics test.dia *** Error in `clang_tidy': free(): invalid pointer: 0x00007fffa65bb4d8 *** Aborted (core dumped) The patch seems correct to me and the way to distinguish between owning and non-owning constructors seems also fine. I'll commit the patch tomorrow if nobody offers a better solution. -- Alex On Wed, Nov 12, 2014 at 9:32 PM, Aaron Wishnick <[email protected]> wrote: > Understanding the bug better, I've attached a patch that more correctly > fixes this bug, by teaching ChainedDiagnosticConsumer how to not take > ownership of one of its arguments, and having SetupSerializedDiagnostics() > use it. Is there a more idiomatic way, in the LLVM project, of a "maybe" > owning pointer? I see that some related functions take a "ShouldOwnClient" > argument, but this seems a little more kludgy for two arguments with > separate ownership. > > Thanks, > Aaron > > On Wed, Nov 12, 2014 at 3:14 PM, Aaron Wishnick < > [email protected]> wrote: > >> Hi Alexander, sorry to dig up an old issue, but I've just gotten some >> more time to look into it. This is still reproducing for me on trunk, and I >> can see where the ChainedDiagnosticConsumer is created, as well as why it >> ends up trying to free a stack object. In short, there's a function >> SetupSerializedDiagnostics() in CompilerInstance.cpp that doesn't know how >> to handle the case where its DiagnosticsEngine doesn't own its client. This >> bug can be reproduced by using clang-tidy with a compilation database that >> uses the "--serialize-diagnostics" flag. >> >> When I run a debug build with the arguments "clang-tidy -p >> /path/to/compile_commands.json /path/to/source.cpp", I get a failed assert >> in tools/clang/lib/Frontend/CompilerInstance.cpp, line 173, in >> SetupSerializedDiagnostics(): >> >> static void SetupSerializedDiagnostics(DiagnosticOptions *DiagOpts, >> DiagnosticsEngine &Diags, >> StringRef OutputFile) { >> auto SerializedConsumer = >> clang::serialized_diags::create(OutputFile, DiagOpts); >> >> assert(Diags.ownsClient()); >> Diags.setClient(new ChainedDiagnosticConsumer( >> std::unique_ptr<DiagnosticConsumer>(Diags.takeClient()), >> std::move(SerializedConsumer))); >> } >> >> Stepping one stack frame up into createDiagnostics(), it looks like this >> code path is hit because the "if >> (!Opts->DiagnosticSerializationFile.empty())" condition on line 209 of >> CompilerInstance.cpp is met. >> >> If I skip that assert, and continue, I get that same "pointer being freed >> was not allocated" error, once the program finishes and the >> ChainedDiagnosticConsumer is deleted. The address is from the stack, rather >> than the heap, and it corresponds to the value of "Diags.Client" before >> that call to "Diags.takeClient()." In other words, I think the problem is >> that the DiagnosticsEngine passed into SetupSerializedDiagnostics doesn't >> own its client, and the client is stack allocated, and then the client is >> stored in a unique_ptr which is owned by the ChainedDiagnosticConsumer. >> >> Ultimately, I can see this comes from ClangTidy.cpp, line 470. This >> ClangTidyDiagnosticConsumer is created on the stack, and is the one that >> eventually ends up being freed, causing the bug. >> >> I am using this in conjunction with Xcode: I am using xcodebuild to build >> my project, and then oclint-xcodebuild to generate the >> compile_commands.json database. Sure enough, all of the commands in the >> compilation database include the argument "--serialize-diagnostics >> /path/to/source.dia". If I remove these arguments, this bug doesn't occur. >> So, I think the issue is that SetupSerializedDiagnostics doesn't know how >> to handle the case where the DiagnosticsEngine doesn't own its client. >> >> Hope this helps! >> >> Best, >> Aaron >> >> >> On Mon, Jun 23, 2014 at 9:51 AM, Alexander Kornienko <[email protected]> >> wrote: >> >>> >>> On Mon, Jun 23, 2014 at 2:51 PM, Alexander Kornienko <[email protected]> >>> wrote: >>> >>>> >>>> On Thu, Jun 19, 2014 at 5:59 PM, Aaron Wishnick < >>>> [email protected]> wrote: >>>> >>>>> When I run clang-tidy on OS X 10.9.3, I immediately get this output: >>>>> >>>>> clang-tidy(97903,0x7fff782fb310) malloc: *** error for object >>>>> 0x7fff5fbfecd0: pointer being freed was not allocated >>>>> *** set a breakpoint in malloc_error_break to debug >>>>> >>>>> This occurs inside the destructor of ClangTidyDiagnosticConsumer. >>>>> Here's my callstack: >>>>> >>>>> #4 0x000000010058e3e2 in ~ClangTidyDiagnosticConsumer at >>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/tools/extra/clang-tidy/ClangTidyDiagnosticConsumer.h:190 >>>>> #5 0x0000000100656a73 in >>>>> std::__1::default_delete<clang::DiagnosticConsumer>::operator()(clang::DiagnosticConsumer*) >>>>> const [inlined] at >>>>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/memory:2426 >>>>> #6 0x0000000100656a4b in >>>>> std::__1::unique_ptr<clang::DiagnosticConsumer, >>>>> std::__1::default_delete<clang::DiagnosticConsumer> >>>>> >::reset(clang::DiagnosticConsumer*) [inlined] at >>>>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/memory:2625 >>>>> #7 0x00000001006569f5 in ~unique_ptr [inlined] at >>>>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/memory:2593 >>>>> #8 0x00000001006569f5 in ~unique_ptr [inlined] at >>>>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/memory:2593 >>>>> #9 0x00000001006569f5 in ~ChainedDiagnosticConsumer at >>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/include/clang/Frontend/ChainedDiagnosticConsumer.h:23 >>>>> #10 0x0000000100656595 in ~ChainedDiagnosticConsumer at >>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/include/clang/Frontend/ChainedDiagnosticConsumer.h:23 >>>>> #11 0x00000001006565b9 in ~ChainedDiagnosticConsumer at >>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/include/clang/Frontend/ChainedDiagnosticConsumer.h:23 >>>>> #12 0x00000001015eec84 in ~DiagnosticsEngine at >>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/lib/Basic/Diagnostic.cpp:68 >>>>> #13 0x00000001015eec35 in ~DiagnosticsEngine at >>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/lib/Basic/Diagnostic.cpp:66 >>>>> #14 0x00000001006bd3d3 in >>>>> llvm::RefCountedBase<clang::DiagnosticsEngine>::Release() const at >>>>> /Users/awishnick/clang-tidy/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:55 >>>>> #15 0x00000001006bd325 in >>>>> llvm::IntrusiveRefCntPtrInfo<clang::DiagnosticsEngine>::release(clang::DiagnosticsEngine*) >>>>> at >>>>> /Users/awishnick/clang-tidy/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:90 >>>>> #16 0x00000001006bd2fd in >>>>> llvm::IntrusiveRefCntPtr<clang::DiagnosticsEngine>::release() at >>>>> /Users/awishnick/clang-tidy/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:199 >>>>> #17 0x00000001006bd2c5 in ~IntrusiveRefCntPtr at >>>>> /Users/awishnick/clang-tidy/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:172 >>>>> #18 0x00000001006bbe15 in ~IntrusiveRefCntPtr at >>>>> /Users/awishnick/clang-tidy/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:172 >>>>> #19 0x000000010065cbc1 in ~CompilerInstance at >>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/lib/Frontend/CompilerInstance.cpp:63 >>>>> #20 0x000000010065c505 in ~CompilerInstance at >>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/lib/Frontend/CompilerInstance.cpp:61 >>>>> #21 0x00000001005d6474 in >>>>> clang::tooling::FrontendActionFactory::runInvocation(clang::CompilerInvocation*, >>>>> clang::FileManager*, clang::DiagnosticConsumer*) at >>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/lib/Tooling/Tooling.cpp:270 >>>>> #22 0x00000001005d614f in >>>>> clang::tooling::ToolInvocation::runInvocation(char const*, >>>>> clang::driver::Compilation*, clang::CompilerInvocation*) at >>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/lib/Tooling/Tooling.cpp:243 >>>>> #23 0x00000001005d5290 in clang::tooling::ToolInvocation::run() at >>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/lib/Tooling/Tooling.cpp:229 >>>>> #24 0x00000001005d7b29 in >>>>> clang::tooling::ClangTool::run(clang::tooling::ToolAction*) at >>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/lib/Tooling/Tooling.cpp:360 >>>>> #25 0x0000000100566cd2 in >>>>> clang::tidy::runClangTidy(clang::tidy::ClangTidyOptionsProvider*, >>>>> clang::tooling::CompilationDatabase const&, >>>>> llvm::ArrayRef<std::__1::basic_string<char, std::__1::char_traits<char>, >>>>> std::__1::allocator<char> > >, >>>>> std::__1::vector<clang::tidy::ClangTidyError, >>>>> std::__1::allocator<clang::tidy::ClangTidyError> >*) at >>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/tools/extra/clang-tidy/ClangTidy.cpp:345 >>>>> #26 0x0000000100002a96 in main at >>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/tools/extra/clang-tidy/tool/ClangTidyMain.cpp:145 >>>>> >>>>> In short, it appears that ClangTool takes ownership of the diagnostic >>>>> consumer, but it's being allocated on the stack. My fix is to allocate it >>>>> on the heap instead. I've attached my patch. Please let me know if this >>>>> assessment is incorrect, or if you'd like me to go about this differently. >>>>> >>>> >>>> Well, the ownership of the diagnostic consumer shouldn't be >>>> transferred, and I don't see any evidence ClangTool::setDiagnosticConsumer >>>> expects this to happen. This all looks strange, and I'm investigating this. >>>> >>> >>> I wasn't able to reproduce this crash. Your stack trace has >>> ChainedDiagnosticConsumer in it, which afaiu, it is only used twice in >>> Clang, and both places don't seem to be unrelated to clang-tidy. Could you >>> set a breakpoint in ChainedDiagnosticConsumer constructor and send me the >>> stack trace where it gets called in clang-tidy? (or add an "assert(false);" >>> there to get the stack trace on the console in the assertions-enabled build) >>> >>> >>>> >>>> >>>>> >>>>> Thanks! >>>>> Aaron >>>>> >>>>> >>>>> _______________________________________________ >>>>> cfe-commits mailing list >>>>> [email protected] >>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>> >>>> >> >> On Mon, Jun 23, 2014 at 9:51 AM, Alexander Kornienko <[email protected]> >> wrote: >> >>> >>> On Mon, Jun 23, 2014 at 2:51 PM, Alexander Kornienko <[email protected]> >>> wrote: >>> >>>> >>>> On Thu, Jun 19, 2014 at 5:59 PM, Aaron Wishnick < >>>> [email protected]> wrote: >>>> >>>>> When I run clang-tidy on OS X 10.9.3, I immediately get this output: >>>>> >>>>> clang-tidy(97903,0x7fff782fb310) malloc: *** error for object >>>>> 0x7fff5fbfecd0: pointer being freed was not allocated >>>>> *** set a breakpoint in malloc_error_break to debug >>>>> >>>>> This occurs inside the destructor of ClangTidyDiagnosticConsumer. >>>>> Here's my callstack: >>>>> >>>>> #4 0x000000010058e3e2 in ~ClangTidyDiagnosticConsumer at >>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/tools/extra/clang-tidy/ClangTidyDiagnosticConsumer.h:190 >>>>> #5 0x0000000100656a73 in >>>>> std::__1::default_delete<clang::DiagnosticConsumer>::operator()(clang::DiagnosticConsumer*) >>>>> const [inlined] at >>>>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/memory:2426 >>>>> #6 0x0000000100656a4b in >>>>> std::__1::unique_ptr<clang::DiagnosticConsumer, >>>>> std::__1::default_delete<clang::DiagnosticConsumer> >>>>> >::reset(clang::DiagnosticConsumer*) [inlined] at >>>>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/memory:2625 >>>>> #7 0x00000001006569f5 in ~unique_ptr [inlined] at >>>>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/memory:2593 >>>>> #8 0x00000001006569f5 in ~unique_ptr [inlined] at >>>>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../lib/c++/v1/memory:2593 >>>>> #9 0x00000001006569f5 in ~ChainedDiagnosticConsumer at >>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/include/clang/Frontend/ChainedDiagnosticConsumer.h:23 >>>>> #10 0x0000000100656595 in ~ChainedDiagnosticConsumer at >>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/include/clang/Frontend/ChainedDiagnosticConsumer.h:23 >>>>> #11 0x00000001006565b9 in ~ChainedDiagnosticConsumer at >>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/include/clang/Frontend/ChainedDiagnosticConsumer.h:23 >>>>> #12 0x00000001015eec84 in ~DiagnosticsEngine at >>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/lib/Basic/Diagnostic.cpp:68 >>>>> #13 0x00000001015eec35 in ~DiagnosticsEngine at >>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/lib/Basic/Diagnostic.cpp:66 >>>>> #14 0x00000001006bd3d3 in >>>>> llvm::RefCountedBase<clang::DiagnosticsEngine>::Release() const at >>>>> /Users/awishnick/clang-tidy/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:55 >>>>> #15 0x00000001006bd325 in >>>>> llvm::IntrusiveRefCntPtrInfo<clang::DiagnosticsEngine>::release(clang::DiagnosticsEngine*) >>>>> at >>>>> /Users/awishnick/clang-tidy/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:90 >>>>> #16 0x00000001006bd2fd in >>>>> llvm::IntrusiveRefCntPtr<clang::DiagnosticsEngine>::release() at >>>>> /Users/awishnick/clang-tidy/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:199 >>>>> #17 0x00000001006bd2c5 in ~IntrusiveRefCntPtr at >>>>> /Users/awishnick/clang-tidy/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:172 >>>>> #18 0x00000001006bbe15 in ~IntrusiveRefCntPtr at >>>>> /Users/awishnick/clang-tidy/llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:172 >>>>> #19 0x000000010065cbc1 in ~CompilerInstance at >>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/lib/Frontend/CompilerInstance.cpp:63 >>>>> #20 0x000000010065c505 in ~CompilerInstance at >>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/lib/Frontend/CompilerInstance.cpp:61 >>>>> #21 0x00000001005d6474 in >>>>> clang::tooling::FrontendActionFactory::runInvocation(clang::CompilerInvocation*, >>>>> clang::FileManager*, clang::DiagnosticConsumer*) at >>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/lib/Tooling/Tooling.cpp:270 >>>>> #22 0x00000001005d614f in >>>>> clang::tooling::ToolInvocation::runInvocation(char const*, >>>>> clang::driver::Compilation*, clang::CompilerInvocation*) at >>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/lib/Tooling/Tooling.cpp:243 >>>>> #23 0x00000001005d5290 in clang::tooling::ToolInvocation::run() at >>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/lib/Tooling/Tooling.cpp:229 >>>>> #24 0x00000001005d7b29 in >>>>> clang::tooling::ClangTool::run(clang::tooling::ToolAction*) at >>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/lib/Tooling/Tooling.cpp:360 >>>>> #25 0x0000000100566cd2 in >>>>> clang::tidy::runClangTidy(clang::tidy::ClangTidyOptionsProvider*, >>>>> clang::tooling::CompilationDatabase const&, >>>>> llvm::ArrayRef<std::__1::basic_string<char, std::__1::char_traits<char>, >>>>> std::__1::allocator<char> > >, >>>>> std::__1::vector<clang::tidy::ClangTidyError, >>>>> std::__1::allocator<clang::tidy::ClangTidyError> >*) at >>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/tools/extra/clang-tidy/ClangTidy.cpp:345 >>>>> #26 0x0000000100002a96 in main at >>>>> /Users/awishnick/clang-tidy/llvm/tools/clang/tools/extra/clang-tidy/tool/ClangTidyMain.cpp:145 >>>>> >>>>> In short, it appears that ClangTool takes ownership of the diagnostic >>>>> consumer, but it's being allocated on the stack. My fix is to allocate it >>>>> on the heap instead. I've attached my patch. Please let me know if this >>>>> assessment is incorrect, or if you'd like me to go about this differently. >>>>> >>>> >>>> Well, the ownership of the diagnostic consumer shouldn't be >>>> transferred, and I don't see any evidence ClangTool::setDiagnosticConsumer >>>> expects this to happen. This all looks strange, and I'm investigating this. >>>> >>> >>> I wasn't able to reproduce this crash. Your stack trace has >>> ChainedDiagnosticConsumer in it, which afaiu, it is only used twice in >>> Clang, and both places don't seem to be unrelated to clang-tidy. Could you >>> set a breakpoint in ChainedDiagnosticConsumer constructor and send me the >>> stack trace where it gets called in clang-tidy? (or add an "assert(false);" >>> there to get the stack trace on the console in the assertions-enabled build) >>> >>> >>>> >>>> >>>>> >>>>> Thanks! >>>>> Aaron >>>>> >>>>>
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
