Expressions cause segfault on Windows (MSVC 2019 v16) about gtsam HOT 9 CLOSED

Update: I removed (ptr) in all lines like these:

so it's like

Record* record = new Record(...);

instead of

Record* record = new (ptr) Record(...);

Now the examples seem to work (no crash, low final error). Can someone smarter than me say if this modification is safe?

from gtsam.

Interesting. This is the placement new operator and I remember it being crucial for performance and correctness. I have no idea why it would give problems on windows, unless there is some change in how they handle this (fairly rarely used) C++ feature.

from gtsam.

@laxnpander Could you try modifying

from gtsam.

@ProfFan Still segfaulting. I just realised that Pose2SLAMExampleExpressions.exe works. However, Pose3SLAMExampleExpressions_BearingRangeWithTransform.exe as well as SFMExampleExpressions.exe are crashing. Not sure if this helps. Will recompile your change with debug flag and see if stacktrace changed or not.

Edit: Maybe I was mistaken, Pose2SLAMExampleExpressions seems to be crashing as well.

Stack trace for this:

	Pose2SLAMExampleExpressions.exe!gtsam::internal::ExecutionTrace<gtsam::Pose2>::~ExecutionTrace<gtsam::Pose2>() Zeile 174	C++
 	Pose2SLAMExampleExpressions.exe!gtsam::Expression<gtsam::Pose2>::valueAndJacobianMap(const gtsam::Values & values, gtsam::internal::JacobianMap & jacobians) Zeile 224	C++
 	Pose2SLAMExampleExpressions.exe!gtsam::ExpressionFactor<gtsam::Pose2>::linearize(const gtsam::Values & x) Zeile 139	C++
 	gtsamDebug.dll!gtsam::NonlinearFactorGraph::linearize(const gtsam::Values & linearizationPoint) Zeile 270	C++
 	gtsamDebug.dll!gtsam::GaussNewtonOptimizer::iterate() Zeile 49	C++
 	gtsamDebug.dll!gtsam::NonlinearOptimizer::defaultOptimize() Zeile 95	C++
 	gtsamDebug.dll!gtsam::NonlinearOptimizer::optimize() Zeile 98	C++
 	Pose2SLAMExampleExpressions.exe!main(int argc, char * * argv) Zeile 72	C++

from gtsam.

@ProfFan: Out of curiosity: Did you try running these examples on a windows system? I tried on two different laptops and both were throwing the same error. Same MSVC version though. The CI also just checks for compilation. I just wonder if this is a common issue or related to my configuration...

from gtsam.

It appears the issue is caused by destruction order. In MSVC, the pointer is being deallocated prior to internal::ExecutionTrace being destroyed.
gtsam/nonlinear/Expression-inl.h (209-224)

#ifdef _MSC_VER
  auto traceStorage = static_cast<internal::ExecutionTraceStorage*>(_aligned_malloc(size, internal::TraceAlignment));
#else
  internal::ExecutionTraceStorage traceStorage[size];
#endif

  internal::ExecutionTrace<T> trace;
  T value(this->traceExecution(values, trace, traceStorage));
  trace.startReverseAD1(jacobians);

#ifdef _MSC_VER
  _aligned_free(traceStorage); // <-- Deallocation and trace.content.ptr is now dangling.
#endif

  return value;
} // <-- internal::ExecutionTrace<T>::~ExecutionTrace() dtor calls trace.content.ptr if kind==Function

In the destructor, it is invoking a call on the dangling pointer.
gtsam/nonlinear/internal/ExecutionTrace.h (173-177)

  ~ExecutionTrace() {
    if (kind == Function) {
      content.ptr->~CallRecord<Dim>(); // <-- content.ptr is invalid
    }
  }

In my workaround, I used std::unique_ptr<> with a custom deleter. After this change, my test succeeded. Note that I seem to encounter the error when running in Debug, Release seems to be happy accessing the dangling pointer.

#ifdef _MSC_VER
  std::unique_ptr<void, void(*)(void*)> traceStorageDeleter(
    _aligned_malloc(size, internal::TraceAlignment),
    [](void *ptr) { _aligned_free(ptr); } );
  auto traceStorage = static_cast<internal::ExecutionTraceStorage*>(traceStorageDeleter.get());
#else
  internal::ExecutionTraceStorage traceStorage[size];
#endif

  internal::ExecutionTrace<T> trace;
  T value(this->traceExecution(values, trace, traceStorage));
  trace.startReverseAD1(jacobians);

  return value;
} // <-- traceStorage is deallocated after trace's dtor.

from gtsam.

Hi @oicchris, great sleuthing! Would you be willing to do a quick PR with your fix for the _MSC_VER path?

from gtsam.

Pfff please ignore the force pushes.. was attempting to reword a comment to make it palatable. Didn't realize it would retain the extra pushes. I'm new to Github, is there a way to clean up the above mess?

from gtsam.

Closing, thank you @oicchris !

from gtsam.