Accidental Type Punning in Driver Station synchronization

[jkoritzinsky]

I was just looking through some of the code and I noticed something I thought was interesting/concerning with our Driver Station synchronization code. When we call InitializeMultiWait here, we get back a pointer to a std::condition_variable from the HAL, which we see here (Note: in the corresponding header file MULTIWAIT_ID is typedefed to std::condition_variable here). When we call SetNewDataSem here we send the pointer to the std::condition_variable that we just got back. However, setNewDataSem (the C implementation), takes a pthread_cond_t * as a parameter (as seen here). These types don't match, so that's a little dangerous.

However, it currently works because the first member (which is at the address of the full object) of a std::condition_variable in libstdc++ (the C++ standard library that is used on the roboRIO) happens to be a pthread_cond_t, which can be seen on lines 64-74 here. Also note that __gthread_cond_t is defined to be a pthread_cond_t in a different file.

Do we want to do anything about this? It probably won't be an issue at all, but it's something I just wanted to put out there.

[ThadHouse]

Interesting. They changed the semaphore code this summer, and I didn't notice that. I'll post a question in the commit that changed that stuff.

[jkoritzinsky]

Awesome! BTW here's where the typedefs are located: http://gcc.gnu.org/onlinedocs/libstdc++/latest-doxygen/a01214_source.html

And here's where they set the semaphore in their code: https://github.com/robotpy/allwpilib/blob/master/wpilibc/wpilibC++Devices/src/DriverStation.cpp#L49

They don't have the issue with the type punning because they have access to the native_handle method. We can emulate that if we are willing to try to P/Invoke C++ methods (which is possible, but it's a bit of a pain).

[ThadHouse]

I would rather just ask them to change it if possible. Here's were I asked the question.

https://usfirst.collab.net/gerrit/#/c/1008/

Python is going to run into the same issue as well, and they have official commit access, so they should be able to get it changed.

[jkoritzinsky]

I just checked the python code and you're right. I found the same problem over there. I'll notify them of the issue as well.

[ThadHouse]

The reason it probably didn't get noticed is that before this summer initializeMultiWait returned a raw pthread_cond_t. And then they changed it when they moved away from raw pointers this summer. The code was changed to allow windows builds of the HAL (which haven't been implemented yet), but I'm thinking this was thought to not be a big deal. And unless the py guys dug as deep as you did, they might not have noticed the change.

[virtuald]

The multiwait thing used to be a terribly broken stupid hack thing. Hopefully they fixed it when migrating to semaphores.

[ThadHouse]

Well as of right now they broke it more it looks like, but it can be fixed with a fairly easy change. Having no way to get a NATIVE_MULTIWAIT_ID without C++ code is an issue, but can be solved easily.

[virtuald]

If you push a PR to our allwpilib repo, I'll forward it to gerrit. Though -- Thad, if you can comment on gerrit, you probably can push new reviews there.

[jkoritzinsky]

I'll throw together a quick PR to the robotpy/allwpilib repo with the needed function.

[ThadHouse]

I've tried multiple times, and I always get this error.

fatal: Upload denied for project 'allwpilib'
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

Looking here https://usfirst.collab.net/sf/go/artf4148 it seems like even with gerrit comment access you need different permissions to push. I've asked multiple times but have never gotten a response from anyone at WPI.

[jkoritzinsky]

Sent the pull request. Let me know if you need anything else.

[virtuald]

@ThadHouse you can't push directly -- you have to use the git review -s thing to upload as a review. Or something similar to that...

[ThadHouse]

Fixed upstream