issue-2282: Client changes - [Bug] Crash when hovering on a static array

[shewitt-au]

This is an attempt at a fix for issue 2282: [Bug] Crash when hovering on a static array

There are changes to both ImHex and PatternLanguage.
PatternLanguages changes here

Initial debugging suggested a use after free in the parent chain of patterns (0xbaadf00d).

I replaced Pattern’s parent pointer with a shared_ptr. I had to perform CPR (Compile-Patch-Repeat. I just made that up. I dare you to look at me with a straight face and tell me it’s not funny) numerous times. I was slowed down by the time it takes to compile ImHex with even modest changes. I think there is way too much implementation in header files. But I digress.

Along the way I fixed other issued as I encountered them. Of note:

• shared_prts initialized with unique_ptrs
• cloning of objects used in std::sort, changing the objects during sorting
• patterns newed but not shared_ptred

I think that it was basically a memory management issue.

All that said, I don’t know the code well, and I made heaps of changes. It’s possible I’ve fixed something and broken other stuff. My money is on something being broken by this.

There are some failed checks.

Oh yeah, there's a const_cast. I'll look into getting rid of that. The compiler was fighting back and I was getting over it.

[paxcut]

I tested the msi installer and it does seem to fix the problem. I ran the test case that created the crash and now I can both open the nodes and hover over the array without crashes or issues

[shewitt-au]

Ok. I have updated both PRs.

I rolled back a lot of the code I was working on as it was getting too error prone. I have added a utility to pattern_language to make the two stage creation easier and to allows gradual/on-demand adoption: lib/include/pl/helpers/construct_shared_object.hpp

I have not patched any of the patterns to use two stage creation yet.

Another issue is I suspect we’ll get std::shared_ptr cycles, and thus memory leaks, because the parent pointer is a std::shared_ptr and one can only assume the parent has std::shared_ptr's to its children. I think the parent pointer should be a weak_ptr.

Builds fine for me. The client side GitHub tests fail however. Not sure why. Perphaps it's building without the matching pattern_language changes?

[shewitt-au]

OK. Seems good for me so far. I've added enough two-stage constructions to fix all the bugs I've encountered. I’m sure there’s more. I've also made the parent pointer weak. A framework is (hopefully) in place to easily fix issues as they arise.

[paxcut]

I wanted to test your changes but your builds are failing. The build includes both the ImHex and pattern language changes and the first error can be traced bask to this line in your Imhex repository.

    std::shared_ptr<construct_shared_object<pl::ptrn::Pattern> Evaluator>Variable(const std::string &name, const ast::ASTNodeTypeDecl *type, const std::optional<Token::Literal> &value, bool outVariable, bool reference, bool templateVariable, bool constant) {

I am not sure where that comes from but if you follow the links in your Imhex repository for the pattern language linked there that what you'll find in line 355. I don't see any commits that have that change so you must be linking some other commit as a submodule.

[shewitt-au]

@paxcut
I put in a fix. Tests running now. I'm confused though. "so you must be linking some other commit as a submodule." How do I check for that?

[shewitt-au]

@paxcut

Failed again. Had another crack. Tests running. They are not failng so quickly. Looks like you were spot on. Thanks for pointing out the root cause. With my git knowledge I would not have figured that out.

[shewitt-au]

Ok. Only two tests failed, and they don't look like they are caused by my changes.

[shewitt-au]

Ok. This PR (and its pattern-side brethren) is now close to where I want it to be for now.

The two-stage construction may be contentious, and I get that, but it was used in four pattens to fix (hopefully) real bugs.

<pl/helpers/construct_shared_object.hpp> is not intended to be an end onto itself. But, for now at least, it has served its purpose. And, to be honest, it was interesting to write. I have never used C++ concepts before. Even from this brief foray I’m blown away.

Firstly, it is a debugging tool. It has made it easier for me to update Pattern code which was causing bugs due to using shared_from_this in constructors. Assuming all the creation code has been updated to use construct_shared_object I can make the construction two-stage by simply adding a post_construct method.

It allowed me to find Pattern creation that bypasses construct_shared_object by making the Pattern’s constructions protected and using the BEFRIEND_SHARED_OBJECT_CREATOR macro to allow construct_shared_object to still create it. This will not pick up such bypasses in a Pattern class’s code itself however.

I think perhaps we need think about using the factory patten for Pattern creation in future.

But, that said, I’ve not has any crashes with importing patterns and associated activity with these changes. I'm sure someone will rectity that.