Re: [git patches 1/2] warnings: attack valid cases spotted by warnings
On Tue, Jul 17, 2007 at 05:42:39PM -0400, Jeff Garzik wrote: commit ae97fec3701a559929c3529e35417fab133a4d39 Author: Jeff Garzik [EMAIL PROTECTED] Date: Tue Jul 17 01:08:29 2007 -0400 drivers/usb/misc/auerswald: fix status check, remove redundant check 1) We should only set 'actual_length' output variable if usb length is known to be good. 2) No need to check actual_length for NULL. The only caller always passes non-NULL value. Signed-off-by: Jeff Garzik [EMAIL PROTECTED] I have no objection to this patch at all, however it does not remove the compiler warning :( thanks, greg k-h - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [git patches 1/2] warnings: attack valid cases spotted by warnings
Hey, I appreciate it, but I really do have to warn you that I did this all
blind, and just meant for it to be a I think this kind of direction is
more productive thing. I'm not going to guarantee that it works at all.
Oh, understood, and I'm definitely planning on taking your patch as
inspiration and a kick in the pants to finally clean up the code
rather than trying to use it as is. Obviously it was a quick hack
from someone that doesn't know the hardware -- for example
handle_raddr_seg() is at least misnamed, since UD work requests don't
have raddr segments.
But if you're going to take the time to work on the code, then I feel
like it's only polite not to drop your work on the floor. Just like I
hope someone who works on IB and who is usually sending me stuff to
merge would give a patch from me some attention.
BTW, I noticed one interesting thing while starting on this cleanup.
I wanted to make sure that the generated code didn't change with the
first step, and I actually discovered that the patch below seems to
make the generated code *better*, maybe because some gcc alias
analysis doesn't get as paranoid without the void *:
add/remove: 0/0 grow/shrink: 0/7 up/down: 0/-85 (-85)
function old new delta
mthca_arbel_post_srq_recv373 369 -4
mthca_arbel_post_receive 570 562 -8
mthca_tavor_post_srq_recv520 508 -12
mthca_tavor_post_send 13441330 -14
mthca_arbel_post_send 14811467 -14
mlx4_ib_post_send 15981582 -16
mthca_tavor_post_receive 792 775 -17
And here's the patch itself -- I think this is a reasonable size step
to break things up into. I assume that this is the basic form of the
cleanup that you're proposing?
diff --git a/drivers/infiniband/hw/mlx4/qp.c b/drivers/infiniband/hw/mlx4/qp.c
index 8d09aa3..a59c7f0 100644
--- a/drivers/infiniband/hw/mlx4/qp.c
+++ b/drivers/infiniband/hw/mlx4/qp.c
@@ -1183,6 +1183,14 @@ static int mlx4_wq_overflow(struct mlx4_ib_wq *wq, int
nreq, struct ib_cq *ib_cq
return cur + nreq = wq-max_post;
}
+static void set_data_seg(struct mlx4_wqe_data_seg *dseg,
+struct ib_sge *sg)
+{
+ dseg-byte_count = cpu_to_be32(sg-length);
+ dseg-lkey = cpu_to_be32(sg-lkey);
+ dseg-addr = cpu_to_be64(sg-addr);
+}
+
int mlx4_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
struct ib_send_wr **bad_wr)
{
@@ -1313,12 +1321,7 @@ int mlx4_ib_post_send(struct ib_qp *ibqp, struct
ib_send_wr *wr,
}
for (i = 0; i wr-num_sge; ++i) {
- ((struct mlx4_wqe_data_seg *) wqe)-byte_count =
- cpu_to_be32(wr-sg_list[i].length);
- ((struct mlx4_wqe_data_seg *) wqe)-lkey =
- cpu_to_be32(wr-sg_list[i].lkey);
- ((struct mlx4_wqe_data_seg *) wqe)-addr =
- cpu_to_be64(wr-sg_list[i].addr);
+ set_data_seg(wqe, wr-sg_list + i);
wqe += sizeof (struct mlx4_wqe_data_seg);
size += sizeof (struct mlx4_wqe_data_seg) / 16;
diff --git a/drivers/infiniband/hw/mthca/mthca_qp.c
b/drivers/infiniband/hw/mthca/mthca_qp.c
index 0e9ef24..2548250 100644
--- a/drivers/infiniband/hw/mthca/mthca_qp.c
+++ b/drivers/infiniband/hw/mthca/mthca_qp.c
@@ -1740,13 +1740,8 @@ int mthca_tavor_post_send(struct ib_qp *ibqp, struct
ib_send_wr *wr,
}
for (i = 0; i wr-num_sge; ++i) {
- ((struct mthca_data_seg *) wqe)-byte_count =
- cpu_to_be32(wr-sg_list[i].length);
- ((struct mthca_data_seg *) wqe)-lkey =
- cpu_to_be32(wr-sg_list[i].lkey);
- ((struct mthca_data_seg *) wqe)-addr =
- cpu_to_be64(wr-sg_list[i].addr);
- wqe += sizeof (struct mthca_data_seg);
+ mthca_set_data_seg(wqe, wr-sg_list + i);
+ wqe += sizeof (struct mthca_data_seg);
size += sizeof (struct mthca_data_seg) / 16;
}
@@ -1869,13 +1864,8 @@ int mthca_tavor_post_receive(struct ib_qp *ibqp, struct
ib_recv_wr *wr,
}
for (i = 0; i wr-num_sge; ++i) {
- ((struct mthca_data_seg *) wqe)-byte_count =
- cpu_to_be32(wr-sg_list[i].length);
- ((struct mthca_data_seg *) wqe)-lkey =
- cpu_to_be32(wr-sg_list[i].lkey);
- ((struct mthca_data_seg *) wqe)-addr =
- cpu_to_be64(wr-sg_list[i].addr);
-
Re: [git patches 1/2] warnings: attack valid cases spotted by warnings
On Wed, 18 Jul 2007, Roland Dreier wrote: BTW, I noticed one interesting thing while starting on this cleanup. I wanted to make sure that the generated code didn't change with the first step, and I actually discovered that the patch below seems to make the generated code *better*, maybe because some gcc alias analysis doesn't get as paranoid without the void *: Absolutely. The way to get pretty much any compiler to generate better code is: - code it simply - don't have tons of variables with overlapping lifetime - use limited-scope variables (ie don't have the variables at the outermost scope, declare them in the smallest scope you can) and trying to split things up into functions helps all of these. In fact, you can often get better code even when the functions aren't even inlined, because of the simpler code issue. Gcc sometimes tries to be too clever with its CSE etc, and generates really nasty complex code with lots of register spills, just because it keeps stuff live rather than just regenerating them. So inlining a function doesn't even make it faster, all the time. You want to inline when - the function is so small that the call is literally a big part of it, and it doesn't even need more than a couple of registers, so the calling convention has more register pressure than inlining the function itself would have. OR - the callers tend to have constant arguments that can benefit from constant folding inside the function but inlining in many other circumstances actually generates *worse* code and just makes debugging harder and the I$ footprint bigger. And here's the patch itself -- I think this is a reasonable size step to break things up into. I assume that this is the basic form of the cleanup that you're proposing? Yes, this looks good. Doing these kinds of things for the various other things is likely to make the code more readable, and as you already found out, the generated code doesn't tend to be worse either. It might not _always_ be a win size and performance-wise, but it can be, and so readability should generally always be the #1 goal, because quite often it actually does help the compiler too. Linus - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [git patches 1/2] warnings: attack valid cases spotted by warnings
Greg KH wrote: On Tue, Jul 17, 2007 at 05:42:39PM -0400, Jeff Garzik wrote: commit ae97fec3701a559929c3529e35417fab133a4d39 Author: Jeff Garzik [EMAIL PROTECTED] Date: Tue Jul 17 01:08:29 2007 -0400 drivers/usb/misc/auerswald: fix status check, remove redundant check 1) We should only set 'actual_length' output variable if usb length is known to be good. 2) No need to check actual_length for NULL. The only caller always passes non-NULL value. Signed-off-by: Jeff Garzik [EMAIL PROTECTED] I have no objection to this patch at all, however it does not remove the compiler warning :( The description doesn't say it removes a warning, so it doesn't :) You want to look at drivers/*: mark variables with uninitialized_var() which was in posting [git patches 2/2] warnings: use uninitialized_var() and is now upstream commit a6343afb6e16b65b9f0b264f94f8207212e7e3ae Jeff - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [git patches 1/2] warnings: attack valid cases spotted by warnings
On Wed, Jul 18, 2007 at 04:03:05PM -0400, Jeff Garzik wrote: Greg KH wrote: On Tue, Jul 17, 2007 at 05:42:39PM -0400, Jeff Garzik wrote: commit ae97fec3701a559929c3529e35417fab133a4d39 Author: Jeff Garzik [EMAIL PROTECTED] Date: Tue Jul 17 01:08:29 2007 -0400 drivers/usb/misc/auerswald: fix status check, remove redundant check 1) We should only set 'actual_length' output variable if usb length is known to be good. 2) No need to check actual_length for NULL. The only caller always passes non-NULL value. Signed-off-by: Jeff Garzik [EMAIL PROTECTED] I have no objection to this patch at all, however it does not remove the compiler warning :( The description doesn't say it removes a warning, so it doesn't :) You want to look at drivers/*: mark variables with uninitialized_var() which was in posting [git patches 2/2] warnings: use uninitialized_var() and is now upstream commit a6343afb6e16b65b9f0b264f94f8207212e7e3ae Ah, sorry, I just went for the first USB change entry, sorry about that :) And thanks for doing this, clean builds are good to have. greg k-h - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[git patches 1/2] warnings: attack valid cases spotted by warnings
Those may be used uninitialized warnings are annoying. So annoying that kernel developers tune them out, and that occasionally hides real bugs, as my past patches (and those below) indicate. I included the full-length changelog below the diffstat, because that is where the best explanation for each change is found. In most cases that's where all the length is -- a paragraph or two describing what is usually a one-line code change, usually an initialization or simple cleanup. This is the first of two git pull sets, the -parent-. If you pull 'warnings' branch, you get what you see below and nothing else. WYSIWYG. You'll see in the second email why I distinguish the two. Please pull from 'warnings' branch of master.kernel.org:/pub/scm/linux/kernel/git/jgarzik/misc-2.6.git warnings Jeff Garzik (11): kernel/auditfilter: kill bogus uninit'd-var compiler warning [netdrvr] natsemi: Fix device removal bug [netdrvr] eepro100, ne2k-pci: abort resume if pci_enable_device() fails drivers/usb/misc/auerswald: fix status check, remove redundant check drivers/net/wan/pc300_drv: fix bug caught by gcc warning drivers/telephony/ixj: cleanup and fix gcc warning drivers/mtd/ubi/eba: minor cleanup: tighten scope of a local var drivers/net/wan/sbni: kill uninit'd var warning drivers/infiniband/hw/mthca/mthca_qp: kill uninit'd var warning [libata] sata_mv: use pci_try_set_mwi() drivers/atm/ambassador: kill uninit'd var warning, and fix bug drivers/ata/sata_mv.c |2 +- drivers/atm/ambassador.c |4 +++- drivers/infiniband/hw/mthca/mthca_qp.c |4 ++-- drivers/mtd/ubi/eba.c |4 ++-- drivers/net/eepro100.c |7 ++- drivers/net/natsemi.c |2 +- drivers/net/ne2k-pci.c |7 ++- drivers/net/wan/pc300_drv.c|2 ++ drivers/net/wan/sbni.c |7 +++ drivers/telephony/ixj.c|7 ++- drivers/usb/misc/auerswald.c |2 +- kernel/auditfilter.c | 12 12 files changed, 37 insertions(+), 23 deletions(-) commit b1734d2388cc45ecdec58615e35955d0d402f938 Author: Jeff Garzik [EMAIL PROTECTED] Date: Tue Jul 17 02:32:21 2007 -0400 drivers/atm/ambassador: kill uninit'd var warning, and fix bug An uninitialized variable warning illuminated an area where indeed the variable was being used without initialization. Unfortunately, after verifying all such paths were fixed, the warning still appears. So we follow the initialization practice of other variables in this function. Signed-off-by: Jeff Garzik [EMAIL PROTECTED] commit ea8b4db97aa41a66c05daa4055a1974692ccd52d Author: Jeff Garzik [EMAIL PROTECTED] Date: Tue Jul 17 02:21:50 2007 -0400 [libata] sata_mv: use pci_try_set_mwi() Because sometimes in life, it's ok to fail. Signed-off-by: Jeff Garzik [EMAIL PROTECTED] commit 9db48926208562df3c778682e064990170ab8971 Author: Jeff Garzik [EMAIL PROTECTED] Date: Tue Jul 17 02:03:49 2007 -0400 drivers/infiniband/hw/mthca/mthca_qp: kill uninit'd var warning drivers/infiniband/hw/mthca/mthca_qp.c: In function ‘mthca_tavor_post_send’: drivers/infiniband/hw/mthca/mthca_qp.c:1594: warning: ‘f0’ may be used uninitialized in this function drivers/infiniband/hw/mthca/mthca_qp.c: In function ‘mthca_arbel_post_send’: drivers/infiniband/hw/mthca/mthca_qp.c:1949: warning: ‘f0’ may be used uninitialized in this function Initializing 'f0' is not strictly necessary in either case, AFAICS. I was considering use of uninitialized_var(), but looking at the complex flow of control in each function, I feel it is wiser and safer to simply zero the var and be certain of ourselves. Signed-off-by: Jeff Garzik [EMAIL PROTECTED] commit e5fb4f42268654ca41ab50b1406fb7da97559db5 Author: Jeff Garzik [EMAIL PROTECTED] Date: Tue Jul 17 01:56:32 2007 -0400 drivers/net/wan/sbni: kill uninit'd var warning It's actually convenient in the code to initialize this and a sister variable to zero. Signed-off-by: Jeff Garzik [EMAIL PROTECTED] commit 2ab934b8afa89b9b3e71b7fb66470a19772f5012 Author: Jeff Garzik [EMAIL PROTECTED] Date: Tue Jul 17 01:49:56 2007 -0400 drivers/mtd/ubi/eba: minor cleanup: tighten scope of a local var Signed-off-by: Jeff Garzik [EMAIL PROTECTED] commit 0d480db85dea59e1393c3968fbdac0117431e797 Author: Jeff Garzik [EMAIL PROTECTED] Date: Tue Jul 17 01:35:08 2007 -0400 drivers/telephony/ixj: cleanup and fix gcc warning 1) Fix gcc uninit'd var warnings by adding 'default' switch stmt labels in two cases. It was lightning-strikes unlikely that a problem would ever arise, but not impossible. 2) Tighten the scope of 'blankword' in two cases.
Re: [git patches 1/2] warnings: attack valid cases spotted by warnings
drivers/infiniband/hw/mthca/mthca_qp: kill uninit'd var warning drivers/infiniband/hw/mthca/mthca_qp.c: In function âmthca_tavor_post_sendâ: drivers/infiniband/hw/mthca/mthca_qp.c:1594: warning: âf0â may be used uninitialized in this function drivers/infiniband/hw/mthca/mthca_qp.c: In function âmthca_arbel_post_sendâ: drivers/infiniband/hw/mthca/mthca_qp.c:1949: warning: âf0â may be used uninitialized in this function Initializing 'f0' is not strictly necessary in either case, AFAICS. I was considering use of uninitialized_var(), but looking at the complex flow of control in each function, I feel it is wiser and safer to simply zero the var and be certain of ourselves. Signed-off-by: Jeff Garzik [EMAIL PROTECTED] I don't really like this. These functions are in the hottest, most latency-sensitive code path of this driver, which is used by people who care about nanoseconds. I'm quite confident that the code is correct as written, and it really feels wrong to me to add bloat to the fastpath just to cover up a shortcoming of gcc. And I don't like using uninitialized_var() here for a similar reason... the functions are complex and I would prefer to avoid hiding future bugs that may creep in. In fact adding the initialization to 0 has a similar effect, since it shuts up the compiler even if the logic in the function gets screwed up. On the other hand I definitely sympathize with the desire to have a warning-free build so that real warnings are more visible. I guess I could live with the uninitialized_var() patch, although I would feel a little sad. - R. - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [git patches 1/2] warnings: attack valid cases spotted by warnings
Roland Dreier wrote: drivers/infiniband/hw/mthca/mthca_qp: kill uninit'd var warning drivers/infiniband/hw/mthca/mthca_qp.c: In function ‘mthca_tavor_post_send’: drivers/infiniband/hw/mthca/mthca_qp.c:1594: warning: ‘f0’ may be used uninitialized in this function drivers/infiniband/hw/mthca/mthca_qp.c: In function ‘mthca_arbel_post_send’: drivers/infiniband/hw/mthca/mthca_qp.c:1949: warning: ‘f0’ may be used uninitialized in this function Initializing 'f0' is not strictly necessary in either case, AFAICS. I was considering use of uninitialized_var(), but looking at the complex flow of control in each function, I feel it is wiser and safer to simply zero the var and be certain of ourselves. Signed-off-by: Jeff Garzik [EMAIL PROTECTED] I don't really like this. These functions are in the hottest, most latency-sensitive code path of this driver, which is used by people who care about nanoseconds. I'm quite confident that the code is correct as written, and it really feels wrong to me to add bloat to the fastpath just to cover up a shortcoming of gcc. I don't buy that performance argument, in this case. You are already dirtying the same cacheline with other variable initializations. Like I noted in the changeset description (hey, this is precisely why I included it, so that we could have this discussion), IMO the flow of control makes it not only impossible for the compiler to understand the full value range of 'f0', but also difficult for humans as well. I could perhaps understand initializing the variable to some poison value rather than zero, but IMO the code is stronger with f0 set to a sane value. It's poorly readable, poorly commented code as-is. Jeff - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [git patches 1/2] warnings: attack valid cases spotted by warnings
Jeff Garzik wrote: I don't buy that performance argument, in this case. You are already dirtying the same cacheline with other variable initializations. Or simply sitting in a CPU register for large stretches of function runtime... Jeff - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [git patches 1/2] warnings: attack valid cases spotted by warnings
On Tue, 17 Jul 2007 14:53:02 -0700 Roland Dreier [EMAIL PROTECTED] wrote: drivers/infiniband/hw/mthca/mthca_qp: kill uninit'd var warning drivers/infiniband/hw/mthca/mthca_qp.c: In function __mthca_tavor_post_send__: drivers/infiniband/hw/mthca/mthca_qp.c:1594: warning: __f0__ may be used uninitialized in this function (rofl, look at that mess: it was utterly impractical, unrealistic and stupid for gcc to go and UTFify the compiler output. Sigh. LANG=C, guys). And I don't like using uninitialized_var() here for a similar reason... the functions are complex and I would prefer to avoid hiding future bugs that may creep in. umm, the code *already* produces a warning. So if you later add a real used-uninitialised bug, you won't know about it, because everyone was trained to ignore the warning anyway. Best would be to find some way to restructure the code to make the warning go away. Meanwhile I'd say switch this to uninitialized_var() if it's that much of a worry. - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [git patches 1/2] warnings: attack valid cases spotted by warnings
On Tue, 17 Jul 2007, Andrew Morton wrote:
(rofl, look at that mess: it was utterly impractical, unrealistic and
stupid for gcc to go and UTFify the compiler output. Sigh. LANG=C, guys).
Yeah, I absolutely *detest* how gcc does idiotic quoting just because you
happen to be in a UTF-8 locale. There's no reason for it what-so-ever, and
I don't understand the mindset of whoever did that crap.
They should use a nice ASCII tick-mark ('), not some fancy quotation
marks.
Linus
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [git patches 1/2] warnings: attack valid cases spotted by warnings
I don't buy that performance argument, in this case. You are already dirtying the same cacheline with other variable initializations. Like I noted in the changeset description (hey, this is precisely why I included it, so that we could have this discussion), IMO the flow of control makes it not only impossible for the compiler to understand the full value range of 'f0', but also difficult for humans as well. I could perhaps understand initializing the variable to some poison value rather than zero, but IMO the code is stronger with f0 set to a sane value. The more I think about it, the less sense initializing f0 to 0 makes. The whole problem with an uninitialized variable is that a random value from the stack might be used. So setting a variable to something meaningless (guaranteeing that a garbage value is used in case of a bug) just to shut up a warning makes no sense -- it's no safer than leaving the code as is. uninitialized_var() gets rid of the warning, saves a little text and instruction cache, and documents things better. (BTW, I agree the code is a little confusing as written. I think things could be cleaned up and made more efficient by getting rid of the initialization of size0 too -- I'll look at doing that) Anyway, I queued this up for my next merge with Linus: commit 6d7d080e9f7cd535a8821efd3835c5cfa5223ab6 Author: Roland Dreier [EMAIL PROTECTED] Date: Tue Jul 17 19:30:51 2007 -0700 IB/mthca: Use uninitialized_var() for f0 Commit 9db48926 (drivers/infiniband/hw/mthca/mthca_qp: kill uninit'd var warning) added = 0 to the declarations of f0 to shut up gcc warnings. However, there's no point in making the code bigger by initializing f0 to a random value just to get rid of a warning; setting f0 to 0 is no safer than just using uninitialized_var(), which documents the situation better and gives smaller code too. For example, on x86_64: add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-16 (-16) function old new delta mthca_tavor_post_send 13521344 -8 mthca_arbel_post_send 14891481 -8 Signed-off-by: Roland Dreier [EMAIL PROTECTED] diff --git a/drivers/infiniband/hw/mthca/mthca_qp.c b/drivers/infiniband/hw/mthca/mthca_qp.c index 11f1d99..0e9ef24 100644 --- a/drivers/infiniband/hw/mthca/mthca_qp.c +++ b/drivers/infiniband/hw/mthca/mthca_qp.c @@ -1591,7 +1591,13 @@ int mthca_tavor_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr, int i; int size; int size0 = 0; - u32 f0 = 0; + /* +* f0 is only used if nreq != 0, and f0 will be initialized +* the first time through the main loop, since size0 == 0 the +* first time through. So nreq cannot become non-zero without +* initializing f0, and f0 is in fact never used uninitialized. +*/ + u32 uninitialized_var(f0); int ind; u8 op0 = 0; @@ -1946,7 +1952,13 @@ int mthca_arbel_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr, int i; int size; int size0 = 0; - u32 f0 = 0; + /* +* f0 is only used if nreq != 0, and f0 will be initialized +* the first time through the main loop, since size0 == 0 the +* first time through. So nreq cannot become non-zero without +* initializing f0, and f0 is in fact never used uninitialized. +*/ + u32 uninitialized_var(f0); int ind; u8 op0 = 0; - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [git patches 1/2] warnings: attack valid cases spotted by warnings
On Tue, 17 Jul 2007, Roland Dreier wrote: So setting a variable to something meaningless (guaranteeing that a garbage value is used in case of a bug) just to shut up a warning makes no sense -- it's no safer than leaving the code as is. Wrong. It's safer for two reasons: - now everybody will see the *same* behaviour - the meaningless value is guaranteed to not be a security leak but the whole shut up bogus warnings is the best reason. So it *is* safer than leaving the code as-is. Of course, usually the best approach is to rewrite the code to be simpler, so that even gcc sees that something is obviously initialized. Sadly, people seldom do the right thing, and sometimes gcc just blows incredibly hard. Linus - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [git patches 1/2] warnings: attack valid cases spotted by warnings
So setting a variable to something meaningless (guaranteeing that a
garbage value is used in case of a bug) just to shut up a warning makes
no sense -- it's no safer than leaving the code as is.
Wrong.
It's safer for two reasons:
- now everybody will see the *same* behaviour
- the meaningless value is guaranteed to not be a security leak
but the whole shut up bogus warnings is the best reason.
So it *is* safer than leaving the code as-is.
OK, fair enough. What I said wasn't quite right, but in my case I
think neither of your reasons really applies, since the uninitialized
variable would be written into some hardware control block, so the
effect would probably still be random even if the value is the same
and the information leak doesn't really matter.
Anyway, I think that in this case it's not too hard to show that the
variable really can't be used uninitialized, so I prefer the smaller
generated code from uninitialized_var() (plus a comment explaining why
that's safe).
Of course, usually the best approach is to rewrite the code to be simpler,
so that even gcc sees that something is obviously initialized. Sadly,
people seldom do the right thing, and sometimes gcc just blows incredibly
hard.
In this case the code is basically
u32 x;
for (n = 0; cond; ++n) {
...
if (!n)
x = something;
...
}
if (n) {
...
use(x);
...
}
and gcc still warns...
- R.
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [git patches 1/2] warnings: attack valid cases spotted by warnings
Roland Dreier wrote:
In this case the code is basically
u32 x;
for (n = 0; cond; ++n) {
...
if (!n)
x = something;
...
}
if (n) {
...
use(x);
...
}
and gcc still warns...
Interestingly, the above accurately describes a common code pattern
matching code which caused gcc to emit the uninit'd-var warnings.
For the record I think initializating 'f0' to zero is safer for the
reasons Linus gave, and in addition, f0 is or'd with a value written to
a hardware register, which means things should go awry (if they go) in a
semi-predictable manner.
According to the assembly language produced, sure it is larger -- by one
(per function) MOV that is adjacent to other initializations, making it
highly likely the initializations are all streamed together. I doubt
one MOV per function will make a huge difference, considering the peace
of mind it buys.
Jeff
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [git patches 1/2] warnings: attack valid cases spotted by warnings
On Tue, 17 Jul 2007, Roland Dreier wrote:
I think this patch (on top of the previous one) actually makes the
code clearer
Quite frankly, calling this making the code clearer is a bit ridiculous.
That code still is absolute *crap* from a readability angle. It doesn't
follow any sane coding standards, and certainly not the most important
ones (keep the function small, don't have tons of local variables),
and it has absolutely ridiculously ugly casts that get repeated over and
over and over again.
Quite frankly, I don't quite understand where you get those enormous balls
you have, that you can then talk about how ugly it is to just add a = 0
that shuts up a compiler warning. That's the _least_ ugly part of the
whole damn function!
So rather than sending out that idiotic patch, look at that code for five
seconds, and ponder whether it really needs to be that ugly.
Here's a few things that you could *really* do to make it somewhat more
readable:
- make that whole switch() statement from hell another function
entirely, and have it return the size of the thing, so that you don't
need to have
wqe += xxx
size += xxx / 16;
repeated fifty times (and so that it's also obvious that the
always matches).
- make each switch case actually call a small function with the argument
cast to the right pointer type, so that you need *one* cast per case,
rather than a handful.
End result? More readable source code, with functions that are 20 lines
long (or less), rather than 200 lines of spagetti-coding.
And you know what? That's actually more important than 16 bytes of object
code, although looking at the size of the infiniband code, I *seriously*
doubt any infiniband person has ever cared about object code size in their
life. That thing is not for weak machines or stomachs.
The warnign (and fixing it up) is the _least_ of the problems in that
code, methinks.
Anyway, here's a totally untested cleanup that compiles but probably
doesn't work, because I didn't check that I did the right thing with all
the pointer arithmetic (ie when I change wqe to a real structure pointer
instead of just a void *, maybe I left some pointer arithmetic around
that expected it to work as a byte pointer, but now really works on the
whole structure size instead).
So this patch is NOT meant to be applied, but it is meant to teach people
how things like this should be done. They should *not* be one big function
with lots of case statements. They should be lots of small functions!
Linus
---
drivers/infiniband/hw/mthca/mthca_qp.c | 236 ---
1 files changed, 122 insertions(+), 114 deletions(-)
diff --git a/drivers/infiniband/hw/mthca/mthca_qp.c
b/drivers/infiniband/hw/mthca/mthca_qp.c
index 11f1d99..74da9bc 100644
--- a/drivers/infiniband/hw/mthca/mthca_qp.c
+++ b/drivers/infiniband/hw/mthca/mthca_qp.c
@@ -1578,6 +1578,113 @@ static inline int mthca_wq_overflow(struct mthca_wq
*wq, int nreq,
return cur + nreq = wq-max;
}
+static int handle_next_seg(struct ib_send_wr *wr, struct mthca_next_seg * wqe)
+{
+ wqe-nda_op = 0;
+ wqe-ee_nds = 0;
+ wqe-flags =((wr-send_flags IB_SEND_SIGNALED) ?
+cpu_to_be32(MTHCA_NEXT_CQ_UPDATE) : 0) |
+ ((wr-send_flags IB_SEND_SOLICITED) ?
+cpu_to_be32(MTHCA_NEXT_SOLICIT) : 0) |
+ cpu_to_be32(1);
+
+ if (wr-opcode == IB_WR_SEND_WITH_IMM ||
+ wr-opcode == IB_WR_RDMA_WRITE_WITH_IMM)
+ wqe-imm = wr-imm_data;
+
+ return sizeof(struct mthca_next_seg);
+}
+
+static int handle_raddr_seg(struct mthca_dev *dev, struct mthca_qp *qp, struct
ib_send_wr *wr,
+ struct mthca_raddr_seg *wqe, int ind)
+{
+ switch (qp-transport) {
+ case RC:
+ switch (wr-opcode) {
+ case IB_WR_ATOMIC_CMP_AND_SWP:
+ case IB_WR_ATOMIC_FETCH_AND_ADD: {
+ struct mthca_atomic_seg *atomic;
+
+ wqe-raddr = cpu_to_be64(wr-wr.atomic.remote_addr);
+ wqe-rkey = cpu_to_be32(wr-wr.atomic.rkey);
+ wqe-reserved = 0;
+
+ atomic = (struct mthca_atomic_seg *) (wqe+1);
+
+ if (wr-opcode == IB_WR_ATOMIC_CMP_AND_SWP) {
+ atomic-swap_add =
cpu_to_be64(wr-wr.atomic.swap);
+ atomic-compare =
cpu_to_be64(wr-wr.atomic.compare_add);
+ } else {
+ atomic-swap_add =
cpu_to_be64(wr-wr.atomic.compare_add);
+ atomic-compare = 0;
+ }
+
+ return sizeof (struct mthca_raddr_seg) +
+ sizeof (struct mthca_atomic_seg);
+ }
+
+ case IB_WR_RDMA_WRITE:
+ case
Re: [git patches 1/2] warnings: attack valid cases spotted by warnings
Quite frankly, I don't quite understand where you get those enormous balls you have, that you can then talk about how ugly it is to just add a = 0 that shuts up a compiler warning. That's the _least_ ugly part of the whole damn function! The clanking when I walk annoys people in the office too... But you're right. It is stupid of me to make such a big deal about this. My excuse is that I've seen those warnings so many times and actually given them more thought than they deserve, and I really felt that Jeff's change makes the admittedly already ugly code a tiny little bit worse. Anyway, here's a totally untested cleanup that compiles but probably doesn't work, because I didn't check that I did the right thing with all the pointer arithmetic (ie when I change wqe to a real structure pointer instead of just a void *, maybe I left some pointer arithmetic around that expected it to work as a byte pointer, but now really works on the whole structure size instead). Given that you took the time to do this, I'll get the patch into a working state and apply it. And maybe split it into reviewable chunks while I'm at it ;) Thanks, Roland - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [git patches 1/2] warnings: attack valid cases spotted by warnings
On Tue, 17 Jul 2007, Roland Dreier wrote: Anyway, here's a totally untested cleanup that compiles but probably doesn't work, because I didn't check that I did the right thing with all the pointer arithmetic (ie when I change wqe to a real structure pointer instead of just a void *, maybe I left some pointer arithmetic around that expected it to work as a byte pointer, but now really works on the whole structure size instead). Given that you took the time to do this, I'll get the patch into a working state and apply it. And maybe split it into reviewable chunks while I'm at it ;) Hey, I appreciate it, but I really do have to warn you that I did this all blind, and just meant for it to be a I think this kind of direction is more productive thing. I'm not going to guarantee that it works at all. I spent more time than I really wanted to on actually making sure the end result even compiles (quite often, I'm perfectly happy to just send out pseudo-code to indicate what I think should be done), but maybe I shouldn't have done that, just so that nobody thinks that the patch is necessarily going to *work*. In other words, it's a totally mindless cleanup and re-factorization. It *may* work, but quite frankly, I did it just for that one email, and spent the absolutly minimum possible time thinking about it. As a result, I really think it needs some serious looking over. I also think that my new handle_raddr_seg() function could itself be split up into subfunctions along the switch() subcases. So not only wasn't it meant to be guaranteed correct, but it wasn't even meant to be seen as the best possible final situation: it could be - and should be - factored out even more (and the same goes for a lot of the other functions in that file that I didn't really look at, just glance and notice that they have some of the same problem patterns). Linus - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
