+dblakie On Thu Nov 13 2014 at 1:53:39 AM Alexander Kornienko <[email protected]> wrote:
> 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. > I don't really see anything better under the current restrictions. Perhaps David has an idea, he has done a lot of the unique_ptr migrations in llvm. > > -- 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 >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
