{"action":"created","issue":{"url":"https://api.github.com/repos/RobotLocomotion/drake/issues/3462","repository_url":"https://api.github.com/repos/RobotLocomotion/drake","labels_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/3462/labels{/name}","comments_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/3462/comments","events_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/3462/events","html_url":"https://github.com/RobotLocomotion/drake/pull/3462","id":177242935,"number":3462,"title":"Fix runAtlasWalking on OSX in Release mode (#2165)","user":{"login":"tkoolen","id":2585880,"avatar_url":"https://avatars.githubusercontent.com/u/2585880?v=3","gravatar_id":"","url":"https://api.github.com/users/tkoolen","html_url":"https://github.com/tkoolen","followers_url":"https://api.github.com/users/tkoolen/followers","following_url":"https://api.github.com/users/tkoolen/following{/other_user}","gists_url":"https://api.github.com/users/tkoolen/gists{/gist_id}","starred_url":"https://api.github.com/users/tkoolen/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/tkoolen/subscriptions","organizations_url":"https://api.github.com/users/tkoolen/orgs","repos_url":"https://api.github.com/users/tkoolen/repos","events_url":"https://api.github.com/users/tkoolen/events{/privacy}","received_events_url":"https://api.github.com/users/tkoolen/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":{"login":"jwnimmer-tri","id":17596505,"avatar_url":"https://avatars.githubusercontent.com/u/17596505?v=3","gravatar_id":"","url":"https://api.github.com/users/jwnimmer-tri","html_url":"https://github.com/jwnimmer-tri","followers_url":"https://api.github.com/users/jwnimmer-tri/followers","following_url":"https://api.github.com/users/jwnimmer-tri/following{/other_user}","gists_url":"https://api.github.com/users/jwnimmer-tri/gists{/gist_id}","starred_url":"https://api.github.com/users/jwnimmer-tri/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jwnimmer-tri/subscriptions","organizations_url":"https://api.github.com/users/jwnimmer-tri/orgs","repos_url":"https://api.github.com/users/jwnimmer-tri/repos","events_url":"https://api.github.com/users/jwnimmer-tri/events{/privacy}","received_events_url":"https://api.github.com/users/jwnimmer-tri/received_events","type":"User","site_admin":false},"assignees":[{"login":"jwnimmer-tri","id":17596505,"avatar_url":"https://avatars.githubusercontent.com/u/17596505?v=3","gravatar_id":"","url":"https://api.github.com/users/jwnimmer-tri","html_url":"https://github.com/jwnimmer-tri","followers_url":"https://api.github.com/users/jwnimmer-tri/followers","following_url":"https://api.github.com/users/jwnimmer-tri/following{/other_user}","gists_url":"https://api.github.com/users/jwnimmer-tri/gists{/gist_id}","starred_url":"https://api.github.com/users/jwnimmer-tri/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jwnimmer-tri/subscriptions","organizations_url":"https://api.github.com/users/jwnimmer-tri/orgs","repos_url":"https://api.github.com/users/jwnimmer-tri/repos","events_url":"https://api.github.com/users/jwnimmer-tri/events{/privacy}","received_events_url":"https://api.github.com/users/jwnimmer-tri/received_events","type":"User","site_admin":false}],"milestone":null,"comments":19,"created_at":"2016-09-15T17:53:18Z","updated_at":"2016-09-20T19:48:16Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/RobotLocomotion/drake/pulls/3462","html_url":"https://github.com/RobotLocomotion/drake/pull/3462","diff_url":"https://github.com/RobotLocomotion/drake/pull/3462.diff","patch_url":"https://github.com/RobotLocomotion/drake/pull/3462.patch"},"body":"Fixes https://github.com/RobotLocomotion/drake/issues/2165.\r\n~~Hopefully also fixes https://github.com/RobotLocomotion/drake/issues/3147 (pending CI build results). Note that `runAtlasManip` still doesn't pass on OSX Release (don't know about other configurations), but that's because the robot flips out, not because of a crash, so I think a new issue should be opened for that.~~\r\n\r\nCrash happened when `param_sets` got move-assigned (i.e. assigned from an rvalue), but not when it was copy-assigned from an lvalue (as demonstrated by https://github.com/rdeits/drake/commit/6de25e992a321521af21cb75ac90314e248409e4). There are real differences between the implementation of a `std::map`'s move assignment operator and its copy assignment operator, so it's not that weird that there is a difference in behavior.\r\n\r\nStackOverflow question that describes the same behavior: https://stackoverflow.com/questions/38403189/move-assigning-a-stdmap-with-an-aligned-value-type-segfaults. I was able to reproduce their minimal example on my machine, and it only results in a segfault in release mode, not in debug, consistent with what we're seeing. This example runs fine with O0, but segfaults with O1, O2 and O3.\r\n\r\nHere's a modified version of that example that includes Eigen:\r\n\r\n```C++\r\n#include <iostream>\r\n#include <map>\r\n#include <Eigen/Core>\r\n\r\nusing Eigen::Vector4d;\r\n\r\nstruct Foo {\r\n  double a;\r\n  Vector4d b; // fixed size vectorizable type\r\n  EIGEN_MAKE_ALIGNED_OPERATOR_NEW\r\n};\r\n\r\ntemplate <typename Key, typename T>\r\nusing eigen_aligned_map =\r\nstd::map<Key, T, std::less<Key>, Eigen::aligned_allocator<std::pair<Key const, T> > >;\r\n\r\nint main() {\r\n//  std::map<int, Foo> m{{0, {1., Vector4d::Random()}}};\r\n//  std::map<int, Foo> n{{1, {1., Vector4d::Random()}}};\r\n\r\n  eigen_aligned_map<int, Foo> m{{0, {1., Vector4d::Random()}}};\r\n  eigen_aligned_map<int, Foo> n{{1, {1., Vector4d::Random()}}};\r\n\r\n  std::cout << \"GOT HERE\" << std::endl;\r\n  m = std::move(n);  //  segfault happens here\r\n  std::cout << \"GOT HERE TOO\" << std::endl;\r\n}\r\n```\r\n\r\nStrangely enough, for this second example, things are fine for O0 *and* O1, but bad for O2 and O3, regardless of whether I use `std::map` or `eigen_aligned_map`. So following Eigen's standard rules for using fixed-size vectorizable types in STL containers (https://eigen.tuxfamily.org/dox-devel/group__TopicStlContainers.html) does not work for this case.\r\n\r\nI'm not sure whose bug this is (Eigen's or Clang's). I'll file a bug report with Eigen first, see what they say about it.\r\n\r\nThis PR works around the issue by using the copy-assignment operator instead, as @rdeits noticed already.\r\n\r\nIn addition, there were some pretty pervasive Eigen alignment issues. These may have contributed to #3147 as well.\r\n\r\nAlso adds `drake::eigen_aligned_std_map`.\r\n\r\n<!-- Reviewable:start -->\r\n---\r\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"34\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/robotlocomotion/drake/3462)\r\n<!-- Reviewable:end -->\r\n"},"comment":{"url":"https://api.github.com/repos/RobotLocomotion/drake/issues/comments/248413568","html_url":"https://github.com/RobotLocomotion/drake/pull/3462#issuecomment-248413568","issue_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/3462","id":248413568,"user":{"login":"tkoolen","id":2585880,"avatar_url":"https://avatars.githubusercontent.com/u/2585880?v=3","gravatar_id":"","url":"https://api.github.com/users/tkoolen","html_url":"https://github.com/tkoolen","followers_url":"https://api.github.com/users/tkoolen/followers","following_url":"https://api.github.com/users/tkoolen/following{/other_user}","gists_url":"https://api.github.com/users/tkoolen/gists{/gist_id}","starred_url":"https://api.github.com/users/tkoolen/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/tkoolen/subscriptions","organizations_url":"https://api.github.com/users/tkoolen/orgs","repos_url":"https://api.github.com/users/tkoolen/repos","events_url":"https://api.github.com/users/tkoolen/events{/privacy}","received_events_url":"https://api.github.com/users/tkoolen/received_events","type":"User","site_admin":false},"created_at":"2016-09-20T19:48:16Z","updated_at":"2016-09-20T19:48:16Z","body":"I was going to use this PR as an experiment for that :-)\n\n---\n\nReview status: 11 of 14 files reviewed at latest revision, 6 unresolved discussions.\n\n---\n\n*[drake/common/eigen_stl_types.h, line 21 at r3](https://reviewable.io:443/reviews/robotlocomotion/drake/3462#-KS7oLZkK9-Ds3rj3Svx:-KS86F5MmeVRrr8cBWBv:b-896fix) ([raw file](https://github.com/robotlocomotion/drake/blob/2683b3d6777ad9077bd76bb66bed0aedf73185be/drake/common/eigen_stl_types.h#L21)):*\n<details><summary><i>Previously, jwnimmer-tri (Jeremy Nimmer) wrote…</i></summary>\n> What about a simple unit test case (duplicate the unordered_map one) to confirm that this typedef is correct?  A lot of the uses are in conditionally-compiled code.\n</details>\nDone.\n\n---\n\n*[drake/systems/controllers/controlUtil.h, line 4 at r3](https://reviewable.io:443/reviews/robotlocomotion/drake/3462#-KS7qok1x3NG596Zv1aK:-KS86wXE1scinMGLoQyz:b-896fix) ([raw file](https://github.com/robotlocomotion/drake/blob/2683b3d6777ad9077bd76bb66bed0aedf73185be/drake/systems/controllers/controlUtil.h#L4)):*\n<details><summary><i>Previously, jwnimmer-tri (Jeremy Nimmer) wrote…</i></summary>\n> Nope, should have stayed where it was.  A blank line between std and Eigen includes will let clang-format preserve this.  #2269 would also help.\n</details>\nDone.\n\n---\n\n*[drake/systems/controllers/QPCommon.h, line 172 at r3](https://reviewable.io:443/reviews/robotlocomotion/drake/3462#-KS7rox4JfYN2v7QTxyE:-KS876SD1c0KHtP2hlWT:bcg1gtg) ([raw file](https://github.com/robotlocomotion/drake/blob/2683b3d6777ad9077bd76bb66bed0aedf73185be/drake/systems/controllers/QPCommon.h#L172)):*\n<details><summary><i>Previously, jwnimmer-tri (Jeremy Nimmer) wrote…</i></summary>\n> BTW I am not seeing why this is required?  Abundance of caution / consistency?\n</details>\n`Vector6d` is fixed-size vectorizable.\n\n---\n\n*[drake/util/yaml/yamlUtil.cpp, line 326 at r3](https://reviewable.io:443/reviews/robotlocomotion/drake/3462#-KS7pMy3GVIm0dme3iuo:-KS87S86sE-zDO2nAfWS:b-8dqqsq) ([raw file](https://github.com/robotlocomotion/drake/blob/2683b3d6777ad9077bd76bb66bed0aedf73185be/drake/util/yaml/yamlUtil.cpp#L326)):*\n<details><summary><i>Previously, jwnimmer-tri (Jeremy Nimmer) wrote…</i></summary>\n> BTW `QPControllerParams` doesn't use`EIGEN_MAKE_ALIGNED_OPERATOR_NEW`; why does a collection of it need this map type -- just an abundance of caution?  Would you prefer to add the `_NEW` onto it also just in case?\n</details>\nQPControllerParams *does* use EIGEN_MAKE_ALIGNED_OPERATOR_NEW (and it should).\n\n---\n\n*[drake/util/yaml/yamlUtil.h, line 10 at r3](https://reviewable.io:443/reviews/robotlocomotion/drake/3462#-KS7pzrG4aIVwWj5rxAV:-KS87oAwB9gCX7nHNUN4:b-896fix) ([raw file](https://github.com/robotlocomotion/drake/blob/2683b3d6777ad9077bd76bb66bed0aedf73185be/drake/util/yaml/yamlUtil.h#L10)):*\n<details><summary><i>Previously, jwnimmer-tri (Jeremy Nimmer) wrote…</i></summary>\n> Should remain prior to all drake includes (third-party library) but follow the stdlib includes.\n> \n> ```\n> ...\n> #include <map>\n>  \n> #include \"yaml-cpp/yaml.h\"\n> \n> #include \"drake/common/eigen_stl_types.h\"\n> ...\n> ```\n</details>\nDone.\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/robotlocomotion/drake/3462#-:-KS854FfP1CBk93FBLtG:bn8vegh)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"tkoolen"}}