PropGCC Native Locks Problems
SRLM
Posts: 5,045
I've noticed some odd behavior with the locks, and I think it should either be changed or documented.
1. lockret does not clear a lock.
Argument for change: The intuitive use of a lock is that any time you successfully check out a lock, it is "fresh". The default behavior, though, breaks this understanding. For example:
Cog A:
This behavior leads to more tightly coupled code, since your "fresh" lock depends on every usage of that particular lock before. I think it should be improved by making the lockret clear the lock before returning it.
Arguments against, and my responses:
1. (It's a performance hit to release the lock on lockret) Since lockret should be called infrequently, I don't think it will make a difference.
2. (The change deviates from standard Propeller convention) Yes, but I think it fixes a flaw in the original implementation. And, it wouldn't break current conventions. If code used lockclr before the modified lockret it would just duplicate the clear action, without breaking any programs.
2. lockset does not return true.
Argument for change: lockset does not return true, which is unintuitive (and contradicts all documentation sources).
This
Output:
As you can see, a "true" lock is not equal to true, but instead 0xFFFFFFFF. I think that this should be changed so that lockset returns C "true" or C "false".
Arguments against, and my responses:
1. (It's a performance hit to convert the output to "true" or "false") Yes, it may result in an extra instruction or two, but I think that it is worth it to make the code behave appropriately. If performance is that much of an issue, the user could use the lock instructions directly from assembly.
1. lockret does not clear a lock.
Argument for change: The intuitive use of a lock is that any time you successfully check out a lock, it is "fresh". The default behavior, though, breaks this understanding. For example:
Cog A:
int lockidA = locknew(); lockset(lockidA); // == false //Do stuffCog B:
[code]//do stuff for a while int lockidB = locknew(); //Happens to be the same as lockidA lockset(lockidB); // == true
This behavior leads to more tightly coupled code, since your "fresh" lock depends on every usage of that particular lock before. I think it should be improved by making the lockret clear the lock before returning it.
Arguments against, and my responses:
1. (It's a performance hit to release the lock on lockret) Since lockret should be called infrequently, I don't think it will make a difference.
2. (The change deviates from standard Propeller convention) Yes, but I think it fixes a flaw in the original implementation. And, it wouldn't break current conventions. If code used lockclr before the modified lockret it would just duplicate the clear action, without breaking any programs.
2. lockset does not return true.
Argument for change: lockset does not return true, which is unintuitive (and contradicts all documentation sources).
This
int lock = locknew();
if(lockset(lock) == false){
printf("Lock was false!\n");
}
if(lockset(lock) == true){
printf("Lock was true!\n");
}
if(lockset(lock) != false){
printf("Lock was not false!\n");
}
printf("lockset 'true' value == 0x%X\n", lockset(lock));
lockclr(lock);
lockret(lock);
Output:
Lock was false! Lock was not false! lockset 'true' value == 0xFFFFFFFF
As you can see, a "true" lock is not equal to true, but instead 0xFFFFFFFF. I think that this should be changed so that lockset returns C "true" or C "false".
Arguments against, and my responses:
1. (It's a performance hit to convert the output to "true" or "false") Yes, it may result in an extra instruction or two, but I think that it is worth it to make the code behave appropriately. If performance is that much of an issue, the user could use the lock instructions directly from assembly.
Comments
I disagree that it makes sense. To me, the advantage of GCC is that you don't have to know how the hardware works, or what makes sense at the ASM level. GCC should abstract that all away.
In regards to the independence of lockset/lockclr and locknew/lockret, I still think that returning a lock should clear it. It's the more intuitive way, and it follows with the standard pedagogy of concurrent programming.
If it looks, like a duck, walks like a duck,....
Following the "principle of least surprise" and give the is a Propeller we are working on then my opinion is that anything called lockset, lockclr etc should behave in the same way as the PASM counterparts.
Anything with different semantics should have a different name so as to make it clear.
Anyway, it's probably better if the lock owner issues the lockret, which implies that the lock be set upon return. Otherwise, another cog may obtain the lock between the lockclr and lockret.
-Phil
As for the returned state, that's just the way it is. If you want a defined state after fetching a new ID make sure it is what you expect/want.
If you want Propeller locks in PropGCC, the intrinsics should work just like the PASM. If you want C compatible locks, you should use the mutex locks in pthreads.
Of course, the option that we always have, if you want things to work the way you think they should be, then do your own thing!
Exactly: I think lockset should return C "true" (1) or C "false", not Spin "true"(-1) and Spin "false"(0).
The difference being that the next time that particular lock is checked out (perhaps by some unrelated cog or object), it is already set (which could lead to a deadlock as it waits for it to be unset).
The difference that I'm arguing for is that even the Propeller locks should have C semantics.
"true" the constant is already 1. The difference being that your statement is really 'if(0x55aa != 0) { dosomething(); } ', and I'm saying that it should be possible to say 'if(lockset(num) == true) { dosomething; }'. At this point, that statement would never evaluate to true, which I think breaks the implied semantic contract that C programmers have.
Re: make my own:
Yes, I could. But I wanted to bring this point up, because I think it's a problem with the PropGCC implementation. I've taken the "make my own" route for some things that I don't agree with (ie. serial drivers), but I think that this issue falls into a different category.
In any case, here is what needs to be changed:
-Phil
That is "conceptual true" with regard to conditionals like if, while, for, etc.... Some languages will not let you use anything other than true or false or a comparison that results in true or false in if, while, for, etc.... C/C++ do not care. Whether they should or not is a pedantic debate.
Many if not most programmers don't care what the actual value is of the bool symbol "true" because it is whatever the C++ people defined, and one should never expect to compare the true symbol (or false for that matter) to a number. A C++ bool is "true" or "false" and nothing else. Straight C does not have a bool type.
Ignoring a few exceptions, yes, propeller.h is a direct copy of propeller-specific functions. But the manual is "wrong" in a few cases in regards to C/C++, since "TRUE" in the manual is 0xFFFFFFFF (v1.2, page 93) and lockclr() doesn't return anything as it "should" (v1.2, page 120).
In a case like this there would be deadlock:
Psuedocode:
Cog A: global int lockID = locknew(); ... while(lockset(lockID) != false){} //loop while the lock is checked out. Cog B: while(lockID < 0 || lockID > 7){} //loop until a lock is taken from the pool. while(lockset(lockID) != false){} //loop while the lock is checked out.In this case, we can't use lockclr() right after the locknew() in CogA, because CogB may have checked out that lock already.
I realize that there are other solutions to this deadlock problem, but what I'm trying to convey is that the expected behavior of a lock is to be ready to use when you get it (IMHO).
I'm making the argument so that we can use statements like 'while(lockset(lockID) == true){}'. It doesn't matter what the particular value of true is, as long as it compare. I don't think we are comparing to a number: I think that lockset(lockID) returns semantic T or F value.
Yes, I have access to both the repository and the sites pages (I've taken over maintenance of the FAQ). I'll update the p2test comments, regenerate the HTML, and see if I can cut and paste just the locks portion.
That is fine as long as you don't use ANSI C89 which doesn't provide a true or false symbol right?
I guess a #ifdef could handle that though.
-Phil
It's a little more subtle than that:
In C, a Boolean test such as will evaluate to 1 or 0. However, when you perform a Boolean test (e.g. , the code only tests whether the values "a" and "b" are both nonzero: if one of them is zero, the expression evaluates to 0, if both of them are nonzero (regardless of their actual values), the expression evaluates to 1.
I would say a zero/nonzero return value from a function that has a Boolean result is acceptable in C as well as C++ because if you do a simple test like it will behave as intended. If you have such a value (i.e. a value that can be zero or nonzero) and you want to make it into a value that's either 0 or 1, you can always prefix it with two exclamation points:
My opinion is that PropGCC already does the minimum that's necessary to get a value that can be tested for zero or nonzero, which is perfectly fine for C as well as C++ semantics.
I see comparison against TRUE as bad practice, at least in C. No doubt, various static analysis tools such as Lint or QAC will catch comparison against TRUE as a potential bug. When comparing a boolean value (stored in an int or unsigned int, in C), you should always just compare "the Boolean way":
if (function( )) /* OK */ { ... } if (!function( )) /* OK */ { ... } if (function( ) == FALSE) /* Acceptable, but I personally don't like this style */ { ... } if (function( ) == TRUE) /* Bad practice: could be a potential bug */ { ... }The correct code would be
while(lockset(lockID)) { }.===Jac
(nothing personal of course, SRLM).
I agree that is one correct code. I personally prefer to have all boolean type expressions explicitly compared to true or false: I think it is more clear.
And maybe that's my problem here: I'm coming at this from a C++ perspective, and not the C perspective.
No offense taken. I just noticed a quirk, and decided to propose a change. It looks like, out of 7 people who responded, 7 like it the way it is. So, I'll pipe down
All done. As a side note, it would be great if PropGCC had a place where the actual .html files could be uploaded, so that two separate copies don't have to be maintained. Is there such a place?
There are no built-in 'true' or 'false' keywords in C though, so maybe you're thinking of C++? I don't know if there are 'true' and 'false' keywords in C++. In any case (in C or C++) you're never supposed to compare something to true, or TRUE as you often see in C (it's common to create a #define for that). You only _assign_ to TRUE, you never compare to it. You only compare to FALSE. So 'if (A == FALSE)' is OK (as is 'if (!A)', but 'if (A == TRUE)' is never OK, you can only do 'if (A)'. A=TRUE; is what it's for.
So in C it's equally common to see as is or, even, #define TRUE -1
Thank you!
I've struggled with .html and google sites too. I'd really like to see a "normal" web site.
That was what I was always taught as well. You test if the value is not 0 for a TRUE/FALSE situation. Since TRUE means all kinds of different things in different systems testing that something equals TRUE invites issues. So returning 0xFFFFFFFF is not TRUE but it is also not FALSE and if the test is written with this rule of thumb in mind then it evaluates correctly. I do think updating the documentation is a fine idea so that is clear.
That was what I was always taught as well. You test if the value is not 0 for a TRUE/FALSE situation. Since TRUE means all kinds of different things in different systems testing that something equals TRUE invites issues. So returning 0xFFFFFFFF is not TRUE but it is also not FALSE and if the test is written with this rule of thumb in mind then it evaluates correctly. I do think updating the documentation is a fine idea so that is clear.