Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix segment fault on 32-bit machine #459

Merged
merged 8 commits into from
Aug 21, 2018

Conversation

dqyi11
Copy link
Contributor

@dqyi11 dqyi11 commented Aug 17, 2018

This PR fixes the last issue left in #365
I have tested it on 32-bit machine.

The previous code uses createState() of a state space to a State raw point, which causes problem in ownership and thus memory leak.
The timilimit in the unit test is increased for successul rate.


Before creating a pull request

  • Document new methods and classes (N/A)
  • Format code with make format

Before merging a pull request

  • Set version target by selecting a milestone on the right side
  • Summarize this change in CHANGELOG.md
  • Add unit test(s) for this change (N/A)

@dqyi11 dqyi11 changed the title Fix segment fault32 bit machine Fix segment fault on 32-bit machine Aug 17, 2018
@codecov
Copy link

codecov bot commented Aug 17, 2018

Codecov Report

Merging #459 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #459      +/-   ##
==========================================
- Coverage   78.89%   78.88%   -0.02%     
==========================================
  Files         258      258              
  Lines        6303     6300       -3     
==========================================
- Hits         4973     4970       -3     
  Misses       1330     1330
Impacted Files Coverage Δ
...anner/vectorfield/detail/VectorFieldIntegrator.hpp 100% <ø> (ø) ⬆️
...anner/vectorfield/detail/VectorFieldIntegrator.cpp 94.73% <100%> (-0.27%) ⬇️

CHANGELOG.md Outdated
@@ -49,6 +49,7 @@
* Changed interface for TrajectoryPostProcessor: [#341](https://github.com/personalrobotics/aikido/pull/341)
* Planning calls with InverseKinematicsSampleable constraints explicitly set MetaSkeleton to solve IK with: [#379](https://github.com/personalrobotics/aikido/pull/379)
* Added a kinodynamic timer that generates a time-optimal smooth trajectory without completely stopping at each waypoint: [#443](https://github.com/personalrobotics/aikido/pull/443)
* Fix segment fault on 32 bit machine in vector-field planner: [#459](https://github.com/personalrobotics/aikido/pull/459)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Fixed

//==============================================================================
VectorFieldIntegrator::~VectorFieldIntegrator()
{
mVectorField->getStateSpace()->freeState(mState);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest using ScopedState instead of we take the responsibility of the memory allocation/deallocation.

jslee02
jslee02 previously approved these changes Aug 18, 2018
@dqyi11
Copy link
Contributor Author

dqyi11 commented Aug 20, 2018

I have tested this branch on a 32bit machine. All the previous tests are passed.
There are two new tests failed, which are 49 - test_JointAvoidanceConfigurationRanker (OTHER_FAULT) and 50 - test_NominalConfigurationRanker (Failed).

The error received in running test_JointAvoidanceConfigurationRanker is:

Running main() from gtest_main.cc
[==========] Running 2 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 2 tests from JointAvoidanceConfigurationRankerTest
[ RUN ] JointAvoidanceConfigurationRankerTest.Constructor
[ OK ] JointAvoidanceConfigurationRankerTest.Constructor (1 ms)
[ RUN ] JointAvoidanceConfigurationRankerTest.OrderTest
test_JointAvoidanceConfigurationRanker: /usr/include/eigen3/Eigen/src/Core/DenseStorage.h:78: Eigen::internal::plain_array<T, Size, MatrixOrArrayOptions, 16>::plain_array() [with T = double; int Size = 2; int MatrixOrArrayOptions = 0]: Assertion `(reinterpret_cast<size_t>(eigen_unaligned_array_assert_workaround_gcc47(array)) & 0xf) == 0 && "this assertion is explained here: " "http://eigen.tuxfamily.org/dox-devel/group__TopicUnalignedArrayAssert.html" " **** READ THIS WEB PAGE !!! ****"' failed.
Aborted (core dumped)

The error received in running test_NominalConfigurationRanker is:

Running main() from gtest_main.cc
[==========] Running 2 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 2 tests from NominalConfigurationRankerTest
[ RUN ] NominalConfigurationRankerTest.Constructor
[ OK ] NominalConfigurationRankerTest.Constructor (1 ms)
[ RUN ] NominalConfigurationRankerTest.OrderTest
/home/daqing/catkin_ws/src/aikido/tests/distance/test_NominalConfigurationRanker.cpp:109: Failure
Value of: aikido::tests::CompareEigenMatrices(rankedState, jointPositions[i], EPS)
Actual: false (Matrices differ in row 0, column 0: 0.29999999999999999 !=: 0.10000000000000001.)
Expected: true
/home/daqing/catkin_ws/src/aikido/tests/distance/test_NominalConfigurationRanker.cpp:109: Failure
Value of: aikido::tests::CompareEigenMatrices(rankedState, jointPositions[i], EPS)
Actual: false (Matrices differ in row 0, column 0: 0.10000000000000001 !=: 0.20000000000000001.)
Expected: true
/home/daqing/catkin_ws/src/aikido/tests/distance/test_NominalConfigurationRanker.cpp:109: Failure
Value of: aikido::tests::CompareEigenMatrices(rankedState, jointPositions[i], EPS)
Actual: false (Matrices differ in row 0, column 0: 0.20000000000000001 !=: 0.29999999999999999.)
Expected: true
[ FAILED ] NominalConfigurationRankerTest.OrderTest (1 ms)
[----------] 2 tests from NominalConfigurationRankerTest (2 ms total)
[----------] Global test environment tear-down
[==========] 2 tests from 1 test case ran. (2 ms total)
[ PASSED ] 1 test.
[ FAILED ] 1 test, listed below:
[ FAILED ] NominalConfigurationRankerTest.OrderTest

@aditya-vk do you have any idea?

@jslee02
Copy link
Member

jslee02 commented Aug 20, 2018

@dqyi11 Thanks for the fix! However, I'm questionable if we really want to support 32 bit platforms. Do we have any use case? If we decide to do so, I think we should have a CI setup so that we can continuously have surveillance on the failings. Otherwise, let's not bother 32-bit issues. @brianhou @gilwoolee @aditya-vk thoughts? :)

@brianhou
Copy link
Contributor

brianhou commented Aug 21, 2018

I don't particularly care about 32-bit platforms, but it seems like it might reveal otherwise hidden bugs (e.g. this mixup between States vs ScopedStates). However, I also feel like our builds already take an incredibly long time. Maybe this is something we can run only on master on Ubuntu and only in Debug?

@dqyi11
Copy link
Contributor Author

dqyi11 commented Aug 21, 2018

I think the failures in 32-bit machine are caused by multiple reasons. Most of them are related with memory operation. Fixing them could also improve the robustness of aikido on 64-bit machine.

Maybe we can close this one. Because all the problems we found previously are addressed.
We will open new issues with explained reasons/details if we encounter in the future.

@brianhou brianhou merged commit e90d24e into master Aug 21, 2018
@brianhou brianhou deleted the bugfix/dqyi/fixSegmentFault32BitMachine branch August 21, 2018 17:49
gilwoolee pushed a commit that referenced this pull request Jan 21, 2019
* fix memory leak

* increase timelimit for success rate

* update CHANGELOG

* change to use scopedState

* Update CHANGELOG.md

* Minor wording fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants