Check for semaphore max value.

[antoinealb]

As discussed in issue #2, we want to fail hard if we release a semaphore more than its initial value.

[msplr]

Are you sure that the counter must never be greater than the initial value? Semaphores are used for several things: mutual exclusion, thread synchronisation and signaling. I think there are some use cases where semaphores have an initial value of 0, which will fail at the test.

[pierluca]

Initial value 0 means that the semaphore blocks access to an unavailable resource. Why create it in the first place? The simplest, meaningful semaphore allows one access, which can be taken immediately. (making it THEN unavailable for others to use)

On Thu, Jun 12, 2014 at 8:16 PM, Michael Spieler notifications@github.com wrote:

Are you sure that the counter must never be greater than the initial value? Semaphores are used for several things: mutual exclusion, thread synchronisation and signaling. I think there are some use cases where semaphores have an initial value of 0, which will fail at the test.

— Reply to this email directly or view it on GitHub https://github.com/cvra/platform-abstraction/pull/3#issuecomment-45928245 .

[msplr]

A simple example would be a synchronized buffer with two semaphores: one for counting the available space and one for counting the available data. At initialization the data-counting semaphore is initialized with 0. This will fail as soon as the buffer is filled and os_semaphore_release() is called.

[msplr]

In the example you described, you are using semaphores as a sort of mutex for managing resources, where every thread who takes the semaphore also has to release it. But semaphores do not have to be taken and released from the same thread (they have no "owner" like in mutexes/locks).

[antoinealb]

I agree that this may be an issue. Perhaps the best solution is to add a parameter for the maximum semaphore value ? Although this may be a problem when transitioning to the real implementation since uc/OS-III doesn't check for a maximum semaphore count (well, they are just checked for overflow, but we can leave that out).

Another option that I see would be to add a specific function to configure the mock semaphore behaviour to check or not to check if we signal'd a semaphore more often than we took it, because it could be a problem if we release a semaphore twice, for example for your free space-counting semaphore. This function could be removed (or be a no-op) when using the real implementation.

[froj]

In the example you described, you are using semaphores as a sort of mutex for managing resources

I agree and I can't think of a case where a semaphore like this doesn't have max_count = 1, i.e. is a mutex.

IMO we shouldn't care about max_count.

[antoinealb]

Not caring about max_count in tests, or at least, not having the option to care is dangerous : In the example of the buffer with two semaphores, releasing the semaphore multiple times without taking it could result in memory corruption or other kind of bugs that could be prevented using this sort of validation.

But I also agree that the current implementation prevents a lot of useful operations on semaphores, which is also problematic.

[froj]

releasing the semaphore multiple times without taking it

That's exactly what you do in the example of the buffer with two semaphores. How many times you are allowed to post the semaphore isn't really a property of the semaphore itself but rather something like BUFFER_SIZE.

[msplr]

Shouldn't the semaphore counter be checked in the application code's tests and not in the semaphore code?

[froj]

Shouldn't the semaphore counter be checked in the application code's tests and not in the semaphore code?

That's what I was trying to say. :)

[pierluca]

oooh right, consumer/producer pattern, sorry, good point.

Shouldn't the semaphore counter be checked in the application code's tests and not in the semaphore code?

It is easy to forget it in application test code. The only limit I see with the current code is the max_count. We could, however, initialize it separately : semaphore_t *os_semaphore_create(unsigned int count, unsigned int maxCount)

For consumer/producer patterns, maxCount = size of buffer.

[antoinealb]

The problem I see with adding it to the os_semaphore_create is that maximum semaphore value is not enforced by uc/OS-III and someone might guess it is if we put it in the production code. Perhaps adding it as os_semaphore_mock_set_maxcount would indicate to people that this feature only works on test code ?

[31415us]

why would you want to enforce a maxcount in a semaphore? i mean i get that for testing purposes you'd maybe want to check that a certain threshold is never passed but such stuff belongs strictly in the testing code!

[antoinealb]

That's why I suggest putting it in the test double only.

[pierluca]

• if we're doing this platform abstraction layer, we could enforce it ourselves on uc/OS-III as part of this layer (not as asserts, but definitely logging)
• check common semaphore APIs , many of them have a "max count" parameter. it's a reasonable safeguard against use of resources you know can't be there.
• if you want to explicitly bypass it, you can always set it to UINT32_MAX

on a separate note, why is nobody using unsigned for unsigned values and the stdint.h types? (uint32_t, int16_t, uint64_t , ... ) don't we have C99 support? it would be useful to guarantee that the code compiles in a consistent way across 16, 32 and 64 bit platforms...

On Thu, Jun 12, 2014 at 10:46 PM, Antoine Albertelli < notifications@github.com> wrote:

The problem I see with adding it to the os_semaphore_create is that maximum semaphore value is not enforced by uc/OS-III and someone might guess it is if we put it in the production code. Perhaps adding it as os_semaphore_mock_set_maxcount would indicate to people that this feature only works on test code ?

— Reply to this email directly or view it on GitHub https://github.com/cvra/platform-abstraction/pull/3#issuecomment-45945801 .

[froj]

on a separate note, why is nobody using unsigned for unsigned values and the stdint.h types? (uint32_t, int16_t, uint64_t , ... ) don't we have C99 support?

Opening a new issue to discuss that and locking this thrad.