Hi, I was looking the kernel source code and there are a lot of places in which either "(expression) ? 1 : 0" or "(expression) ? 0 : 1" appear. As fair as I can tell both can be replaced by "!!expression" and "!expression". Moreover there it seems that using "!!" does not add a "nopl" instruction at the end of the call. Does anybody knows why? Anyway. Wouldn't be nice if kernel provides something like "boolean(x)" macro and "inv_boolean(x)" to do this operations? #ifdef MOD_IF int mod_if(int x) { return (x == 0) ? 0 : 1; } #endif #ifdef MOD_X int mod_x(int x) { return !!x; } #endif 0000000000000000 <mod_if>: 0: 31 c0 xor %eax,%eax 2: 85 ff test %edi,%edi 4: 0f 95 c0 setne %al 7: c3 retq 8: 0f 1f 84 00 00 00 00 nopl 0x0(%rax,%rax,1) f: 00 0000000000000010 <mod_x>: 10: 31 c0 xor %eax,%eax 12: 85 ff test %edi,%edi 14: 0f 95 c0 setne %al 17: c3 retq Regards, Vinícius -- Simplicity is the ultimate sophistication
On Tue, Dec 30, 2014 at 3:25 AM, Vinícius Tinti <viniciustinti@gmail.com> wrote:
I was looking the kernel source code and there are a lot of places in which either "(expression) ? 1 : 0" or "(expression) ? 0 : 1" appear. As fair as I can tell both can be replaced by "!!expression" and "!expression".
Moreover there it seems that using "!!" does not add a "nopl" instruction at the end of the call. Does anybody knows why?
It seems that the nop instruction is inserted for alignment, and if you reverse the order of functions in your c source, nop will still be inserted between them.
0000000000000000 <mod_if>: 0: 31 c0 xor %eax,%eax 2: 85 ff test %edi,%edi 4: 0f 95 c0 setne %al 7: c3 retq 8: 0f 1f 84 00 00 00 00 nopl 0x0(%rax,%rax,1) f: 00
0000000000000010 <mod_x>: 10: 31 c0 xor %eax,%eax 12: 85 ff test %edi,%edi 14: 0f 95 c0 setne %al 17: c3 retq
-- Thanks. -- Max
On Mon, Dec 29, 2014 at 10:34 PM, Max Filippov <jcmvbkbc@gmail.com> wrote:
On Tue, Dec 30, 2014 at 3:25 AM, Vinícius Tinti <viniciustinti@gmail.com> wrote:
I was looking the kernel source code and there are a lot of places in which either "(expression) ? 1 : 0" or "(expression) ? 0 : 1" appear. As fair as I can tell both can be replaced by "!!expression" and "!expression".
Moreover there it seems that using "!!" does not add a "nopl" instruction at the end of the call. Does anybody knows why?
It seems that the nop instruction is inserted for alignment, and if you reverse the order of functions in your c source, nop will still be inserted between them.
I notice that too. If you use both functions in other code they both will have the nopl. And "!expression" is as good as the "(expression) ? 0 : 1". Thus no gain and worse readability I think.
0000000000000000 <mod_if>: 0: 31 c0 xor %eax,%eax 2: 85 ff test %edi,%edi 4: 0f 95 c0 setne %al 7: c3 retq 8: 0f 1f 84 00 00 00 00 nopl 0x0(%rax,%rax,1) f: 00
0000000000000010 <mod_x>: 10: 31 c0 xor %eax,%eax 12: 85 ff test %edi,%edi 14: 0f 95 c0 setne %al 17: c3 retq
-- Thanks. -- Max
-- Simplicity is the ultimate sophistication
On Mon, 29 Dec 2014 22:25:41 -0200, Vinícius Tinti said:
I was looking the kernel source code and there are a lot of places in which either "(expression) ? 1 : 0" or "(expression) ? 0 : 1" appear. As fair as I can tell both can be replaced by "!!expression" and "!expression".
As far as the compiler goes, they're the same thing, as you already discovered. Some are just code written by not-so-experts. In other cases, the author may have wanted to keep clear that we were calculating an integer to be used for further arithmetic rather than a boolean. Patches to clean it up wouldn't be a bad idea in some cases. However, each location will have to be examined for what was intended and what produces more readable code. If you're *really* ambitious, converting stuff to use a boolean rather than an int where appropriate would be nice - we don't do that enough currently. But that will require actually reading and understanding the code. That's not a good task for those who like to submit patches without thinking....
On Mon, Dec 29, 2014 at 11:00 PM, <Valdis.Kletnieks@vt.edu> wrote:
On Mon, 29 Dec 2014 22:25:41 -0200, Vinícius Tinti said:
I was looking the kernel source code and there are a lot of places in which either "(expression) ? 1 : 0" or "(expression) ? 0 : 1" appear. As fair as I can tell both can be replaced by "!!expression" and "!expression".
As far as the compiler goes, they're the same thing, as you already discovered.
Some are just code written by not-so-experts.
In other cases, the author may have wanted to keep clear that we were calculating an integer to be used for further arithmetic rather than a boolean.
Patches to clean it up wouldn't be a bad idea in some cases. However, each location will have to be examined for what was intended and what produces more readable code. If you're *really* ambitious, converting stuff to use a boolean rather than an int where appropriate would be nice - we don't do that enough currently. But that will require actually reading and understanding the code. That's not a good task for those who like to submit patches without thinking....
In fact, to be sure that I was not messing around I was thinking in write a Clang plugin for doing so. With this I can be 100% sure that I am not introducing any errors. But anyway if there is no benefits on doing so there is no need to do. -- Simplicity is the ultimate sophistication
On Mon, Dec 29, 2014 at 08:00:08PM -0500, Valdis.Kletnieks@vt.edu wrote:
If you're *really* ambitious, converting stuff to use a boolean rather than an int where appropriate would be nice - we don't do that enough currently. But that will require actually reading and understanding the code.
It should not be assumed that true will always be 1 as defined in include/linux/stddef.h, right?
On Sat, 03 Jan 2015 18:54:00 -0500, John de la Garza said:
It should not be assumed that true will always be 1 as defined in include/linux/stddef.h, right?
No, I mean use an actual 'bool' type rather than 'int'. Consider this from kernel/softirq.c: static inline bool lockdep_softirq_start(void) { bool in_hardirq = false; if (trace_hardirq_context(current)) { in_hardirq = true; trace_hardirq_exit(); } lockdep_softirq_enter(); return in_hardirq; } However, this will require actual code analysis to make sure that it's *really* being used as a boolean, not an int. In particular, assignments to/from the variable need to be audited for casting issues (and possibly the correct rework is to convert *several* variables to bool at once).
On Sat, Jan 03, 2015 at 11:20:29PM -0500, Valdis.Kletnieks@vt.edu wrote:
On Sat, 03 Jan 2015 18:54:00 -0500, John de la Garza said:
It should not be assumed that true will always be 1 as defined in include/linux/stddef.h, right?
No, I mean use an actual 'bool' type rather than 'int'. Consider this from kernel/softirq.c:
yes, bool has two possible values true and false from include/linux/stddef.h: enum { false = 0, true = 1 }; I assume it is a bad idea to depend on true being 1, right? I mean, I should assume that true could be changed to any non 0 value in the future, right?
On Sun, Jan 04, 2015 at 06:43:22PM -0500, John de la Garza wrote:
On Sat, Jan 03, 2015 at 11:20:29PM -0500, Valdis.Kletnieks@vt.edu wrote:
On Sat, 03 Jan 2015 18:54:00 -0500, John de la Garza said:
It should not be assumed that true will always be 1 as defined in include/linux/stddef.h, right?
No, I mean use an actual 'bool' type rather than 'int'. Consider this from kernel/softirq.c:
yes, bool has two possible values true and false
from include/linux/stddef.h: enum { false = 0, true = 1 };
I assume it is a bad idea to depend on true being 1, right? I mean, I should assume that true could be changed to any non 0 value in the future, right?
Why would that matter? Just always test for "true" and "false" and you will be fine. greg k-h
On Sun, Jan 04, 2015 at 04:50:58PM -0800, Greg KH wrote:
On Sun, Jan 04, 2015 at 06:43:22PM -0500, John de la Garza wrote:
I assume it is a bad idea to depend on true being 1, right? I mean, I should assume that true could be changed to any non 0 value in the future, right?
Why would that matter? Just always test for "true" and "false" and you will be fine.
What if you need to store the value in a bitmap that needs to be 1 or 0 so they can be shifted/anded in?
On Wed, Jan 07, 2015 at 08:46:52PM -0800, John de la Garza wrote:
On Sun, Jan 04, 2015 at 04:50:58PM -0800, Greg KH wrote:
On Sun, Jan 04, 2015 at 06:43:22PM -0500, John de la Garza wrote:
I assume it is a bad idea to depend on true being 1, right? I mean, I should assume that true could be changed to any non 0 value in the future, right?
Why would that matter? Just always test for "true" and "false" and you will be fine.
What if you need to store the value in a bitmap that needs to be 1 or 0 so they can be shifted/anded in?
Then you don't use a boolean if you are worried about it, come on, why is this even a discussion?
On Sun, 04 Jan 2015 18:43:22 -0500, John de la Garza said:
On Sat, Jan 03, 2015 at 11:20:29PM -0500, Valdis.Kletnieks@vt.edu wrote:
On Sat, 03 Jan 2015 18:54:00 -0500, John de la Garza said:
It should not be assumed that true will always be 1 as defined in include/linux/stddef.h, right?
No, I mean use an actual 'bool' type rather than 'int'. Consider this from kernel/softirq.c:
yes, bool has two possible values true and false
from include/linux/stddef.h: enum { false = 0, true = 1 };
Note that's an *anonynous* enum, which defines the two values, but it *doesn't* define an enum type that can be used to force type safety. No, if you're converting a variable from int to bool, the *important* line is from include/linux/types.h: typedef _Bool bool; which ensures more type safety than the enum does.
On Sun, Jan 04, 2015 at 08:17:15PM -0500, Valdis.Kletnieks@vt.edu wrote:
On Sun, 04 Jan 2015 18:43:22 -0500, John de la Garza said:
On Sat, Jan 03, 2015 at 11:20:29PM -0500, Valdis.Kletnieks@vt.edu wrote:
On Sat, 03 Jan 2015 18:54:00 -0500, John de la Garza said:
It should not be assumed that true will always be 1 as defined in include/linux/stddef.h, right?
No, I mean use an actual 'bool' type rather than 'int'. Consider this from kernel/softirq.c:
yes, bool has two possible values true and false
from include/linux/stddef.h: enum { false = 0, true = 1 };
Note that's an *anonynous* enum, which defines the two values, but it *doesn't* define an enum type that can be used to force type safety.
No, if you're converting a variable from int to bool, the *important* line is from include/linux/types.h:
typedef _Bool bool;
which ensures more type safety than the enum does.
right, I see that now so _Bool is a defined by the compiler and typedefed to bool
participants (5)
-
Greg KH -
John de la Garza -
Max Filippov -
Valdis.Kletnieks＠vt.edu -
Vinícius Tinti