From patchwork Thu Jan 23 15:15:38 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Yann Droneaud X-Patchwork-Id: 3529641 Return-Path: X-Original-To: patchwork-linux-rdma@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 217A2C02DC for ; Thu, 23 Jan 2014 15:16:15 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id CE23A2016C for ; Thu, 23 Jan 2014 15:16:12 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id AB89D20108 for ; Thu, 23 Jan 2014 15:16:11 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753359AbaAWPQK (ORCPT ); Thu, 23 Jan 2014 10:16:10 -0500 Received: from smtpfb2-g21.free.fr ([212.27.42.10]:44821 "EHLO smtpfb2-g21.free.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751169AbaAWPQI (ORCPT ); Thu, 23 Jan 2014 10:16:08 -0500 Received: from smtp5-g21.free.fr (smtp5-g21.free.fr [212.27.42.5]) by smtpfb2-g21.free.fr (Postfix) with ESMTP id 3102DCA8B45 for ; Thu, 23 Jan 2014 16:15:57 +0100 (CET) Received: from [192.168.20.20] (unknown [37.161.76.231]) by smtp5-g21.free.fr (Postfix) with ESMTP id E28CFD4812E; Thu, 23 Jan 2014 16:15:39 +0100 (CET) Message-ID: <1390490138.11718.23.camel@localhost.localdomain> Subject: Re: [PATCH for-next V1 1/2] IB/core: Fix build warnings From: Yann Droneaud To: Or Gerlitz Cc: roland@kernel.org, linux-rdma@vger.kernel.org Date: Thu, 23 Jan 2014 16:15:38 +0100 In-Reply-To: <52E10B12.2080504@mellanox.com> References: <1383466844-8805-1-git-send-email-ogerlitz@mellanox.com> <1383466844-8805-2-git-send-email-ogerlitz@mellanox.com> <1390470420.9865.1.camel@localhost.localdomain> <52E10B12.2080504@mellanox.com> Organization: OPTEYA X-Mailer: Evolution 3.10.3 (3.10.3-1.fc20) Mime-Version: 1.0 Sender: linux-rdma-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-rdma@vger.kernel.org X-Spam-Status: No, score=-7.5 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Hi Or, Le jeudi 23 janvier 2014 à 14:29 +0200, Or Gerlitz a écrit : > On 23/01/2014 11:47, Yann Droneaud wrote: > > Le dimanche 03 novembre 2013 à 10:20 +0200, Or Gerlitz a écrit : > >> >Fix the below few "make W=1" build warnings we have on the IB core. > >> > > >> >drivers/infiniband/core/sysfs.c: In function ‘state_show’: > >> >drivers/infiniband/core/sysfs.c:107: warning: comparison of unsigned expression >= 0 is always true > >> >drivers/infiniband/core/verbs.c: In function ‘ib_modify_qp_is_ok’: > >> >drivers/infiniband/core/verbs.c:783: warning: comparison of unsigned expression < 0 is always false > >> >drivers/infiniband/core/verbs.c:784: warning: comparison of unsigned expression < 0 is always false > >> >drivers/infiniband/core/iwcm.c: In function ‘destroy_cm_id’: > >> >drivers/infiniband/core/iwcm.c:330: warning: variable ‘ret’ set but not used > >> > > >> >Signed-off-by: Or Gerlitz > > Reviewed-by: Yann Droneaud > > > > PS: Perhaps you could split the patch in two parts: one to remove the > > unused variable, and another to remove the check on unsigned variables ? > Roland decided not to take the unsigned expression < 0 patches, writing > to me > > "I applied the unused variable fix, but the others seem not needed, > because the latest kernel seems to include -Wno-sign-compare." > > what's your thinking? > I think Roland should write to the list instead of answering privately, but you're probably not asking my opinion about this matter. Regarding the warnings, I've double check GCC behavor. AFAICT it's not related to -Wno-sign-compare enabled or not, but some subtle issue. The actual option to trigger the warning is -Wtype-limits, and it's implied with -Wextra. And -Wextra is enabled with make W=1. A simple test case such as the one below will trigger the warning: int f(unsigned int ui) { if (ui < 0) return -1; return 0; } Tested with GCC 4.8.2 under Fedora 20, compiled for x86_64, cross-compiled for ARM and Power (ppc64), it produces the warning: $ gcc -Wtype-limits -c warning-simple.c warning-simple.c: In function 'f': warning-simple.c:3:3: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits] if (ui < 0) ^ The same will happen with a more elaborate condition: $ diff -u1 warning-simple.c warning-elaborate.c $ gcc -Wtype-limits -c warning-elaborate.c warning-elaborate.c: In function 'f': warning-elaborate.c:3:3: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits] if (ui < 0 || ui > 1000) ^ But as noted by Roland, the error is not shown when compiling the kernel InfiniBand modules. Which might sound surprising since the code your patch was about looks really like my simplified test case. One must note that your patch is about fixing comparaison of enum type against 0. So, let's try with the following test case which replaces unsigned int with an enum: enum e { E0, E1, }; int f(enum e ee) { if (ee < 0) return -1; if (ee > 1000) return -1; return 0; } Then no warning is reported while compiling this testcase with GCC 4.8 under Fedora 20, compiled for x86_64, cross-compiled for ARM and Power (ppc64). Indeed, an enum is mostly like an int. That explains why Roland and I weren't able to notice the warning when compiling with W=1. To trigger the warning, because it can triggered, one should built for an ABI that mandate either unsigned enums or short enums. For those who want to try this other side of the ABI world, short enums can be enabled for free with -fshort-enums (use with care). In this case multiple warnings are reported: $ gcc -fshort-enums -Wtype-limits -c warning-enum.c warning-enum.c: In function 'f': warning-enum.c:8:3: warning: comparison is always false due to limited range of data type [-Wtype-limits] if (ee < 0) { ^ warning-enum.c:11:3: warning: comparison is always false due to limited range of data type [-Wtype-limits] if (ee > 1000) { ^ (For C++, have a look at -fstrict-enums). To conclude, I think you will have to give some details on your build environment so that we could reproduce the warning. In the mean time, Roland was right to apply on the subset of your patch that remove the unused variable. Regards. --- warning-simple.c +++ warning-elaborate.c @@ -2,3 +2,3 @@ { - if (ui < 0) + if (ui < 0 || ui > 1000) return -1;