Fix (issue #1336) dangling pointer access violation.

[oicchris]

Fixes (#1336) a dangling pointer access violation when Expression<>::valueAndJacobianMap() is called using a MSVC compiler. Deallocation of the pointer occurs before internal::ExecutionTrace trace's dtor is called. Inside the dtor the dangling pointer is dereferenced.

On a side note, I'm wondering why I was able to execute the unpatched code in Release. I only encountered this issue when I ran in Debug. Is it possible that the legal checks get "optimized" out in Release? I'm guessing not because that'd be scary and reminiscent of the old Win9x days.

[varunagrawal]

LGTM.

Regarding Debug vs Release, that's interesting. Unfortunately, not a lot of us use Windows for GTSAM so at least I don't have the expertise there.

[dellaert]

Asserts are only run in Debug mode. We recommend developing code in Debug mode then switch to Release for ops.

[oicchris]

Hmm I wonder if I forked at a bad commit. Let me update/merge and I'll re-up my PR.

[dellaert]

If CI passes with this, all for it. After that, you might also want to squash and PR to release/4.2 if you're up for that. I would wait to cut the 4.2 release if you tell me you'll do that.

[oicchris]

Weird, some Ubuntu Python debug-only builds are failing and the error messages aren't forthcoming. In any case, I'm new to github and not sure the correct workflow to do as you asked. In my organization's git, we have the process simplified to cherry pick the changes into the Release branch. If it is simply to fork the release/4.2 branch and apply the changes, along with a commit log reference to the develop branch commit, I could do that. Please advise, thank you!

[oicchris]

I did a search on the line: "g++-7: internal compiler error: Killed (program cc1plus)". According to this stackoverflow, it is possible that the CI is running out of memory. However, some diagnostics might be needed, like the dmesg utility mentioned in the article. Do you have more information on the actual cause of the error?

[oicchris]

Ahh, it appears that this issue is not isolated to my commit, eg. PR #1410. Perhaps I shouldn't have based this fix off of the develop branch? Should I fork release/4.2 to apply the fix and present a PR?

[dellaert]

I really think it's just an OOM issue - my colleague Kartik was able to compile and run the gcc CI locally.

[dellaert]

thanks @oicchris ! Since you already forked, you should just check out the release/2.4 branch and “cherry-pick” your commits above. You can then do a second PR, making sure the target is the release/2.4 branch.