kernel.h: Add for_each_if()
diff mbox

Message ID 20180709162509.29343-1-daniel.vetter@ffwll.ch
State New
Headers show

Commit Message

Daniel Vetter July 9, 2018, 4:25 p.m. UTC
To avoid compilers complainig about ambigious else blocks when putting
an if condition into a for_each macro one needs to invert the
condition and add a dummy else. We have a nice little convenience
macro for that in drm headers, let's move it out. Subsequent patches
will roll it out to other places.

The issue the compilers complain about are nested if with an else
block and no {} to disambiguate which if the else belongs to. The C
standard is clear, but in practice people forget:

if (foo)
	if (bar)
		/* something */
	else
		/* something else

The same can happen in a for_each macro when it also contains an if
condition at the end, except the compiler message is now really
confusing since there's only 1 if:

for_each_something()
	if (bar)
		/* something */
	else
		/* something else

The for_each_if() macro, by inverting the condition and adding an
else, avoids the compiler warning.

Motivated by a discussion with Andy and Yisheng, who want to add
another for_each_macro which would benefit from for_each_if() instead
of hand-rolling it.

Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: David Airlie <airlied@linux.ie>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: NeilBrown <neilb@suse.com>
Cc: Wei Wang <wvw@google.com>
Cc: Stefan Agner <stefan@agner.ch>
Cc: Andrei Vagin <avagin@openvz.org>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Yisheng Xie <ysxie@foxmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
--
v2: Explain a bit better what this is good for, after the discussion
with Peter Z.
---
 include/drm/drmP.h     | 3 ---
 include/linux/kernel.h | 3 +++
 2 files changed, 3 insertions(+), 3 deletions(-)

Comments

Andy Shevchenko July 9, 2018, 6:30 p.m. UTC | #1
On Mon, 2018-07-09 at 18:25 +0200, Daniel Vetter wrote:

> v2: Explain a bit better what this is good for, after the discussion
> with Peter Z.

Since I have not been Cc'ed to your discussion there is another
weirdness which this macro prohibits, i.e.

for_each_blah() {
} else {
 ...blahblah...
}
Andrew Morton July 9, 2018, 11:30 p.m. UTC | #2
On Mon,  9 Jul 2018 18:25:09 +0200 Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> To avoid compilers complainig about ambigious else blocks when putting
> an if condition into a for_each macro one needs to invert the
> condition and add a dummy else. We have a nice little convenience
> macro for that in drm headers, let's move it out. Subsequent patches
> will roll it out to other places.
> 
> The issue the compilers complain about are nested if with an else
> block and no {} to disambiguate which if the else belongs to. The C
> standard is clear, but in practice people forget:
> 
> if (foo)
> 	if (bar)
> 		/* something */
> 	else
> 		/* something else

um, yeah, don't do that.  Kernel coding style is very much to do

	if (foo) {
		if (bar)
			/* something */
		else
			/* something else
	}

And if not doing that generates a warning then, well, do that.

> The same can happen in a for_each macro when it also contains an if
> condition at the end, except the compiler message is now really
> confusing since there's only 1 if:
> 
> for_each_something()
> 	if (bar)
> 		/* something */
> 	else
> 		/* something else
> 
> The for_each_if() macro, by inverting the condition and adding an
> else, avoids the compiler warning.

Ditto.

> Motivated by a discussion with Andy and Yisheng, who want to add
> another for_each_macro which would benefit from for_each_if() instead
> of hand-rolling it.

Ditto.

> v2: Explain a bit better what this is good for, after the discussion
> with Peter Z.

Presumably the above was discussed in whatever-thread-that-was.
Daniel Vetter July 10, 2018, 7:53 a.m. UTC | #3
On Mon, Jul 09, 2018 at 04:30:01PM -0700, Andrew Morton wrote:
> On Mon,  9 Jul 2018 18:25:09 +0200 Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > To avoid compilers complainig about ambigious else blocks when putting
> > an if condition into a for_each macro one needs to invert the
> > condition and add a dummy else. We have a nice little convenience
> > macro for that in drm headers, let's move it out. Subsequent patches
> > will roll it out to other places.
> > 
> > The issue the compilers complain about are nested if with an else
> > block and no {} to disambiguate which if the else belongs to. The C
> > standard is clear, but in practice people forget:
> > 
> > if (foo)
> > 	if (bar)
> > 		/* something */
> > 	else
> > 		/* something else
> 
> um, yeah, don't do that.  Kernel coding style is very much to do
> 
> 	if (foo) {
> 		if (bar)
> 			/* something */
> 		else
> 			/* something else
> 	}
> 
> And if not doing that generates a warning then, well, do that.
> 
> > The same can happen in a for_each macro when it also contains an if
> > condition at the end, except the compiler message is now really
> > confusing since there's only 1 if:
> > 
> > for_each_something()
> > 	if (bar)
> > 		/* something */
> > 	else
> > 		/* something else
> > 
> > The for_each_if() macro, by inverting the condition and adding an
> > else, avoids the compiler warning.
> 
> Ditto.
> 
> > Motivated by a discussion with Andy and Yisheng, who want to add
> > another for_each_macro which would benefit from for_each_if() instead
> > of hand-rolling it.
> 
> Ditto.
> 
> > v2: Explain a bit better what this is good for, after the discussion
> > with Peter Z.
> 
> Presumably the above was discussed in whatever-thread-that-was.

So there's a bunch of open coded versions of this already in kernel
headers (at least the ones I've found). Not counting the big pile of
existing users in drm. They are all wrong and should be reverted to a
plain if? That why there's a bunch more patches in this series.

And yes I made it clear in the discussion that if you sprinkle enough {}
there's no warning, should have probably captured this here.

Aka a formal Nack-pls-keep-your-stuff-in-drm: would be appreciated so I
can stop bothering with this.

Thanks, Daniel
NeilBrown July 10, 2018, 10:32 a.m. UTC | #4
On Tue, Jul 10 2018, Daniel Vetter wrote:

> On Mon, Jul 09, 2018 at 04:30:01PM -0700, Andrew Morton wrote:
>> On Mon,  9 Jul 2018 18:25:09 +0200 Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> 
>> > To avoid compilers complainig about ambigious else blocks when putting
>> > an if condition into a for_each macro one needs to invert the
>> > condition and add a dummy else. We have a nice little convenience
>> > macro for that in drm headers, let's move it out. Subsequent patches
>> > will roll it out to other places.
>> > 
>> > The issue the compilers complain about are nested if with an else
>> > block and no {} to disambiguate which if the else belongs to. The C
>> > standard is clear, but in practice people forget:
>> > 
>> > if (foo)
>> > 	if (bar)
>> > 		/* something */
>> > 	else
>> > 		/* something else
>> 
>> um, yeah, don't do that.  Kernel coding style is very much to do
>> 
>> 	if (foo) {
>> 		if (bar)
>> 			/* something */
>> 		else
>> 			/* something else
>> 	}
>> 
>> And if not doing that generates a warning then, well, do that.
>> 
>> > The same can happen in a for_each macro when it also contains an if
>> > condition at the end, except the compiler message is now really
>> > confusing since there's only 1 if:
>> > 
>> > for_each_something()
>> > 	if (bar)
>> > 		/* something */
>> > 	else
>> > 		/* something else
>> > 
>> > The for_each_if() macro, by inverting the condition and adding an
>> > else, avoids the compiler warning.
>> 
>> Ditto.
>> 
>> > Motivated by a discussion with Andy and Yisheng, who want to add
>> > another for_each_macro which would benefit from for_each_if() instead
>> > of hand-rolling it.
>> 
>> Ditto.
>> 
>> > v2: Explain a bit better what this is good for, after the discussion
>> > with Peter Z.
>> 
>> Presumably the above was discussed in whatever-thread-that-was.
>
> So there's a bunch of open coded versions of this already in kernel
> headers (at least the ones I've found). Not counting the big pile of
> existing users in drm. They are all wrong and should be reverted to a
> plain if? That why there's a bunch more patches in this series.
>
> And yes I made it clear in the discussion that if you sprinkle enough {}
> there's no warning, should have probably captured this here.
>
> Aka a formal Nack-pls-keep-your-stuff-in-drm: would be appreciated so I
> can stop bothering with this.

I think is it problematic to have macros like

#define for_each_foo(...) for (......) if (....)

because
   for_each_foo(...)
      if (x) ....; else ......;

is handled badly.
So in that sense, your work seems like a good thing.

However it isn't clear to me that you need a new macro.
The above macro could simply be changed to

#define for_each_foo(...) for (......) if (!....);else

Clearly people don't always think to do this, but would adding a macro
help people to think?

If we were to have a macro, it isn't clear to me that for_each_if() is a
good name.
Every other macro I've seen that starts "for_each_" causes the body to
loop.  This one doesn't.  If someone doesn't know what for_each_if()
does and sees it in code, they are unlikely to jump to the right
conclusion.
I would suggest that "__if" would be a better choice.  I think most
people would guess that means "like 'if', but a bit different", which is
fairly accurate.

I think the only sure way to avoid bad macros being written is to teach
some static checker to warn about any macro with a dangling "if".
Possibly checkpatch.pl could do that (but I'm not volunteering).

I do agree that it would be good to do something, and if people find
for_each_fi() to actually reduce the number of poorly written macros,
then I don't object to it.

Thanks,
NeilBrown
Daniel Vetter July 11, 2018, 11:51 a.m. UTC | #5
On Tue, Jul 10, 2018 at 12:32 PM, NeilBrown <neilb@suse.com> wrote:
> On Tue, Jul 10 2018, Daniel Vetter wrote:
>
>> On Mon, Jul 09, 2018 at 04:30:01PM -0700, Andrew Morton wrote:
>>> On Mon,  9 Jul 2018 18:25:09 +0200 Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>>>
>>> > To avoid compilers complainig about ambigious else blocks when putting
>>> > an if condition into a for_each macro one needs to invert the
>>> > condition and add a dummy else. We have a nice little convenience
>>> > macro for that in drm headers, let's move it out. Subsequent patches
>>> > will roll it out to other places.
>>> >
>>> > The issue the compilers complain about are nested if with an else
>>> > block and no {} to disambiguate which if the else belongs to. The C
>>> > standard is clear, but in practice people forget:
>>> >
>>> > if (foo)
>>> >    if (bar)
>>> >            /* something */
>>> >    else
>>> >            /* something else
>>>
>>> um, yeah, don't do that.  Kernel coding style is very much to do
>>>
>>>      if (foo) {
>>>              if (bar)
>>>                      /* something */
>>>              else
>>>                      /* something else
>>>      }
>>>
>>> And if not doing that generates a warning then, well, do that.
>>>
>>> > The same can happen in a for_each macro when it also contains an if
>>> > condition at the end, except the compiler message is now really
>>> > confusing since there's only 1 if:
>>> >
>>> > for_each_something()
>>> >    if (bar)
>>> >            /* something */
>>> >    else
>>> >            /* something else
>>> >
>>> > The for_each_if() macro, by inverting the condition and adding an
>>> > else, avoids the compiler warning.
>>>
>>> Ditto.
>>>
>>> > Motivated by a discussion with Andy and Yisheng, who want to add
>>> > another for_each_macro which would benefit from for_each_if() instead
>>> > of hand-rolling it.
>>>
>>> Ditto.
>>>
>>> > v2: Explain a bit better what this is good for, after the discussion
>>> > with Peter Z.
>>>
>>> Presumably the above was discussed in whatever-thread-that-was.
>>
>> So there's a bunch of open coded versions of this already in kernel
>> headers (at least the ones I've found). Not counting the big pile of
>> existing users in drm. They are all wrong and should be reverted to a
>> plain if? That why there's a bunch more patches in this series.
>>
>> And yes I made it clear in the discussion that if you sprinkle enough {}
>> there's no warning, should have probably captured this here.
>>
>> Aka a formal Nack-pls-keep-your-stuff-in-drm: would be appreciated so I
>> can stop bothering with this.
>
> I think is it problematic to have macros like
>
> #define for_each_foo(...) for (......) if (....)
>
> because
>    for_each_foo(...)
>       if (x) ....; else ......;
>
> is handled badly.
> So in that sense, your work seems like a good thing.
>
> However it isn't clear to me that you need a new macro.
> The above macro could simply be changed to
>
> #define for_each_foo(...) for (......) if (!....);else
>
> Clearly people don't always think to do this, but would adding a macro
> help people to think?
>
> If we were to have a macro, it isn't clear to me that for_each_if() is a
> good name.
> Every other macro I've seen that starts "for_each_" causes the body to
> loop.  This one doesn't.  If someone doesn't know what for_each_if()
> does and sees it in code, they are unlikely to jump to the right
> conclusion.
> I would suggest that "__if" would be a better choice.  I think most
> people would guess that means "like 'if', but a bit different", which is
> fairly accurate.
>
> I think the only sure way to avoid bad macros being written is to teach
> some static checker to warn about any macro with a dangling "if".
> Possibly checkpatch.pl could do that (but I'm not volunteering).
>
> I do agree that it would be good to do something, and if people find
> for_each_fi() to actually reduce the number of poorly written macros,
> then I don't object to it.

There's also the proposal of if_noelse() which I think fares a bit
better than the __if().

But I still have the situation that a bunch of maintainers acked this
and Andrew Morton defacto nacked it, which I guess means I'll keep the
macro in drm? The common way to go about this seems to be to just push
the patch series with the ack in some pull request to Linus and ignore
the people who raised questions, but not really my thing.
-Daniel
Andrew Morton July 11, 2018, 11:05 p.m. UTC | #6
On Wed, 11 Jul 2018 13:51:08 +0200 Daniel Vetter <daniel@ffwll.ch> wrote:

> But I still have the situation that a bunch of maintainers acked this
> and Andrew Morton defacto nacked it, which I guess means I'll keep the
> macro in drm? The common way to go about this seems to be to just push
> the patch series with the ack in some pull request to Linus and ignore
> the people who raised questions, but not really my thing.

Heh.

But, am I wrong?  Code which uses regular kernel style doesn't have
these issues.  We shouldn't be enabling irregular style - we should be
making such sites more regular.  The fact that the compiler generates a
nice warning in some cases simply helps us with that.
Daniel Vetter July 12, 2018, 6:39 a.m. UTC | #7
On Thu, Jul 12, 2018 at 1:05 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Wed, 11 Jul 2018 13:51:08 +0200 Daniel Vetter <daniel@ffwll.ch> wrote:
>> But I still have the situation that a bunch of maintainers acked this
>> and Andrew Morton defacto nacked it, which I guess means I'll keep the
>> macro in drm? The common way to go about this seems to be to just push
>> the patch series with the ack in some pull request to Linus and ignore
>> the people who raised questions, but not really my thing.
>
> Heh.
>
> But, am I wrong?  Code which uses regular kernel style doesn't have
> these issues.  We shouldn't be enabling irregular style - we should be
> making such sites more regular.  The fact that the compiler generates a
> nice warning in some cases simply helps us with that.

Yes liberal sprinkling of {} per coding style fixes it too. But given
that the if (!cond) ; else pattern is fairly common even outside of
drm it seems like not just graphics people think that little bit of
added robustness in iterator macros is worth it. Only reason I
bothered with this is because I've seen another open-coded version of
this pattern fly by.

Anyway I'm going on vacations for a few weeks now. Andy/Yisheng, I
guess if you want this it's up to you to haggle for a consensus around
this. If that doesn't happen I'm happy to keep it in drm headers.
-Daniel
NeilBrown July 13, 2018, 11:37 p.m. UTC | #8
On Wed, Jul 11 2018, Andrew Morton wrote:

> On Wed, 11 Jul 2018 13:51:08 +0200 Daniel Vetter <daniel@ffwll.ch> wrote:
>
>> But I still have the situation that a bunch of maintainers acked this
>> and Andrew Morton defacto nacked it, which I guess means I'll keep the
>> macro in drm? The common way to go about this seems to be to just push
>> the patch series with the ack in some pull request to Linus and ignore
>> the people who raised questions, but not really my thing.
>
> Heh.
>
> But, am I wrong?  Code which uses regular kernel style doesn't have
> these issues.  We shouldn't be enabling irregular style - we should be
> making such sites more regular.  The fact that the compiler generates a
> nice warning in some cases simply helps us with that.

I think you are wrong .... or at least, not completely correct.

I think it is perfectly acceptable in Linux to have code like:

  for (....)
  	if (x)
        	something();
        else
        	something_else();

Would you agree?  If not, then I'm the one who is wrong.  Otherwise....

The problem is that for certain poorly written for_each_foo() macros,
such as blkg_for_each_descendant_pre() (and several others identified in
this patch series), writing

   blkg_for_each_descendant_pre(...)
     	if (x)
        	something();
        else
        	something_else();

will trigger a compiler warning.  This is inconsistent with the
behaviour of a simple "for".
So I do think that the macros should be fixed, and I don't think that
sprinkling extra braces is an appropriate response.

I'm not personally convinced that writing
   if_no_else(cond)
is easier than just writing
   if (!(cond)); else

in these macros, but I do think that the macros should be fixed and
maybe this is the path-of-least-resistance to getting it done.

Thanks,
NeilBrown
Randy Dunlap July 13, 2018, 11:42 p.m. UTC | #9
On 07/13/2018 04:37 PM, NeilBrown wrote:
> On Wed, Jul 11 2018, Andrew Morton wrote:
> 
>> On Wed, 11 Jul 2018 13:51:08 +0200 Daniel Vetter <daniel@ffwll.ch> wrote:
>>
>>> But I still have the situation that a bunch of maintainers acked this
>>> and Andrew Morton defacto nacked it, which I guess means I'll keep the
>>> macro in drm? The common way to go about this seems to be to just push
>>> the patch series with the ack in some pull request to Linus and ignore
>>> the people who raised questions, but not really my thing.
>>
>> Heh.
>>
>> But, am I wrong?  Code which uses regular kernel style doesn't have
>> these issues.  We shouldn't be enabling irregular style - we should be
>> making such sites more regular.  The fact that the compiler generates a
>> nice warning in some cases simply helps us with that.
> 
> I think you are wrong .... or at least, not completely correct.
> 
> I think it is perfectly acceptable in Linux to have code like:
> 
>   for (....)
>   	if (x)
>         	something();
>         else
>         	something_else();
> 
> Would you agree?  If not, then I'm the one who is wrong.  Otherwise....

coding-style.rst says:
Also, use braces when a loop contains more than a single simple statement:


> The problem is that for certain poorly written for_each_foo() macros,
> such as blkg_for_each_descendant_pre() (and several others identified in
> this patch series), writing
> 
>    blkg_for_each_descendant_pre(...)
>      	if (x)
>         	something();
>         else
>         	something_else();
> 
> will trigger a compiler warning.  This is inconsistent with the
> behaviour of a simple "for".
> So I do think that the macros should be fixed, and I don't think that
> sprinkling extra braces is an appropriate response.
> 
> I'm not personally convinced that writing
>    if_no_else(cond)
> is easier than just writing
>    if (!(cond)); else

agreed.

> in these macros, but I do think that the macros should be fixed and
> maybe this is the path-of-least-resistance to getting it done.

I'm not opposed to fixing some macros, but some of these macros are just
ease-of-less-typing shortcuts.  They don't improve readability at all;
they harm it.  (of course, that is just one opinion :)
Andy Shevchenko July 16, 2018, 8:11 a.m. UTC | #10
On Fri, 2018-07-13 at 16:42 -0700, Randy Dunlap wrote:
> On 07/13/2018 04:37 PM, NeilBrown wrote:

> 
> coding-style.rst says:
> Also, use braces when a loop contains more than a single simple
> statement:

Independently on a) would we use some macro for condition, or b) fix
macros against this kind of nested conditions, there is another
weirdness we would like to avoid, i.e.

for_each_foo() {
 ...
} else {
 ...
}

It is written according to coding style, but too much weird.

So, summarize this discussion I think we would
- keep for_each_if() in DRM subsystem alone
- fix macros which are using positive condition 'if (cond)' by replacing
with 'if (!cond) {} else' form for sake of robustness.

Do you agree on that?
Randy Dunlap July 16, 2018, 3:41 p.m. UTC | #11
On 07/16/2018 01:11 AM, Andy Shevchenko wrote:
> On Fri, 2018-07-13 at 16:42 -0700, Randy Dunlap wrote:
>> On 07/13/2018 04:37 PM, NeilBrown wrote:
> 
>>
>> coding-style.rst says:
>> Also, use braces when a loop contains more than a single simple
>> statement:
> 
> Independently on a) would we use some macro for condition, or b) fix
> macros against this kind of nested conditions, there is another
> weirdness we would like to avoid, i.e.
> 
> for_each_foo() {
>  ...
> } else {
>  ...
> }
> 
> It is written according to coding style, but too much weird.

Yeah, that's odd.  Looks like else matches the for loop. (!)

> So, summarize this discussion I think we would
> - keep for_each_if() in DRM subsystem alone
> - fix macros which are using positive condition 'if (cond)' by replacing
> with 'if (!cond) {} else' form for sake of robustness.
> 
> Do you agree on that?

Sure, both of those sound good to me.

thanks,
NeilBrown July 16, 2018, 10:16 p.m. UTC | #12
On Mon, Jul 16 2018, Andy Shevchenko wrote:

> On Fri, 2018-07-13 at 16:42 -0700, Randy Dunlap wrote:
>> On 07/13/2018 04:37 PM, NeilBrown wrote:
>
>> 
>> coding-style.rst says:
>> Also, use braces when a loop contains more than a single simple
>> statement:
>
> Independently on a) would we use some macro for condition, or b) fix
> macros against this kind of nested conditions, there is another
> weirdness we would like to avoid, i.e.
>
> for_each_foo() {
>  ...
> } else {
>  ...
> }
>
> It is written according to coding style, but too much weird.

Agreed.  Too weird.

>
> So, summarize this discussion I think we would
> - keep for_each_if() in DRM subsystem alone
> - fix macros which are using positive condition 'if (cond)' by replacing
> with 'if (!cond) {} else' form for sake of robustness.
>
> Do you agree on that?

I agree.

Thanks,
NeilBrown

Patch
diff mbox

diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index f7a19c2a7a80..05350424a4d3 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -110,7 +110,4 @@  static inline bool drm_can_sleep(void)
 	return true;
 }
 
-/* helper for handling conditionals in various for_each macros */
-#define for_each_if(condition) if (!(condition)) {} else
-
 #endif
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 941dc0a5a877..4cb95ab9a5bc 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -71,6 +71,9 @@ 
  */
 #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
 
+/* helper for handling conditionals in various for_each macros */
+#define for_each_if(condition) if (!(condition)) {} else
+
 #define u64_to_user_ptr(x) (		\
 {					\
 	typecheck(u64, x);		\