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

[WIP] Possible ODR failure #8910

Closed
wants to merge 2 commits into from
Closed

Conversation

sherm1
Copy link
Member

@sherm1 sherm1 commented May 31, 2018

This is an attempt to reproduce the ODR failure I reported on Slack #cpp. This should have CI failures.

Slack discussion:

sherm1 [2:10 PM]
TL;DR: Likely not a good idea to rely on the address of function static variables to be invariant across all executing code.

I ran into a case where depending on a static object to have the same address in all the code using it failed, specifically in python test cases, which I believe use a shared library not used in other tests(?). I had a never-destroyed "dummy" value defined like this in cache.h:

static CacheEntryValue& dummy() {
    static never_destroyed<CacheEntryValue> dummy;
    return dummy.access();
  }

I was using ptr==&dummy as a flag to tell me that ptr wasn't set to anything useful. A DRAKE_DEMAND(ptr==&dummy) failed in a case where it should have succeeded. Switching to an explicit bool flag to indicate whether ptr is meaningful fixed the problem (should have done that in the first place!).

I speculate that this would have worked if I'd put the definition of dummy() in a .cc rather than a header (didn't try that as a fix since the mechanism seems brittle). (edited)

jwnimmer-tri [2:18 PM]
@sherm1 That result would indicate a build system misconfiguration. It is supposed to work. Can you post a repro for us? Which test failed? (edited)
Possibly it is something like #8908, where we were violating ODR. I also noted in #python that the pydrake bindings for //examples code are also broken with respect to ODR.


This change is Reviewable

sherm1 added 2 commits May 31, 2018 10:06
Rearchitect fixed input ports to support invalidation.
Simplified SystemOutput.
Get rid of unnecessary data structures in diagram context.
Moved FixInputPort to ContextBase.
Convert spring-mass test to take advantage of cached derivatives.
Modify depth sensor to use the cache.
Add validate step in context allocation.
Rename Freestanding -> Fixed.
Merge changes from PRs: dependency tracker, cache, cache entry,
  cloneable DiagramContinuous/DiscreteState, connect base classes,
  output port allocator context parameter removal, PR RobotLocomotion#8623,
  input port PR RobotLocomotion#8760, diagram output PR RobotLocomotion#8857.
Untangle output ports from system and move implementation to header.
Fix handling of built-in trackers for well-known cache entries.
@EricCousineau-TRI
Copy link
Contributor

Hup, did not realize that this ODR issue was related to the Python stuff, d'oh!
Relates (most likely caused by) #8911

That being said, you mentioned that you have a workaround in which you just use a bool to indicate a meaningful ptr? (Trying to figure out priority of resolving #8911, possibly #7294)

@sherm1
Copy link
Member Author

sherm1 commented May 31, 2018

Oh good I see this did reproduce the problem I had (see CI failures).

That being said, you mentioned that you have a workaround ...

Right, I don't have a problem at the moment. I had introduced an unnecessary dependency on the address being unique; doing it in a more straightforward way avoids trouble and is nicer code anyway.

@sherm1
Copy link
Member Author

sherm1 commented May 31, 2018

BTW I have seen this same problem with the Microsoft linker. I think enforcing ODR with inline static definitions across shared libraries must be a real nightmare. Might make sense to prohibit dependence on such linker wizardry in our coding standards. I repent!

@jwnimmer-tri jwnimmer-tri self-assigned this Jun 1, 2018
@jwnimmer-tri
Copy link
Collaborator

The failures were in:

//bindings/pydrake/examples:py/acrobot_test
//bindings/pydrake/examples:py/compass_gait_test
//bindings/pydrake/examples:py/pendulum_test
//bindings/pydrake/examples:py/rimless_wheel_test
//bindings/pydrake/examples:py/van_der_pol_test
//bindings/pydrake/systems:py/controllers_test
//bindings/pydrake/systems:py/trajectory_optimization_test

The first five are tests of examples, and the second two import examples (pendulum) as part of the test. Thus, this is caused by #8911 so I'll close it as a duplicate of that issue. Thanks for the concrete example, @sherm1!

@sherm1 sherm1 deleted the odr-failure branch November 29, 2018 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants