* [PATCH] Staging: dgnc: fixed code style issue @ 2015-04-19 19:18 Yorick Rommers 2015-04-19 20:34 ` Joe Perches 2015-04-20 8:13 ` [PATCH] Staging: dgnc: fixed code style issue Dan Carpenter 0 siblings, 2 replies; 15+ messages in thread From: Yorick Rommers @ 2015-04-19 19:18 UTC (permalink / raw) To: lidza.louina, markh, gregkh Cc: driverdev-devel, devel, linux-kernel, Yorick Rommers A patch for a line being too long (>80) in dgnc_mgmt.c, fixed by making a temporary value for dgnc_Board[brd], and removing an unnecessary typecast. Signed-off-by: Yorick Rommers <yorick-rommers@hotmail.com> --- drivers/staging/dgnc/dgnc_mgmt.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/staging/dgnc/dgnc_mgmt.c b/drivers/staging/dgnc/dgnc_mgmt.c index b13318a..0e75d37 100644 --- a/drivers/staging/dgnc/dgnc_mgmt.c +++ b/drivers/staging/dgnc/dgnc_mgmt.c @@ -143,12 +143,14 @@ long dgnc_mgmt_ioctl(struct file *file, unsigned int cmd, unsigned long arg) di.info_bdnum = brd; spin_lock_irqsave(&dgnc_Board[brd]->bd_lock, flags); + struct dgnc_board *bd = dgnc_Board[brd]; di.info_bdtype = dgnc_Board[brd]->dpatype; di.info_bdstate = dgnc_Board[brd]->dpastatus; di.info_ioport = 0; di.info_physaddr = (ulong) dgnc_Board[brd]->membase; - di.info_physsize = (ulong) dgnc_Board[brd]->membase - dgnc_Board[brd]->membase_end; + di.info_physsize = bd->membase - bd->membase_end; + if (dgnc_Board[brd]->state != BOARD_FAILED) di.info_nports = dgnc_Board[brd]->nasync; else -- 2.3.5 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] Staging: dgnc: fixed code style issue 2015-04-19 19:18 [PATCH] Staging: dgnc: fixed code style issue Yorick Rommers @ 2015-04-19 20:34 ` Joe Perches 2015-04-19 21:58 ` [PATCH] Staging: dgnc: Using temporary value for repeated dereferences Yorick Rommers 2015-04-20 8:13 ` [PATCH] Staging: dgnc: fixed code style issue Dan Carpenter 1 sibling, 1 reply; 15+ messages in thread From: Joe Perches @ 2015-04-19 20:34 UTC (permalink / raw) To: Yorick Rommers Cc: lidza.louina, markh, gregkh, driverdev-devel, devel, linux-kernel On Sun, 2015-04-19 at 21:18 +0200, Yorick Rommers wrote: > A patch for a line being too long (>80) in dgnc_mgmt.c, > fixed by making a temporary value for dgnc_Board[brd], and removing > an unnecessary typecast. Hello Yorick. The patch subject isn't very descriptive. A better subject might be something like: "staging: dgnc: Use a temporary for repeated dereferences" > diff --git a/drivers/staging/dgnc/dgnc_mgmt.c b/drivers/staging/dgnc/dgnc_mgmt.c [] > @@ -143,12 +143,14 @@ long dgnc_mgmt_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > di.info_bdnum = brd; > > spin_lock_irqsave(&dgnc_Board[brd]->bd_lock, flags); > + struct dgnc_board *bd = dgnc_Board[brd]; Linux style doesn't declare variables in the middle of a function, Variables are only declared after an open brace. > > di.info_bdtype = dgnc_Board[brd]->dpatype; > di.info_bdstate = dgnc_Board[brd]->dpastatus; Please change all the references of dgnc_Board[brd]-> to bd-> in this function instead of just the long line ones. > di.info_ioport = 0; > di.info_physaddr = (ulong) dgnc_Board[brd]->membase; > - di.info_physsize = (ulong) dgnc_Board[brd]->membase - dgnc_Board[brd]->membase_end; > + di.info_physsize = bd->membase - bd->membase_end; > + > if (dgnc_Board[brd]->state != BOARD_FAILED) > di.info_nports = dgnc_Board[brd]->nasync; > else Something like: drivers/staging/dgnc/dgnc_mgmt.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/drivers/staging/dgnc/dgnc_mgmt.c b/drivers/staging/dgnc/dgnc_mgmt.c index b13318a..215f37b 100644 --- a/drivers/staging/dgnc/dgnc_mgmt.c +++ b/drivers/staging/dgnc/dgnc_mgmt.c @@ -129,8 +129,8 @@ long dgnc_mgmt_ioctl(struct file *file, unsigned int cmd, unsigned long arg) case DIGI_GETBD: { int brd; - struct digi_info di; + struct dgnc_board *bd; if (copy_from_user(&brd, uarg, sizeof(int))) return -EFAULT; @@ -142,19 +142,21 @@ long dgnc_mgmt_ioctl(struct file *file, unsigned int cmd, unsigned long arg) di.info_bdnum = brd; - spin_lock_irqsave(&dgnc_Board[brd]->bd_lock, flags); + bd = dgnc_Board[brd]; + + spin_lock_irqsave(&bd->bd_lock, flags); - di.info_bdtype = dgnc_Board[brd]->dpatype; - di.info_bdstate = dgnc_Board[brd]->dpastatus; + di.info_bdtype = bd->dpatype; + di.info_bdstate = bd->dpastatus; di.info_ioport = 0; - di.info_physaddr = (ulong) dgnc_Board[brd]->membase; - di.info_physsize = (ulong) dgnc_Board[brd]->membase - dgnc_Board[brd]->membase_end; - if (dgnc_Board[brd]->state != BOARD_FAILED) - di.info_nports = dgnc_Board[brd]->nasync; + di.info_physaddr = bd->membase; + di.info_physsize = bd->membase - bd->membase_end; + if (bd->state != BOARD_FAILED) + di.info_nports = bd->nasync; else di.info_nports = 0; - spin_unlock_irqrestore(&dgnc_Board[brd]->bd_lock, flags); + spin_unlock_irqrestore(&bd->bd_lock, flags); if (copy_to_user(uarg, &di, sizeof(di))) return -EFAULT; ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH] Staging: dgnc: Using temporary value for repeated dereferences. 2015-04-19 20:34 ` Joe Perches @ 2015-04-19 21:58 ` Yorick Rommers 2015-04-20 0:54 ` Joe Perches 2015-04-20 8:27 ` [PATCH] Staging: dgnc: Using temporary value for repeated dereferences Dan Carpenter 0 siblings, 2 replies; 15+ messages in thread From: Yorick Rommers @ 2015-04-19 21:58 UTC (permalink / raw) To: joe Cc: lidza.louina, gregkh, markh, driverdev-devel, devel, linux-kernel, Yorick Rommers Hello Joe, Thank you once again for the feedback. I've changed my patch accordingly, see below. -------------------------------------- A patch for a line being too long (>80) in dgnc_mgmt.c, fixed by making a temporary value for dgnc_Board[brd], replacing all instanced of dgnc_Board[brd] with temporary value, and removing unnecessary typecasts. Signed-off-by: Yorick Rommers <yorick-rommers@hotmail.com> --- drivers/staging/dgnc/dgnc_mgmt.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/drivers/staging/dgnc/dgnc_mgmt.c b/drivers/staging/dgnc/dgnc_mgmt.c index b13318a..0437117 100644 --- a/drivers/staging/dgnc/dgnc_mgmt.c +++ b/drivers/staging/dgnc/dgnc_mgmt.c @@ -131,6 +131,7 @@ long dgnc_mgmt_ioctl(struct file *file, unsigned int cmd, unsigned long arg) int brd; struct digi_info di; + struct dgnc_board *bd = dgnc_Board[brd]; if (copy_from_user(&brd, uarg, sizeof(int))) return -EFAULT; @@ -142,19 +143,19 @@ long dgnc_mgmt_ioctl(struct file *file, unsigned int cmd, unsigned long arg) di.info_bdnum = brd; - spin_lock_irqsave(&dgnc_Board[brd]->bd_lock, flags); + spin_lock_irqsave(&bd->bd_lock, flags); - di.info_bdtype = dgnc_Board[brd]->dpatype; - di.info_bdstate = dgnc_Board[brd]->dpastatus; + di.info_bdtype = bd->dpatype; + di.info_bdstate = bd->dpastatus; di.info_ioport = 0; - di.info_physaddr = (ulong) dgnc_Board[brd]->membase; - di.info_physsize = (ulong) dgnc_Board[brd]->membase - dgnc_Board[brd]->membase_end; - if (dgnc_Board[brd]->state != BOARD_FAILED) - di.info_nports = dgnc_Board[brd]->nasync; + di.info_physaddr = bd->membase; + di.info_physsize = bd->membase - bd->membase_end; + if (bd->state != BOARD_FAILED) + di.info_nports = bd->nasync; else di.info_nports = 0; - spin_unlock_irqrestore(&dgnc_Board[brd]->bd_lock, flags); + spin_unlock_irqrestore(&bd->bd_lock, flags); if (copy_to_user(uarg, &di, sizeof(di))) return -EFAULT; -- 2.3.5 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] Staging: dgnc: Using temporary value for repeated dereferences. 2015-04-19 21:58 ` [PATCH] Staging: dgnc: Using temporary value for repeated dereferences Yorick Rommers @ 2015-04-20 0:54 ` Joe Perches 2015-04-20 8:27 ` [PATCH] Staging: dgnc: Using temporary value for repeated dereferences Dan Carpenter 1 sibling, 0 replies; 15+ messages in thread From: Joe Perches @ 2015-04-20 0:54 UTC (permalink / raw) To: Yorick Rommers Cc: lidza.louina, gregkh, markh, driverdev-devel, devel, linux-kernel On Sun, 2015-04-19 at 23:58 +0200, Yorick Rommers wrote: > Hello Joe, > > Thank you once again for the feedback. > I've changed my patch accordingly, see below. > > -------------------------------------- > > A patch for a line being too long (>80) in dgnc_mgmt.c, > fixed by making a temporary value for dgnc_Board[brd], > replacing all instanced of dgnc_Board[brd] with temporary value, > and removing unnecessary typecasts. > > Signed-off-by: Yorick Rommers <yorick-rommers@hotmail.com> > --- > drivers/staging/dgnc/dgnc_mgmt.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/drivers/staging/dgnc/dgnc_mgmt.c b/drivers/staging/dgnc/dgnc_mgmt.c > index b13318a..0437117 100644 > --- a/drivers/staging/dgnc/dgnc_mgmt.c > +++ b/drivers/staging/dgnc/dgnc_mgmt.c > @@ -131,6 +131,7 @@ long dgnc_mgmt_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > int brd; > > struct digi_info di; > + struct dgnc_board *bd = dgnc_Board[brd]; Please read the code. brd is got from userspace and you've dereferenced it before getting the value from userspace. > if (copy_from_user(&brd, uarg, sizeof(int))) > return -EFAULT; Look again at the code I suggested. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Staging: dgnc: Using temporary value for repeated dereferences. @ 2015-04-20 0:54 ` Joe Perches 0 siblings, 0 replies; 15+ messages in thread From: Joe Perches @ 2015-04-20 0:54 UTC (permalink / raw) To: Yorick Rommers; +Cc: devel, lidza.louina, driverdev-devel, linux-kernel, gregkh On Sun, 2015-04-19 at 23:58 +0200, Yorick Rommers wrote: > Hello Joe, > > Thank you once again for the feedback. > I've changed my patch accordingly, see below. > > -------------------------------------- > > A patch for a line being too long (>80) in dgnc_mgmt.c, > fixed by making a temporary value for dgnc_Board[brd], > replacing all instanced of dgnc_Board[brd] with temporary value, > and removing unnecessary typecasts. > > Signed-off-by: Yorick Rommers <yorick-rommers@hotmail.com> > --- > drivers/staging/dgnc/dgnc_mgmt.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/drivers/staging/dgnc/dgnc_mgmt.c b/drivers/staging/dgnc/dgnc_mgmt.c > index b13318a..0437117 100644 > --- a/drivers/staging/dgnc/dgnc_mgmt.c > +++ b/drivers/staging/dgnc/dgnc_mgmt.c > @@ -131,6 +131,7 @@ long dgnc_mgmt_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > int brd; > > struct digi_info di; > + struct dgnc_board *bd = dgnc_Board[brd]; Please read the code. brd is got from userspace and you've dereferenced it before getting the value from userspace. > if (copy_from_user(&brd, uarg, sizeof(int))) > return -EFAULT; Look again at the code I suggested. _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] Staging: dgnc: Using a temporary value for repeated dereferences. 2015-04-20 0:54 ` Joe Perches @ 2015-04-20 7:33 ` Yorick Rommers -1 siblings, 0 replies; 15+ messages in thread From: Yorick Rommers @ 2015-04-20 7:33 UTC (permalink / raw) To: joe Cc: lidza.louina, markh, gregkh, driverdev-devel, devel, linux-kernel, Yorick Rommers Sorry, it has been changed in the patch below. --------------------------------------- A patch for a line being too long (>80) in dgnc_mgmt.c, fixed by making a temporary value for dgnc_Board[brd], replacing all instanced of dgnc_Board[brd] with temporary value, and removing unnecessary typecasts. Signed-off-by: Yorick Rommers <yorick-rommers@hotmail.com> --- drivers/staging/dgnc/dgnc_mgmt.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/drivers/staging/dgnc/dgnc_mgmt.c b/drivers/staging/dgnc/dgnc_mgmt.c index b13318a..ac2581a 100644 --- a/drivers/staging/dgnc/dgnc_mgmt.c +++ b/drivers/staging/dgnc/dgnc_mgmt.c @@ -131,6 +131,7 @@ long dgnc_mgmt_ioctl(struct file *file, unsigned int cmd, unsigned long arg) int brd; struct digi_info di; + struct dgnc_board *bd; if (copy_from_user(&brd, uarg, sizeof(int))) return -EFAULT; @@ -142,19 +143,21 @@ long dgnc_mgmt_ioctl(struct file *file, unsigned int cmd, unsigned long arg) di.info_bdnum = brd; - spin_lock_irqsave(&dgnc_Board[brd]->bd_lock, flags); + bd = dgnc_Board[brd]; - di.info_bdtype = dgnc_Board[brd]->dpatype; - di.info_bdstate = dgnc_Board[brd]->dpastatus; + spin_lock_irqsave(&bd->bd_lock, flags); + + di.info_bdtype = bd->dpatype; + di.info_bdstate = bd->dpastatus; di.info_ioport = 0; - di.info_physaddr = (ulong) dgnc_Board[brd]->membase; - di.info_physsize = (ulong) dgnc_Board[brd]->membase - dgnc_Board[brd]->membase_end; - if (dgnc_Board[brd]->state != BOARD_FAILED) - di.info_nports = dgnc_Board[brd]->nasync; + di.info_physaddr = bd->membase; + di.info_physsize = bd->membase - bd->membase_end; + if (bd->state != BOARD_FAILED) + di.info_nports = bd->nasync; else di.info_nports = 0; - spin_unlock_irqrestore(&dgnc_Board[brd]->bd_lock, flags); + spin_unlock_irqrestore(&bd->bd_lock, flags); if (copy_to_user(uarg, &di, sizeof(di))) return -EFAULT; -- 2.3.5 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH] Staging: dgnc: Using a temporary value for repeated dereferences. @ 2015-04-20 7:33 ` Yorick Rommers 0 siblings, 0 replies; 15+ messages in thread From: Yorick Rommers @ 2015-04-20 7:33 UTC (permalink / raw) To: joe Cc: devel, lidza.louina, driverdev-devel, linux-kernel, gregkh, Yorick Rommers Sorry, it has been changed in the patch below. --------------------------------------- A patch for a line being too long (>80) in dgnc_mgmt.c, fixed by making a temporary value for dgnc_Board[brd], replacing all instanced of dgnc_Board[brd] with temporary value, and removing unnecessary typecasts. Signed-off-by: Yorick Rommers <yorick-rommers@hotmail.com> --- drivers/staging/dgnc/dgnc_mgmt.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/drivers/staging/dgnc/dgnc_mgmt.c b/drivers/staging/dgnc/dgnc_mgmt.c index b13318a..ac2581a 100644 --- a/drivers/staging/dgnc/dgnc_mgmt.c +++ b/drivers/staging/dgnc/dgnc_mgmt.c @@ -131,6 +131,7 @@ long dgnc_mgmt_ioctl(struct file *file, unsigned int cmd, unsigned long arg) int brd; struct digi_info di; + struct dgnc_board *bd; if (copy_from_user(&brd, uarg, sizeof(int))) return -EFAULT; @@ -142,19 +143,21 @@ long dgnc_mgmt_ioctl(struct file *file, unsigned int cmd, unsigned long arg) di.info_bdnum = brd; - spin_lock_irqsave(&dgnc_Board[brd]->bd_lock, flags); + bd = dgnc_Board[brd]; - di.info_bdtype = dgnc_Board[brd]->dpatype; - di.info_bdstate = dgnc_Board[brd]->dpastatus; + spin_lock_irqsave(&bd->bd_lock, flags); + + di.info_bdtype = bd->dpatype; + di.info_bdstate = bd->dpastatus; di.info_ioport = 0; - di.info_physaddr = (ulong) dgnc_Board[brd]->membase; - di.info_physsize = (ulong) dgnc_Board[brd]->membase - dgnc_Board[brd]->membase_end; - if (dgnc_Board[brd]->state != BOARD_FAILED) - di.info_nports = dgnc_Board[brd]->nasync; + di.info_physaddr = bd->membase; + di.info_physsize = bd->membase - bd->membase_end; + if (bd->state != BOARD_FAILED) + di.info_nports = bd->nasync; else di.info_nports = 0; - spin_unlock_irqrestore(&dgnc_Board[brd]->bd_lock, flags); + spin_unlock_irqrestore(&bd->bd_lock, flags); if (copy_to_user(uarg, &di, sizeof(di))) return -EFAULT; -- 2.3.5 _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] Staging: dgnc: Using a temporary value for repeated dereferences. 2015-04-20 7:33 ` Yorick Rommers @ 2015-05-03 18:50 ` Greg KH -1 siblings, 0 replies; 15+ messages in thread From: Greg KH @ 2015-05-03 18:50 UTC (permalink / raw) To: Yorick Rommers; +Cc: joe, devel, lidza.louina, driverdev-devel, linux-kernel On Mon, Apr 20, 2015 at 09:33:04AM +0200, Yorick Rommers wrote: > Sorry, it has been changed in the patch below. > > --------------------------------------- > > A patch for a line being too long (>80) in dgnc_mgmt.c, > fixed by making a temporary value for dgnc_Board[brd], > replacing all instanced of dgnc_Board[brd] with temporary value, > and removing unnecessary typecasts. > > Signed-off-by: Yorick Rommers <yorick-rommers@hotmail.com> > --- > drivers/staging/dgnc/dgnc_mgmt.c | 19 +++++++++++-------- > 1 file changed, 11 insertions(+), 8 deletions(-) I can't apply this as-is, please fix up and resend in a format that I can apply it in. thanks, greg k-h ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Staging: dgnc: Using a temporary value for repeated dereferences. @ 2015-05-03 18:50 ` Greg KH 0 siblings, 0 replies; 15+ messages in thread From: Greg KH @ 2015-05-03 18:50 UTC (permalink / raw) To: Yorick Rommers; +Cc: joe, devel, driverdev-devel, linux-kernel, lidza.louina On Mon, Apr 20, 2015 at 09:33:04AM +0200, Yorick Rommers wrote: > Sorry, it has been changed in the patch below. > > --------------------------------------- > > A patch for a line being too long (>80) in dgnc_mgmt.c, > fixed by making a temporary value for dgnc_Board[brd], > replacing all instanced of dgnc_Board[brd] with temporary value, > and removing unnecessary typecasts. > > Signed-off-by: Yorick Rommers <yorick-rommers@hotmail.com> > --- > drivers/staging/dgnc/dgnc_mgmt.c | 19 +++++++++++-------- > 1 file changed, 11 insertions(+), 8 deletions(-) I can't apply this as-is, please fix up and resend in a format that I can apply it in. thanks, greg k-h _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Staging: dgnc: Using temporary value for repeated dereferences. 2015-04-20 0:54 ` Joe Perches (?) (?) @ 2015-04-20 8:33 ` Dan Carpenter 2015-04-20 10:42 ` gcc doesn't warn about uninitialized variable use in switch/case with -O (was: Re: [PATCH] Staging: dgnc: Using temporary value for repeated dereferences) Joe Perches -1 siblings, 1 reply; 15+ messages in thread From: Dan Carpenter @ 2015-04-20 8:33 UTC (permalink / raw) To: Joe Perches Cc: Yorick Rommers, devel, lidza.louina, driverdev-devel, linux-kernel, gregkh On Sun, Apr 19, 2015 at 05:54:19PM -0700, Joe Perches wrote: > > diff --git a/drivers/staging/dgnc/dgnc_mgmt.c b/drivers/staging/dgnc/dgnc_mgmt.c > > index b13318a..0437117 100644 > > --- a/drivers/staging/dgnc/dgnc_mgmt.c > > +++ b/drivers/staging/dgnc/dgnc_mgmt.c > > @@ -131,6 +131,7 @@ long dgnc_mgmt_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > > int brd; > > > > struct digi_info di; > > + struct dgnc_board *bd = dgnc_Board[brd]; > > Please read the code. > brd is got from userspace and you've dereferenced > it before getting the value from userspace. Gar. Oops. I totally missed that when I looked at this patch. :( You would hope that GCC would warn about the uninitialized variable but it doesn't. regards, dan carpenter ^ permalink raw reply [flat|nested] 15+ messages in thread
* gcc doesn't warn about uninitialized variable use in switch/case with -O (was: Re: [PATCH] Staging: dgnc: Using temporary value for repeated dereferences) 2015-04-20 8:33 ` [PATCH] Staging: dgnc: Using " Dan Carpenter @ 2015-04-20 10:42 ` Joe Perches 2015-04-20 12:28 ` Joe Perches 0 siblings, 1 reply; 15+ messages in thread From: Joe Perches @ 2015-04-20 10:42 UTC (permalink / raw) To: Dan Carpenter Cc: Yorick Rommers, devel, lidza.louina, driverdev-devel, linux-kernel, gregkh On Mon, 2015-04-20 at 11:33 +0300, Dan Carpenter wrote: > You would hope that GCC would warn about the uninitialized variable but > it doesn't. That's odd. So I filed this new gcc bugzilla: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65812) gcc 4.9.1 doesn't warn about uninitialized variable use declared in a switch/case statement when compiled with -O Here is a small example: $ cat t.c struct foo { int bar[100]; }; void foo_1(void) { unsigned int m; extern struct foo *array[100]; struct foo *a = array[m]; a = a; } void foo_2(void) { int i = 1; switch (i) { case 1: { unsigned int m; extern struct foo *array[100]; struct foo *a = array[m]; a = a; break; } } } $ gcc warns properly about both foo_1 and foo_2 without -O $ gcc -c -Wall t.c t.c: In function ‘foo_1’: t.c:9:14: warning: ‘m’ is used uninitialized in this function [-Wuninitialized] struct foo *a = array[m]; ^ t.c: In function ‘foo_2’: t.c:21:15: warning: ‘m’ may be used uninitialized in this function [-Wmaybe-uninitialized] struct foo *a = array[m]; ^ $ but gcc warns about foo_1 but not foo_2 with -O $ gcc -c -Wall -O t.c t.c: In function ‘foo_1’: t.c:9:14: warning: ‘m’ is used uninitialized in this function [-Wuninitialized] struct foo *a = array[m]; ^ $ gcc -v Using built-in specs. COLLECT_GCC=/usr/bin/gcc COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/4.9/lto-wrapper Target: x86_64-linux-gnu Configured with: ../src/configure -v --with-pkgversion='Ubuntu 4.9.1-16ubuntu6' --with-bugurl=file:///usr/share/doc/gcc-4.9/README.Bugs --enable-languages=c,c++,java,go,d,fortran,objc,obj-c++ --prefix=/usr --program-suffix=-4.9 --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --with-gxx-include-dir=/usr/include/c++/4.9 --libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --enable-gnu-unique-object --disable-vtable-verify --enable-plugin --with-system-zlib --disable-browser-plugin --enable-java-awt=gtk --enable-gtk-cairo --with-java-home=/usr/lib/jvm/java-1.5.0-gcj-4.9-amd64/jre --enable-java-home --with-jvm-root-dir=/usr/lib/jvm/java-1.5.0-gcj-4.9-amd64 --with-jvm-jar-dir=/usr/lib/jvm-exports/java-1.5.0-gcj-4.9-amd64 --with-arch-directory=amd64 --with-ecj-jar=/usr/share/java/eclipse-ecj.jar --enable-objc-gc --enable-multiarch --disable-werror --with-arch-32=i686 --with-abi=m64 --with-multilib-list=m32,m64,mx32 --enable-multilib --with-tune=generic --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu Thread model: posix gcc version 4.9.1 (Ubuntu 4.9.1-16ubuntu6) ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: gcc doesn't warn about uninitialized variable use in switch/case with -O (was: Re: [PATCH] Staging: dgnc: Using temporary value for repeated dereferences) 2015-04-20 10:42 ` gcc doesn't warn about uninitialized variable use in switch/case with -O (was: Re: [PATCH] Staging: dgnc: Using temporary value for repeated dereferences) Joe Perches @ 2015-04-20 12:28 ` Joe Perches 0 siblings, 0 replies; 15+ messages in thread From: Joe Perches @ 2015-04-20 12:28 UTC (permalink / raw) To: Dan Carpenter Cc: Yorick Rommers, devel, lidza.louina, driverdev-devel, linux-kernel, gregkh On Mon, 2015-04-20 at 03:42 -0700, Joe Perches wrote: > On Mon, 2015-04-20 at 11:33 +0300, Dan Carpenter wrote: > > You would hope that GCC would warn about the uninitialized variable but > > it doesn't. > > That's odd. So I filed this new gcc bugzilla: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65812) > > gcc 4.9.1 doesn't warn about uninitialized variable use > declared in a switch/case statement when compiled with -O Hey Dan/Yorick A bit more investigation: It's true that gcc doesn't report the uninitialized variable for the code I showed, but that's probably not the reason the "may be used uninitialized" warning is not shown for the dgnc code as patched by Yorick. The example code doesn't show the "uninitialized" warning because gcc's dead code elimination is set by -O and the sample code foo_2 function switch/case is eliminated. Perhaps the warning is not shown for this dgnc patch because your .config has CONFIG_GCOV_PROFILE_ALL set. That enables the gcc -fprofile-arcs option which masks the warning, I don't know why. Maybe see: commit 2521f2c228ad750701ba4702484e31d876dbc386 Author: Peter Oberparleiter <oberpar@linux.vnet.ibm.com> Date: Wed Jun 17 16:28:08 2009 -0700 gcov: add gcov profiling infrastructure Enable the use of GCC's coverage testing tool gcov [1] with the Linux kernel. gcov may be useful for: * debugging (has this code been reached at all?) * test improvement (how do I change my test to cover these lines?) * minimizing kernel configurations (do I need this option if the associated code is never run?) The profiling patch incorporates the following changes: * change kbuild to include profiling flags * provide functions needed by profiling code * present profiling data as files in debugfs Note that on some architectures, enabling gcc's profiling option "-fprofile-arcs" for the entire kernel may trigger compile/link/ run-time problems, some of which are caused by toolchain bugs and others which require adjustment of architecture code. For this reason profiling the entire kernel is initially restricted to those architectures for which it is known to work without changes. This restriction can be lifted once an architecture has been tested and found compatible with gcc's profiling. Profiling of single files or directories is still available on all platforms (see config help text). [1] http://gcc.gnu.org/onlinedocs/gcc/Gcov.html ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: gcc doesn't warn about uninitialized variable use in switch/case with -O (was: Re: [PATCH] Staging: dgnc: Using temporary value for repeated dereferences) @ 2015-04-20 12:28 ` Joe Perches 0 siblings, 0 replies; 15+ messages in thread From: Joe Perches @ 2015-04-20 12:28 UTC (permalink / raw) To: Dan Carpenter Cc: devel, gregkh, driverdev-devel, linux-kernel, lidza.louina, Yorick Rommers On Mon, 2015-04-20 at 03:42 -0700, Joe Perches wrote: > On Mon, 2015-04-20 at 11:33 +0300, Dan Carpenter wrote: > > You would hope that GCC would warn about the uninitialized variable but > > it doesn't. > > That's odd. So I filed this new gcc bugzilla: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65812) > > gcc 4.9.1 doesn't warn about uninitialized variable use > declared in a switch/case statement when compiled with -O Hey Dan/Yorick A bit more investigation: It's true that gcc doesn't report the uninitialized variable for the code I showed, but that's probably not the reason the "may be used uninitialized" warning is not shown for the dgnc code as patched by Yorick. The example code doesn't show the "uninitialized" warning because gcc's dead code elimination is set by -O and the sample code foo_2 function switch/case is eliminated. Perhaps the warning is not shown for this dgnc patch because your .config has CONFIG_GCOV_PROFILE_ALL set. That enables the gcc -fprofile-arcs option which masks the warning, I don't know why. Maybe see: commit 2521f2c228ad750701ba4702484e31d876dbc386 Author: Peter Oberparleiter <oberpar@linux.vnet.ibm.com> Date: Wed Jun 17 16:28:08 2009 -0700 gcov: add gcov profiling infrastructure Enable the use of GCC's coverage testing tool gcov [1] with the Linux kernel. gcov may be useful for: * debugging (has this code been reached at all?) * test improvement (how do I change my test to cover these lines?) * minimizing kernel configurations (do I need this option if the associated code is never run?) The profiling patch incorporates the following changes: * change kbuild to include profiling flags * provide functions needed by profiling code * present profiling data as files in debugfs Note that on some architectures, enabling gcc's profiling option "-fprofile-arcs" for the entire kernel may trigger compile/link/ run-time problems, some of which are caused by toolchain bugs and others which require adjustment of architecture code. For this reason profiling the entire kernel is initially restricted to those architectures for which it is known to work without changes. This restriction can be lifted once an architecture has been tested and found compatible with gcc's profiling. Profiling of single files or directories is still available on all platforms (see config help text). [1] http://gcc.gnu.org/onlinedocs/gcc/Gcov.html _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Staging: dgnc: Using temporary value for repeated dereferences. 2015-04-19 21:58 ` [PATCH] Staging: dgnc: Using temporary value for repeated dereferences Yorick Rommers 2015-04-20 0:54 ` Joe Perches @ 2015-04-20 8:27 ` Dan Carpenter 1 sibling, 0 replies; 15+ messages in thread From: Dan Carpenter @ 2015-04-20 8:27 UTC (permalink / raw) To: Yorick Rommers Cc: joe, devel, lidza.louina, driverdev-devel, linux-kernel, gregkh On Sun, Apr 19, 2015 at 11:58:53PM +0200, Yorick Rommers wrote: > Hello Joe, > > Thank you once again for the feedback. > I've changed my patch accordingly, see below. > > -------------------------------------- The patch is nice except for this chunk. And also you should be putting version numbers in the patch description: [PATCH v4] Staging: dgnc: Using temporary variable to save space > > A patch for a line being too long (>80) in dgnc_mgmt.c, > fixed by making a temporary value for dgnc_Board[brd], > replacing all instanced of dgnc_Board[brd] with temporary value, > and removing unnecessary typecasts. > > Signed-off-by: Yorick Rommers <yorick-rommers@hotmail.com> > --- And put a note under the cut off here. --- v4: Removed email to Joe Perches from the patch description regards, dan carpenter ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Staging: dgnc: fixed code style issue 2015-04-19 19:18 [PATCH] Staging: dgnc: fixed code style issue Yorick Rommers 2015-04-19 20:34 ` Joe Perches @ 2015-04-20 8:13 ` Dan Carpenter 1 sibling, 0 replies; 15+ messages in thread From: Dan Carpenter @ 2015-04-20 8:13 UTC (permalink / raw) To: Yorick Rommers Cc: lidza.louina, markh, gregkh, devel, driverdev-devel, linux-kernel On Sun, Apr 19, 2015 at 09:18:23PM +0200, Yorick Rommers wrote: > A patch for a line being too long (>80) in dgnc_mgmt.c, > fixed by making a temporary value for dgnc_Board[brd], and removing > an unnecessary typecast. > > Signed-off-by: Yorick Rommers <yorick-rommers@hotmail.com> > --- > drivers/staging/dgnc/dgnc_mgmt.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/staging/dgnc/dgnc_mgmt.c b/drivers/staging/dgnc/dgnc_mgmt.c > index b13318a..0e75d37 100644 > --- a/drivers/staging/dgnc/dgnc_mgmt.c > +++ b/drivers/staging/dgnc/dgnc_mgmt.c > @@ -143,12 +143,14 @@ long dgnc_mgmt_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > di.info_bdnum = brd; > > spin_lock_irqsave(&dgnc_Board[brd]->bd_lock, flags); > + struct dgnc_board *bd = dgnc_Board[brd]; Change the others as well. > > di.info_bdtype = dgnc_Board[brd]->dpatype; di.info_bdtype = bd->dpatype; > di.info_bdstate = dgnc_Board[brd]->dpastatus; di.info_bdstate = bd->dpastatus; > di.info_ioport = 0; > di.info_physaddr = (ulong) dgnc_Board[brd]->membase; > - di.info_physsize = (ulong) dgnc_Board[brd]->membase - dgnc_Board[brd]->membase_end; > + di.info_physsize = bd->membase - bd->membase_end; > + > if (dgnc_Board[brd]->state != BOARD_FAILED) > di.info_nports = dgnc_Board[brd]->nasync; ^^^^^^^^^^^^^^^ Etc. regards, dan carpenter ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2015-05-03 19:04 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-04-19 19:18 [PATCH] Staging: dgnc: fixed code style issue Yorick Rommers 2015-04-19 20:34 ` Joe Perches 2015-04-19 21:58 ` [PATCH] Staging: dgnc: Using temporary value for repeated dereferences Yorick Rommers 2015-04-20 0:54 ` Joe Perches 2015-04-20 0:54 ` Joe Perches 2015-04-20 7:33 ` [PATCH] Staging: dgnc: Using a " Yorick Rommers 2015-04-20 7:33 ` Yorick Rommers 2015-05-03 18:50 ` Greg KH 2015-05-03 18:50 ` Greg KH 2015-04-20 8:33 ` [PATCH] Staging: dgnc: Using " Dan Carpenter 2015-04-20 10:42 ` gcc doesn't warn about uninitialized variable use in switch/case with -O (was: Re: [PATCH] Staging: dgnc: Using temporary value for repeated dereferences) Joe Perches 2015-04-20 12:28 ` Joe Perches 2015-04-20 12:28 ` Joe Perches 2015-04-20 8:27 ` [PATCH] Staging: dgnc: Using temporary value for repeated dereferences Dan Carpenter 2015-04-20 8:13 ` [PATCH] Staging: dgnc: fixed code style issue Dan Carpenter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.