AccessViolationException with QtGui with newest master

[golddranks]

Hi, after updating to the newest master, it crashes with Qt5Gui generation, throwing AccessViolationException. It seems to throw it on row 465 of CppParser.cpp, in function

CppSharp::Parser::ParserResult^ CppSharp::Parser::ClangParser::ParseHeader(CppSharp::Parser::ParserOptions^ Opts)

on row:

auto arg0 = (::CppSharp::CppParser::ParserOptions*)Opts->NativePtr;

Any idea why this happens? There has been quite a lot of changes, I cursorily checked some commits from the meantime, but without much success, so I was unable to determine which commit introduced the regression. (Got some other crashes on the earlier commits, so I was unable to inspect this one.)

[tritao]

Post the call stack please, along with the log.

[golddranks]

Whoops, that was just the limit of the managed code. I enabled unmanaged code debugging, and now I get much deeper stack... I'll post it.

[golddranks]

Call stack: https://gist.github.com/golddranks/bcd9c524d5154d73f7aa Log: https://gist.github.com/golddranks/06accd12d2ebae891680

[golddranks]

Okay, some more info: I had everything else in Debug mode, but the C++ code, since RelWithDebInfo LLVM seems to interface badly with debug mode C++. However, I noticed now that I had accidentally optimizations on. After turning them off, the top of the call stack looks like this:

ntdll.dll!NtWaitForSingleObject()  Unknown
KernelBase.dll!WaitForSingleObjectEx() Unknown
msvcr120_clr0400.dll!__C_specific_handler()    Unknown
ntdll.dll!RtlpExecuteHandlerForException() Unknown
ntdll.dll!RtlDispatchException()    Unknown
ntdll.dll!KiUserExceptionDispatch()    Unknown
CppSharp.CppParser.dll!std::basic_string<char,std::char_traits<char>,std::allocator<char> >::compare(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & _Right) Line 2136 C++
CppSharp.CppParser.dll!std::operator==<char,std::char_traits<char>,std::allocator<char> >(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & _Left, const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & _Right) Line 2469    C++
CppSharp.CppParser.dll!CppSharp::CppParser::AST::DeclarationContext::FindFunction::__l9::<lambda>(CppSharp::CppParser::AST::Template * t) Line 402  C++
CppSharp.CppParser.dll!std::_Find_if<CppSharp::CppParser::AST::Template * __ptr64 * __ptr64,bool <lambda>(CppSharp::CppParser::AST::Template *) >(CppSharp::CppParser::AST::Template * * _First, CppSharp::CppParser::AST::Template * * _Last, CppSharp::CppParser::AST::DeclarationContext::FindFunction::__l9::bool <lambda>(CppSharp::CppParser::AST::Template *) _Pred) Line 44 C++
    CppSharp.CppParser.dll!std::find_if<std::_Vector_iterator<std::_Vector_val<std::_Simple_types<CppSharp::CppParser::AST::Template * __ptr64> > >,bool <lambda>(CppSharp::CppParser::AST::Template *) >(std::_Vector_iterator<std::_Vector_val<std::_Simple_types<CppSharp::CppParser::AST::Template *> > > _First, std::_Vector_iterator<std::_Vector_val<std::_Simple_types<CppSharp::CppParser::AST::Template *> > > _Last, CppSharp::CppParser::AST::DeclarationContext::FindFunction::__l9::bool <lambda>(CppSharp::CppParser::AST::Template *) _Pred) Line 56   C++
CppSharp.CppParser.dll!CppSharp::CppParser::AST::DeclarationContext::FindFunction(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & USR) Line 404    C++

The crash happens on row

auto foundTemplate = std::find_if(Templates.begin(), Templates.end(),
    [&](Template* t) { return t->TemplatedDecl->USR == USR; });

and inspecting the values tells that Templates.end() returns 0xababababababab which is Visual Studio's way to tell this:

• 0xABABABAB : Used by Microsoft's HeapAlloc() to mark "no man's land" guard bytes after allocated heap memory

Does that help?

[golddranks]

Or is the endpoint supposed to be invalid, out of range?

[tritao]

As you've discovered, for native C++ crashes it's always better to use debug mode for everything, even though it's a lot slower.

About the crash: I don't think it should be blowing up there... some kind of corruption might be happening before that particular call frame.

I tried to repro this yesterday but my Qt# setup is currently broken and I wasn't able to run anything. I'll try again tonight.

[golddranks]

Disclaimer: I'm a total C++ noob. Well, the end iterator was supposed to be like that, it's supposed to point one element out of the range.

The crash is happening inside the lambda expression. The "Template* t" template pointer seems to be fine, but the contents of its TemplatedDecl value is borked. So I guess the bug itself happens when the value doesn't get set properly where it's supposed to, or gets corrupted for some reason.

[golddranks]

Allright, looks a bit clearer now. It's a recursion order bug. It happens when walking qgenericmatrix.h. At first, the translation unit namespace is empty, then in WalkClassTemplate it adds a Template to the Templates vector (NS->Templates.push_back(CT);) . However, the initialization of TemplatedDecl isn't finished yet; it tries to do that (CT->TemplatedDecl = WalkRecordCXX(TD->getTemplatedDecl());). However in order to do that, it calls WalkRecordCXX, and while walking deeper, it encounters the function WalkFunction, which tries to find a function from the Translation Unit's namespace, and expects all the templated functions it searches through to be fully initialized -> crash.

[golddranks]

It seems that the lately added friend declarations introduced or surfaced this, since the walk path goes through a friend declaration.

[golddranks]

This quick & dirty patch fixes the bug:

- CT->TemplatedDecl = WalkRecordCXX(TD->getTemplatedDecl());
+ bool Process;
+ CT->TemplatedDecl = GetRecord(TD->getTemplatedDecl(), Process);
+ WalkRecordCXX(TD->getTemplatedDecl());

It sets the TemplatedDecl before walking deeper. However, this is ugly and should be refactored. But is the main gist okay? In general, shouldn't the initialization of current node finish before moving further to prevent referencing half-initialized nodes? If that's the case, I think that the following row (CT->Parameters = WalkTemplateParameterList(TD->getTemplateParameters());) might pose a risk too.

[tritao]

I can reproduce the problem, I'll need to think a bit about your fix to see if it's the right way to fix this.

Thanks for working on it.

[golddranks]

As it is, the AST walker doesn't generally finalize the nodes before recursing, since it calls WalkThisAndThat right away and finalises after it has parsed the subtree. This doesn't pose a threat in a tree or a directed acyclic graph, and as long as only the lexical AST is being walked, shouldn't be a problem here (because it's strictly a tree)... the problem seems to be that if we follow all the semantic references right away, (types, base classes, friend classes etc.) there might be - and there is - cycles. Is the design of the AST walker considered carefully enough?

[tritao]

The parser code has been written mostly in an ad-hoc way and we've been fixing problems as they've come up. Me and @ddobrev have been long talking of an overdue re-design of it but neither of us has had the time or motivation at the moment to do such a big task and there's been almost no bugs for a while with the current code anyway.

About the fix, I did test it yesterday evening and it works! It looks good to me and your explanation makes sense. I just didn't push it yet because after thinking about @ddobrev's friend declarations pull I'm almost certain it needs to be re-done.

Due to the way friend declarations are processed they'll be added as regular members of the class which is wrong. We'll need to extend the AST with a FriendDecl and expose that to C# so that it can do special processing on them.

I was waiting to get that done to pull your fix, since it might need some tweaking.