* [PATCH] security: Use || instead of | for boolean expressions @ 2016-06-01 6:03 Rui Teng 2016-06-02 14:13 ` Serge E. Hallyn 0 siblings, 1 reply; 4+ messages in thread From: Rui Teng @ 2016-06-01 6:03 UTC (permalink / raw) To: serge.hallyn, james.l.morris, serge Cc: linux-security-module, linux-kernel, Rui Teng Sparse spits out the following warning: security/commoncap.c:989:41: warning: dubious: !x | y Bitwise and logical are equivalent here, but logical was intended. Replacing the bit-wise '|' with the boolean '||' silences the sparse warning. The generated code for both cases is the same. Signed-off-by: Rui Teng <rui.teng@linux.vnet.ibm.com> --- security/commoncap.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/security/commoncap.c b/security/commoncap.c index e7fadde..8f6fb24 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -976,7 +976,7 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3, case PR_CAP_AMBIENT: if (arg2 == PR_CAP_AMBIENT_CLEAR_ALL) { - if (arg3 | arg4 | arg5) + if (arg3 || arg4 || arg5) return -EINVAL; new = prepare_creds(); @@ -986,7 +986,7 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3, return commit_creds(new); } - if (((!cap_valid(arg3)) | arg4 | arg5)) + if (((!cap_valid(arg3)) || arg4 || arg5)) return -EINVAL; if (arg2 == PR_CAP_AMBIENT_IS_SET) { -- 2.7.4 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] security: Use || instead of | for boolean expressions 2016-06-01 6:03 [PATCH] security: Use || instead of | for boolean expressions Rui Teng @ 2016-06-02 14:13 ` Serge E. Hallyn 2016-06-02 16:53 ` Rui Teng 0 siblings, 1 reply; 4+ messages in thread From: Serge E. Hallyn @ 2016-06-02 14:13 UTC (permalink / raw) To: Rui Teng Cc: serge.hallyn, james.l.morris, serge, linux-security-module, linux-kernel, Andy Lutomirski On Wed, Jun 01, 2016 at 02:03:02PM +0800, Rui Teng wrote: > Sparse spits out the following warning: > security/commoncap.c:989:41: warning: dubious: !x | y > > Bitwise and logical are equivalent here, but logical was intended. > Replacing the bit-wise '|' with the boolean '||' silences the sparse warning. Hi, this looks ok, but I'm worried by > The generated code for both cases is the same. That cannot be. The logical result should be the same, but the generated code cannot be. I'm cc:ing Andy as this code came in with his patch. Is there an actual reason for having used bitwise here? thanks, -serge > Signed-off-by: Rui Teng <rui.teng@linux.vnet.ibm.com> > --- > security/commoncap.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/security/commoncap.c b/security/commoncap.c > index e7fadde..8f6fb24 100644 > --- a/security/commoncap.c > +++ b/security/commoncap.c > @@ -976,7 +976,7 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3, > > case PR_CAP_AMBIENT: > if (arg2 == PR_CAP_AMBIENT_CLEAR_ALL) { > - if (arg3 | arg4 | arg5) > + if (arg3 || arg4 || arg5) > return -EINVAL; > > new = prepare_creds(); > @@ -986,7 +986,7 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3, > return commit_creds(new); > } > > - if (((!cap_valid(arg3)) | arg4 | arg5)) > + if (((!cap_valid(arg3)) || arg4 || arg5)) > return -EINVAL; > > if (arg2 == PR_CAP_AMBIENT_IS_SET) { > -- > 2.7.4 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] security: Use || instead of | for boolean expressions 2016-06-02 14:13 ` Serge E. Hallyn @ 2016-06-02 16:53 ` Rui Teng 2016-06-02 17:37 ` Al Viro 0 siblings, 1 reply; 4+ messages in thread From: Rui Teng @ 2016-06-02 16:53 UTC (permalink / raw) To: Serge E. Hallyn Cc: serge.hallyn, james.l.morris, linux-security-module, linux-kernel, Andy Lutomirski On 6/2/16 10:13 PM, Serge E. Hallyn wrote: > On Wed, Jun 01, 2016 at 02:03:02PM +0800, Rui Teng wrote: >> Sparse spits out the following warning: >> security/commoncap.c:989:41: warning: dubious: !x | y >> >> Bitwise and logical are equivalent here, but logical was intended. >> Replacing the bit-wise '|' with the boolean '||' silences the sparse warning. > > Hi, > > this looks ok, but I'm worried by > >> The generated code for both cases is the same. > > That cannot be. The logical result should be the same, but the > generated code cannot be. Thanks for cc:ing the author. I tried to write a sample code to verify it before. Both || and | will generate the same assembly code. For example, compiling following code with "gcc -O2 -S main.c", and replacing || with | can generate the same assembly code. - main.c ------------ int parse(int a, int b, int c) { if (a || b || c) return 1; else return 0; } Of cause, it is only a sample on x86, but even if the generated code is not the same, the logical will be better than bitwise. Because (a || b || c) means (a != 0 || b != 0 || c != 0), once a != 0, the whole expression will be true(short-circuit evaluation). and (a | b | c) means calculate the bitwise first and check the result in the end. And since the args are all integer, there is no need to avoid any short-circuit. > > I'm cc:ing Andy as this code came in with his patch. Is there an > actual reason for having used bitwise here? > > thanks, > -serge > >> Signed-off-by: Rui Teng <rui.teng@linux.vnet.ibm.com> >> --- >> security/commoncap.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/security/commoncap.c b/security/commoncap.c >> index e7fadde..8f6fb24 100644 >> --- a/security/commoncap.c >> +++ b/security/commoncap.c >> @@ -976,7 +976,7 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3, >> >> case PR_CAP_AMBIENT: >> if (arg2 == PR_CAP_AMBIENT_CLEAR_ALL) { >> - if (arg3 | arg4 | arg5) >> + if (arg3 || arg4 || arg5) >> return -EINVAL; >> >> new = prepare_creds(); >> @@ -986,7 +986,7 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3, >> return commit_creds(new); >> } >> >> - if (((!cap_valid(arg3)) | arg4 | arg5)) >> + if (((!cap_valid(arg3)) || arg4 || arg5)) >> return -EINVAL; >> >> if (arg2 == PR_CAP_AMBIENT_IS_SET) { >> -- >> 2.7.4 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] security: Use || instead of | for boolean expressions 2016-06-02 16:53 ` Rui Teng @ 2016-06-02 17:37 ` Al Viro 0 siblings, 0 replies; 4+ messages in thread From: Al Viro @ 2016-06-02 17:37 UTC (permalink / raw) To: Rui Teng Cc: Serge E. Hallyn, serge.hallyn, james.l.morris, linux-security-module, linux-kernel, Andy Lutomirski On Fri, Jun 03, 2016 at 12:53:07AM +0800, Rui Teng wrote: > Of cause, it is only a sample on x86, but even if the generated code is > not the same, the logical will be better than bitwise. > Because (a || b || c) means (a != 0 || b != 0 || c != 0), once a != 0, > the whole expression will be true(short-circuit evaluation). > and (a | b | c) means calculate the bitwise first and check the result > in the end. And since the args are all integer, there is no need to > avoid any short-circuit. Not obvious at all. Comparison of the cost of two OR plus one conditional branch vs. that of "short-circuited" variant is almost certainly going to be in favour of compiler using bitwise operations anyway. At the very least you'll need to examine the first value, so even in the fastest case it's test + branch taken. The rest is going to be worse and the whole thing is going to be not fun for the pipeline either, not to mention higher icache footprint, etc. So I would be quite surprised if cc(1) would use short-circuit there, whichever form you use in the source. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-06-02 17:38 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-06-01 6:03 [PATCH] security: Use || instead of | for boolean expressions Rui Teng 2016-06-02 14:13 ` Serge E. Hallyn 2016-06-02 16:53 ` Rui Teng 2016-06-02 17:37 ` Al Viro
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).