Add cross-platform supoprt for atomic_ops using compare-and-set.

[rgrover]

CR please. @bogdanm @0xc0170 @bremoran @autopulated

[bogdanm]

+1

[hugovincent]

Sorry, not happy with this yet, comments incoming. (Update: comments passed on verbally).

[rgrover]

CR @hugovincent, @bogdanm, @0xc0170, @autopulated, @bremoran

[hugovincent]

Thanks @rgrover looks much better ;-)

[bremoran]

Does M7 use the same LDREX/STREX instructions as M3/4?

[bogdanm]

LGTM, +1

[bremoran]

Please do not merge this yet

[0xc0170]

@bremoran Should be the same, http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0646a/BABFFBJB.html . Can you add more info why not to merge?

[autopulated]

Looks sane to me. Passing expectedCurrentValue by reference alongside a pointer argument feels a bit weird (if you don't think about how the function works internally, you'd wonder: why aren't these both pointers, or both references?), but I guess it's a sensible thing to do here.

[bremoran]

Comments coming, sorry, I was writing test cases.

[bremoran]

-1 The list of unsupported types is massive. At the very least, you should use a templatized version of atomic_cas with indirection via sizeof to pick the correct implementation.

[bogdanm]

@bremoran , that could be a further improvement. If you think the code is correct, I'd merge it in its current format to get things going.

[bremoran]

It's not correct, see my comment above. References should be const unless they're being modified.

[bogdanm]

They are being modified. See @autopulated's comment above.

[bremoran]

I think we have the wrong signature in that case.

expectedCurrentValue is an inout parameter, which I would avoid if at all possible.

atomic_cas(T ptr, T expectedValue, T currentValue, T desiredValue);

This way outputs are clearly identified by being pointers and there is no spooky change of expected values.

[bogdanm]

I'm not sure I follow. References can also be in-out, what's wrong with that? Or is it mainly a matter of style?

[bremoran]

It's style. Inout parameters create heisenbugs.

[rgrover]

In the failure case of atomic_cas, you'd always want to re-read the currentValue. Making this implicit makes code read neater.

[bremoran]

But it overwrites the expected value at the same time. This could hide a bug very easily. Suppose I expect 0xA5 to show up. I write code like this:

uint8_t expected = 0xA5;
do {
something...
} while (!atomic_cas(ptr, expected, expected+1));

My expected value is overwritten on the first pass, then an unexpected value is written instead. Even the parameter definitions hide this behaviour:

/*
 * @param  expectedCurrentValue The expected current value of the data at the location.
 *                              This computed 'desiredValue' should be a function of this current value.
 */

My suggestion does not inhibit concise code, but it makes the behaviour of the function much clearer:

/*
 * function incr(p : pointer to int, a : int) returns int {
 *     done = false
 *     value = *p  // This fetch operation need not be atomic.
 *     while not done {
 *         done = atomic_cas(p, value, &value, value + a) // value gets updated automatically until success
 *     }
 *     return value + a
 * }
 */

[rgrover]

@bremoran: if we don't have the automatic update behaviour, your example above would have read:

uint8_t expected = 0xA5;
do {
expected = *ptr;
something...
} while (!atomic_cas(ptr, expected, expected+1));

In the common case of a successful atomic_cas, it fetches 'expected' redundantly in order to be correct.

Passing a duplicate value argument to atomic_cas(..., value, &value, ...) is also error-prone because of the duplication.

[hugovincent]

Does M7 use the same LDREX/STREX instructions as M3/4?

(It does). Maybe we should use ARMv7-M and v6-M macros for this sort of thing. What macros exist that would work?

[bremoran]

@rgrover, I dislike inout parameters, particularly in references. They obscure the behaviour of the function. If you are unwilling to separate the currentExpectedValue into two parameters, please update the parameter definition to explain clearly that the expected value is replaced with the current value on failure only. I realise that this is documented later in the comment block, but it should be in the parameter definition.

Placing a reference next to a pointer is questionable style, not because there is anything technically wrong with the approach, but because it is mixing two styles. It is very jarring to the programmer. In addition to this, using a reference for an output parameter conceals that the value is passed by reference, which creates the false impression in sample or example code that it will not be modified. I would suggest using a pointer in both places.

I simply want the behaviour of these functions to be explicit in their signatures and documentation. I do not feel that this is currently the case.

[hugovincent]

The list of unsupported types is massive. At the very least, you should use a templatized version of atomic_cas with indirection via sizeof to pick the correct implementation.

No. The hardware only supports atomicity for up to words (32-bits), half-words and bytes. Requiring the user to explicitly cast to one of supported types is a good thing. It's a low level primitive, users need to know roughly what they're doing and how it works, and permitting arbitrary types would cloud that.

As for providing the same templatized API on v7-M and v6-M, you could use this specialisation pattern (top answer here) to specialise by sizeof.

[bremoran]

@hugovincent, Ack. I'm just trying to solve the C++ problem of unsigned char != uint8_t despite the fact that uint8_t is:

typedef unsigned char uint8_t;

This seems like unecessary complexity for developers.

[rgrover]

please review: @autopulated @bremoran @bogdanm @hugovincent

[rgrover]

please review: @autopulated @bremoran @bogdanm @hugovincent this is it. I'm not going to make any other improvements.

[bogdanm]

OK, I understand @rgrover's frustration, I feel like we're trying to make this way too perfect than it needs to be on a first try. We'll improve it as needed in time. +1 from me.

[bremoran]

+1 If volatile warnings show up in builds, we can fix them then.

[0xc0170]

That's the team spirit :tea: +1

[bogdanm]

Are you actually building team spirit with tea ?! Dude. Not cool.

[autopulated]

OK... but are you aware that if you use these primitives with, e.g. the signed (instead of unsigned) versions of these types then it will not choose those template specialisations?

runs and hides

[rgrover]

@autopulated :)