| Message ID | 1357405778-13903-4-git-send-email-paulmck@linux.vnet.ibm.com |
|---|---|
| State | Accepted |
| Commit | 4930521ae10fd28ebc713107fd243c8044a95415 |
| Headers | show |
On Sat, Jan 05, 2013 at 09:09:36AM -0800, Paul E. McKenney wrote: > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> > > It turns out that gcc 4.8 warns on array indexes being out of bounds > unless it can prove otherwise. It gives this warning on some RCU > initialization code. Because this is far from any fastpath, add > an explicit check for array bounds and panic if so. This gives the > compiler enough information to figure out that the array index is never > out of bounds. > > However, if a similar false positive occurs on a fastpath, it will > probably be necessary to tell the compiler to keep its array-index > anxieties to itself. ;-) > > Markus Trippelsdorf <markus@trippelsdorf.de> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > --- > kernel/rcutree.c | 4 ++++ > 1 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/kernel/rcutree.c b/kernel/rcutree.c > index d145796..e0d9815 100644 > --- a/kernel/rcutree.c > +++ b/kernel/rcutree.c > @@ -2938,6 +2938,10 @@ static void __init rcu_init_one(struct rcu_state *rsp, > > BUILD_BUG_ON(MAX_RCU_LVLS > ARRAY_SIZE(buf)); /* Fix buf[] init! */ > > + /* Silence gcc 4.8 warning about array index out of range. */ > + if (rcu_num_lvls > RCU_NUM_LVLS) > + panic("rcu_init_one: rcu_num_lvls overflow"); Why not write this as BUG_ON(rcu_num_lvls > RCU_NUM_LVLS)? Given that the condition in question can never happen, you don't really need an explanatory message. I do find it surprising, though, that the compiler can't figure this one out, given that rcu_num_lvls gets initialized right before this in the same file (and likely inlined into the same function). I wonder if it thought some other code might change it unexpectedly, since rcu_num_lvls doesn't get declared as static? Unfortunately, the loop macros in rcutree.h make it difficult to make rcu_num_lvls static, but as far as I can tell only one use of those macros ever gets expanded outside of rcutree.c: the one in rcutree_trace.c. If you compile out tracing, and declare rcu_num_lvls static, does the warning go away? - Josh Triplett
On Mon, Jan 07, 2013 at 07:50:02AM -0800, Josh Triplett wrote: > On Sat, Jan 05, 2013 at 09:09:36AM -0800, Paul E. McKenney wrote: > > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> > > > > It turns out that gcc 4.8 warns on array indexes being out of bounds > > unless it can prove otherwise. It gives this warning on some RCU > > initialization code. Because this is far from any fastpath, add > > an explicit check for array bounds and panic if so. This gives the > > compiler enough information to figure out that the array index is never > > out of bounds. > > > > However, if a similar false positive occurs on a fastpath, it will > > probably be necessary to tell the compiler to keep its array-index > > anxieties to itself. ;-) > > > > Markus Trippelsdorf <markus@trippelsdorf.de> > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > > --- > > kernel/rcutree.c | 4 ++++ > > 1 files changed, 4 insertions(+), 0 deletions(-) > > > > diff --git a/kernel/rcutree.c b/kernel/rcutree.c > > index d145796..e0d9815 100644 > > --- a/kernel/rcutree.c > > +++ b/kernel/rcutree.c > > @@ -2938,6 +2938,10 @@ static void __init rcu_init_one(struct rcu_state *rsp, > > > > BUILD_BUG_ON(MAX_RCU_LVLS > ARRAY_SIZE(buf)); /* Fix buf[] init! */ > > > > + /* Silence gcc 4.8 warning about array index out of range. */ > > + if (rcu_num_lvls > RCU_NUM_LVLS) > > + panic("rcu_init_one: rcu_num_lvls overflow"); > > Why not write this as BUG_ON(rcu_num_lvls > RCU_NUM_LVLS)? Given that > the condition in question can never happen, you don't really need an > explanatory message. Good point, will do! > I do find it surprising, though, that the compiler can't figure this one > out, given that rcu_num_lvls gets initialized right before this in the > same file (and likely inlined into the same function). I wonder if it > thought some other code might change it unexpectedly, since rcu_num_lvls > doesn't get declared as static? Unfortunately, the loop macros in > rcutree.h make it difficult to make rcu_num_lvls static, but as far as I > can tell only one use of those macros ever gets expanded outside of > rcutree.c: the one in rcutree_trace.c. If you compile out tracing, and > declare rcu_num_lvls static, does the warning go away? I found it quite surprising also, hence the "array-index anxieties" above. I added Marcus on CC for his thoughts on this. Thanx, Paul
On Mon, Jan 07, 2013 at 09:16:02AM -0800, Paul E. McKenney wrote: > On Mon, Jan 07, 2013 at 07:50:02AM -0800, Josh Triplett wrote: > > On Sat, Jan 05, 2013 at 09:09:36AM -0800, Paul E. McKenney wrote: > > > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> > > > > > > It turns out that gcc 4.8 warns on array indexes being out of bounds > > > unless it can prove otherwise. It gives this warning on some RCU > > > initialization code. Because this is far from any fastpath, add > > > an explicit check for array bounds and panic if so. This gives the > > > compiler enough information to figure out that the array index is never > > > out of bounds. > > > > > > However, if a similar false positive occurs on a fastpath, it will > > > probably be necessary to tell the compiler to keep its array-index > > > anxieties to itself. ;-) > > > > > > Markus Trippelsdorf <markus@trippelsdorf.de> > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > > > --- > > > kernel/rcutree.c | 4 ++++ > > > 1 files changed, 4 insertions(+), 0 deletions(-) > > > > > > diff --git a/kernel/rcutree.c b/kernel/rcutree.c > > > index d145796..e0d9815 100644 > > > --- a/kernel/rcutree.c > > > +++ b/kernel/rcutree.c > > > @@ -2938,6 +2938,10 @@ static void __init rcu_init_one(struct rcu_state *rsp, > > > > > > BUILD_BUG_ON(MAX_RCU_LVLS > ARRAY_SIZE(buf)); /* Fix buf[] init! */ > > > > > > + /* Silence gcc 4.8 warning about array index out of range. */ > > > + if (rcu_num_lvls > RCU_NUM_LVLS) > > > + panic("rcu_init_one: rcu_num_lvls overflow"); > > > > Why not write this as BUG_ON(rcu_num_lvls > RCU_NUM_LVLS)? Given that > > the condition in question can never happen, you don't really need an > > explanatory message. > > Good point, will do! Ah, wait, BUG_ON() sometimes compiles to nothingness: #ifndef HAVE_ARCH_BUG_ON #define BUG_ON(condition) do { if (condition) ; } while(0) #endif So I do need the explicit "if". :-( Thanx, Paul > > I do find it surprising, though, that the compiler can't figure this one > > out, given that rcu_num_lvls gets initialized right before this in the > > same file (and likely inlined into the same function). I wonder if it > > thought some other code might change it unexpectedly, since rcu_num_lvls > > doesn't get declared as static? Unfortunately, the loop macros in > > rcutree.h make it difficult to make rcu_num_lvls static, but as far as I > > can tell only one use of those macros ever gets expanded outside of > > rcutree.c: the one in rcutree_trace.c. If you compile out tracing, and > > declare rcu_num_lvls static, does the warning go away? > > I found it quite surprising also, hence the "array-index anxieties" above. > > I added Marcus on CC for his thoughts on this. > > Thanx, Paul
On 2013.01.07 at 09:16 -0800, Paul E. McKenney wrote: > On Mon, Jan 07, 2013 at 07:50:02AM -0800, Josh Triplett wrote: > > On Sat, Jan 05, 2013 at 09:09:36AM -0800, Paul E. McKenney wrote: > > > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> > > > > > > It turns out that gcc 4.8 warns on array indexes being out of bounds > > > unless it can prove otherwise. It gives this warning on some RCU > > > initialization code. Because this is far from any fastpath, add > > > an explicit check for array bounds and panic if so. This gives the > > > compiler enough information to figure out that the array index is never > > > out of bounds. > > > > > > However, if a similar false positive occurs on a fastpath, it will > > > probably be necessary to tell the compiler to keep its array-index > > > anxieties to itself. ;-) > > > > > > Markus Trippelsdorf <markus@trippelsdorf.de> > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > > > --- > > > kernel/rcutree.c | 4 ++++ > > > 1 files changed, 4 insertions(+), 0 deletions(-) > > > > > > diff --git a/kernel/rcutree.c b/kernel/rcutree.c > > > index d145796..e0d9815 100644 > > > --- a/kernel/rcutree.c > > > +++ b/kernel/rcutree.c > > > @@ -2938,6 +2938,10 @@ static void __init rcu_init_one(struct rcu_state *rsp, > > > > > > BUILD_BUG_ON(MAX_RCU_LVLS > ARRAY_SIZE(buf)); /* Fix buf[] init! */ > > > > > > + /* Silence gcc 4.8 warning about array index out of range. */ > > > + if (rcu_num_lvls > RCU_NUM_LVLS) > > > + panic("rcu_init_one: rcu_num_lvls overflow"); > > > I do find it surprising, though, that the compiler can't figure this one > > out, given that rcu_num_lvls gets initialized right before this in the > > same file (and likely inlined into the same function). I wonder if it > > thought some other code might change it unexpectedly, since rcu_num_lvls > > doesn't get declared as static? Unfortunately, the loop macros in > > rcutree.h make it difficult to make rcu_num_lvls static, but as far as I > > can tell only one use of those macros ever gets expanded outside of > > rcutree.c: the one in rcutree_trace.c. If you compile out tracing, and > > declare rcu_num_lvls static, does the warning go away? > > I found it quite surprising also, hence the "array-index anxieties" above. Yes, declaring rcu_num_lvls static would fix the issue. See the following gcc bug: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55529 Please note that this was the only warning of this kind that occurred with an allyesconfig.
On Mon, Jan 07, 2013 at 07:08:55PM +0100, Markus Trippelsdorf wrote: > On 2013.01.07 at 09:16 -0800, Paul E. McKenney wrote: > > On Mon, Jan 07, 2013 at 07:50:02AM -0800, Josh Triplett wrote: > > > On Sat, Jan 05, 2013 at 09:09:36AM -0800, Paul E. McKenney wrote: > > > > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> > > > > > > > > It turns out that gcc 4.8 warns on array indexes being out of bounds > > > > unless it can prove otherwise. It gives this warning on some RCU > > > > initialization code. Because this is far from any fastpath, add > > > > an explicit check for array bounds and panic if so. This gives the > > > > compiler enough information to figure out that the array index is never > > > > out of bounds. > > > > > > > > However, if a similar false positive occurs on a fastpath, it will > > > > probably be necessary to tell the compiler to keep its array-index > > > > anxieties to itself. ;-) > > > > > > > > Markus Trippelsdorf <markus@trippelsdorf.de> > > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > > > > --- > > > > kernel/rcutree.c | 4 ++++ > > > > 1 files changed, 4 insertions(+), 0 deletions(-) > > > > > > > > diff --git a/kernel/rcutree.c b/kernel/rcutree.c > > > > index d145796..e0d9815 100644 > > > > --- a/kernel/rcutree.c > > > > +++ b/kernel/rcutree.c > > > > @@ -2938,6 +2938,10 @@ static void __init rcu_init_one(struct rcu_state *rsp, > > > > > > > > BUILD_BUG_ON(MAX_RCU_LVLS > ARRAY_SIZE(buf)); /* Fix buf[] init! */ > > > > > > > > + /* Silence gcc 4.8 warning about array index out of range. */ > > > > + if (rcu_num_lvls > RCU_NUM_LVLS) > > > > + panic("rcu_init_one: rcu_num_lvls overflow"); > > > > > I do find it surprising, though, that the compiler can't figure this one > > > out, given that rcu_num_lvls gets initialized right before this in the > > > same file (and likely inlined into the same function). I wonder if it > > > thought some other code might change it unexpectedly, since rcu_num_lvls > > > doesn't get declared as static? Unfortunately, the loop macros in > > > rcutree.h make it difficult to make rcu_num_lvls static, but as far as I > > > can tell only one use of those macros ever gets expanded outside of > > > rcutree.c: the one in rcutree_trace.c. If you compile out tracing, and > > > declare rcu_num_lvls static, does the warning go away? > > > > I found it quite surprising also, hence the "array-index anxieties" above. > > Yes, declaring rcu_num_lvls static would fix the issue. See the > following gcc bug: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55529 Seems potentially worth restructuring the code a bit to fold in the one rcutree_trace.c bit that needs it and then making rcu_num_lvls static and internal to rcutree.c. - Josh Triplett
On Mon, Jan 07, 2013 at 10:24:17AM -0800, Josh Triplett wrote: > On Mon, Jan 07, 2013 at 07:08:55PM +0100, Markus Trippelsdorf wrote: > > On 2013.01.07 at 09:16 -0800, Paul E. McKenney wrote: > > > On Mon, Jan 07, 2013 at 07:50:02AM -0800, Josh Triplett wrote: > > > > On Sat, Jan 05, 2013 at 09:09:36AM -0800, Paul E. McKenney wrote: > > > > > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> > > > > > > > > > > It turns out that gcc 4.8 warns on array indexes being out of bounds > > > > > unless it can prove otherwise. It gives this warning on some RCU > > > > > initialization code. Because this is far from any fastpath, add > > > > > an explicit check for array bounds and panic if so. This gives the > > > > > compiler enough information to figure out that the array index is never > > > > > out of bounds. > > > > > > > > > > However, if a similar false positive occurs on a fastpath, it will > > > > > probably be necessary to tell the compiler to keep its array-index > > > > > anxieties to itself. ;-) > > > > > > > > > > Markus Trippelsdorf <markus@trippelsdorf.de> > > > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > > > > > --- > > > > > kernel/rcutree.c | 4 ++++ > > > > > 1 files changed, 4 insertions(+), 0 deletions(-) > > > > > > > > > > diff --git a/kernel/rcutree.c b/kernel/rcutree.c > > > > > index d145796..e0d9815 100644 > > > > > --- a/kernel/rcutree.c > > > > > +++ b/kernel/rcutree.c > > > > > @@ -2938,6 +2938,10 @@ static void __init rcu_init_one(struct rcu_state *rsp, > > > > > > > > > > BUILD_BUG_ON(MAX_RCU_LVLS > ARRAY_SIZE(buf)); /* Fix buf[] init! */ > > > > > > > > > > + /* Silence gcc 4.8 warning about array index out of range. */ > > > > > + if (rcu_num_lvls > RCU_NUM_LVLS) > > > > > + panic("rcu_init_one: rcu_num_lvls overflow"); > > > > > > > I do find it surprising, though, that the compiler can't figure this one > > > > out, given that rcu_num_lvls gets initialized right before this in the > > > > same file (and likely inlined into the same function). I wonder if it > > > > thought some other code might change it unexpectedly, since rcu_num_lvls > > > > doesn't get declared as static? Unfortunately, the loop macros in > > > > rcutree.h make it difficult to make rcu_num_lvls static, but as far as I > > > > can tell only one use of those macros ever gets expanded outside of > > > > rcutree.c: the one in rcutree_trace.c. If you compile out tracing, and > > > > declare rcu_num_lvls static, does the warning go away? > > > > > > I found it quite surprising also, hence the "array-index anxieties" above. > > > > Yes, declaring rcu_num_lvls static would fix the issue. See the > > following gcc bug: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55529 > > Seems potentially worth restructuring the code a bit to fold in the one > rcutree_trace.c bit that needs it and then making rcu_num_lvls static > and internal to rcutree.c. One thing to keep in mind... The added "if" is in initialization code that runs once, and whose memory is reclaimed. Restructuring the runtime code will increase complexity and probably result in some degradation. So I am not convinced that a change is really needed. Thanx, Paul
On Mon, 2013-01-07 at 09:19 -0800, Paul E. McKenney wrote: > On Mon, Jan 07, 2013 at 09:16:02AM -0800, Paul E. McKenney wrote: > > On Mon, Jan 07, 2013 at 07:50:02AM -0800, Josh Triplett wrote: > > > On Sat, Jan 05, 2013 at 09:09:36AM -0800, Paul E. McKenney wrote: > > > > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> > > > > > > > > It turns out that gcc 4.8 warns on array indexes being out of bounds > > > > unless it can prove otherwise. It gives this warning on some RCU > > > > initialization code. Because this is far from any fastpath, add > > > > an explicit check for array bounds and panic if so. This gives the > > > > compiler enough information to figure out that the array index is never > > > > out of bounds. > > > > > > > > However, if a similar false positive occurs on a fastpath, it will > > > > probably be necessary to tell the compiler to keep its array-index > > > > anxieties to itself. ;-) > > > > > > > > Markus Trippelsdorf <markus@trippelsdorf.de> > > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > > > > --- > > > > kernel/rcutree.c | 4 ++++ > > > > 1 files changed, 4 insertions(+), 0 deletions(-) > > > > > > > > diff --git a/kernel/rcutree.c b/kernel/rcutree.c > > > > index d145796..e0d9815 100644 > > > > --- a/kernel/rcutree.c > > > > +++ b/kernel/rcutree.c > > > > @@ -2938,6 +2938,10 @@ static void __init rcu_init_one(struct rcu_state *rsp, > > > > > > > > BUILD_BUG_ON(MAX_RCU_LVLS > ARRAY_SIZE(buf)); /* Fix buf[] init! */ > > > > > > > > + /* Silence gcc 4.8 warning about array index out of range. */ > > > > + if (rcu_num_lvls > RCU_NUM_LVLS) > > > > + panic("rcu_init_one: rcu_num_lvls overflow"); > > > > > > Why not write this as BUG_ON(rcu_num_lvls > RCU_NUM_LVLS)? Given that > > > the condition in question can never happen, you don't really need an > > > explanatory message. > > > > Good point, will do! > > Ah, wait, BUG_ON() sometimes compiles to nothingness: > > #ifndef HAVE_ARCH_BUG_ON > #define BUG_ON(condition) do { if (condition) ; } while(0) > #endif > > So I do need the explicit "if". :-( Bah, those archs shouldn't be bothered with. If they don't want to bug, then that's there problem :-) Lots of places in the kernel have BUG_ON() where they require it to panic. -- Steve
On Mon, Jan 07, 2013 at 05:18:37PM -0500, Steven Rostedt wrote: > On Mon, 2013-01-07 at 09:19 -0800, Paul E. McKenney wrote: > > On Mon, Jan 07, 2013 at 09:16:02AM -0800, Paul E. McKenney wrote: > > > On Mon, Jan 07, 2013 at 07:50:02AM -0800, Josh Triplett wrote: > > > > On Sat, Jan 05, 2013 at 09:09:36AM -0800, Paul E. McKenney wrote: > > > > > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> > > > > > > > > > > It turns out that gcc 4.8 warns on array indexes being out of bounds > > > > > unless it can prove otherwise. It gives this warning on some RCU > > > > > initialization code. Because this is far from any fastpath, add > > > > > an explicit check for array bounds and panic if so. This gives the > > > > > compiler enough information to figure out that the array index is never > > > > > out of bounds. > > > > > > > > > > However, if a similar false positive occurs on a fastpath, it will > > > > > probably be necessary to tell the compiler to keep its array-index > > > > > anxieties to itself. ;-) > > > > > > > > > > Markus Trippelsdorf <markus@trippelsdorf.de> > > > > > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > > > > > --- > > > > > kernel/rcutree.c | 4 ++++ > > > > > 1 files changed, 4 insertions(+), 0 deletions(-) > > > > > > > > > > diff --git a/kernel/rcutree.c b/kernel/rcutree.c > > > > > index d145796..e0d9815 100644 > > > > > --- a/kernel/rcutree.c > > > > > +++ b/kernel/rcutree.c > > > > > @@ -2938,6 +2938,10 @@ static void __init rcu_init_one(struct rcu_state *rsp, > > > > > > > > > > BUILD_BUG_ON(MAX_RCU_LVLS > ARRAY_SIZE(buf)); /* Fix buf[] init! */ > > > > > > > > > > + /* Silence gcc 4.8 warning about array index out of range. */ > > > > > + if (rcu_num_lvls > RCU_NUM_LVLS) > > > > > + panic("rcu_init_one: rcu_num_lvls overflow"); > > > > > > > > Why not write this as BUG_ON(rcu_num_lvls > RCU_NUM_LVLS)? Given that > > > > the condition in question can never happen, you don't really need an > > > > explanatory message. > > > > > > Good point, will do! > > > > Ah, wait, BUG_ON() sometimes compiles to nothingness: > > > > #ifndef HAVE_ARCH_BUG_ON > > #define BUG_ON(condition) do { if (condition) ; } while(0) > > #endif > > > > So I do need the explicit "if". :-( > > Bah, those archs shouldn't be bothered with. If they don't want to bug, > then that's there problem :-) ;-) ;-) ;-) > Lots of places in the kernel have BUG_ON() where they require it to > panic. Fair point, but that doesn't mean that I want them complaining to me when as a result of the compiler's array-index anxieties. Thanx, Paul
diff --git a/kernel/rcutree.c b/kernel/rcutree.c index d145796..e0d9815 100644 --- a/kernel/rcutree.c +++ b/kernel/rcutree.c @@ -2938,6 +2938,10 @@ static void __init rcu_init_one(struct rcu_state *rsp, BUILD_BUG_ON(MAX_RCU_LVLS > ARRAY_SIZE(buf)); /* Fix buf[] init! */ + /* Silence gcc 4.8 warning about array index out of range. */ + if (rcu_num_lvls > RCU_NUM_LVLS) + panic("rcu_init_one: rcu_num_lvls overflow"); + /* Initialize the level-tracking arrays. */ for (i = 0; i < rcu_num_lvls; i++)