diff mbox

[dpdk-dev,v3,04/12] eal: allow empty compile time flags

Message ID 1436172698-21749-5-git-send-email-zlu@ezchip.com (mailing list archive)
State Changes Requested, archived
Headers

Commit Message

Zhigang Lu July 6, 2015, 8:51 a.m. UTC 
  The rte_cpu_check_supported() code breaks with a "comparison is always
false due to limited range of data type" when the compile_time_flags[]
array is empty.  Assigning the array dimension to a local variable
apparently solves this.

Change-Id: I0ae21f529cf7b6dd9cf0f4532dce9198f4bf4230
Signed-off-by: Zhigang Lu <zlu@ezchip.com>
---
 lib/librte_eal/common/eal_common_cpuflags.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Bruce Richardson July 6, 2015, 3:33 p.m. UTC | #1 
On Mon, Jul 06, 2015 at 04:51:29PM +0800, Zhigang Lu wrote:
> The rte_cpu_check_supported() code breaks with a "comparison is always
> false due to limited range of data type" when the compile_time_flags[]
> array is empty.  Assigning the array dimension to a local variable
> apparently solves this.
> 
> Change-Id: I0ae21f529cf7b6dd9cf0f4532dce9198f4bf4230
> Signed-off-by: Zhigang Lu <zlu@ezchip.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Thomas Monjalon July 9, 2015, 12:46 a.m. UTC | #2 
2015-07-06 16:51, Zhigang Lu:
> The rte_cpu_check_supported() code breaks with a "comparison is always
> false due to limited range of data type" when the compile_time_flags[]
> array is empty.  Assigning the array dimension to a local variable
> apparently solves this.

How is it related to the title "allow empty compile time flags"?

> -	unsigned i;
> +	unsigned count = RTE_DIM(compile_time_flags), i;
>  	int ret;

Please define a "const unsigned count =" here to improve readability.

> -	for (i = 0; i < sizeof(compile_time_flags)/sizeof(compile_time_flags[0]); i++) {
> +	for (i = 0; i < count; i++) {
Zhigang Lu July 9, 2015, 4:20 a.m. UTC | #3 
>-----Original Message-----
>From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
>Sent: Thursday, July 09, 2015 8:46 AM
>To: Zhigang Lu
>Cc: dev@dpdk.org
>Subject: Re: [dpdk-dev] [PATCH v3 04/12] eal: allow empty compile time
flags
>
>2015-07-06 16:51, Zhigang Lu:
>> The rte_cpu_check_supported() code breaks with a "comparison is always
>> false due to limited range of data type" when the compile_time_flags[]
>> array is empty.  Assigning the array dimension to a local variable
>> apparently solves this.
>
>How is it related to the title "allow empty compile time flags"?

Changed the title and commit log a little bit as follows.

    eal: allow empty compile time flags RTE_COMPILE_TIME_CPUFLAGS
    
    When RTE_COMPILE_TIME_CPUFLAGS is empty, the rte_cpu_check_supported()
    code breaks with a "comparison is always false due to limited range of
    data type".  This is because the compile_time_flags[] array is empty.
    Assigning the array dimension to a local variable apparently solves
this.

>
>> -	unsigned i;
>> +	unsigned count = RTE_DIM(compile_time_flags), i;
>>  	int ret;
>
>Please define a "const unsigned count =" here to improve readability.

If define as a const unsigned count, we also get the following compiler
warning.
error: comparison of unsigned expression < 0 is always false

>
>> -	for (i = 0; i <
sizeof(compile_time_flags)/sizeof(compile_time_flags[0]); i++)
>{
>> +	for (i = 0; i < count; i++) {
>
>
Bruce Liu July 9, 2015, 6:49 a.m. UTC | #4 
------------------ Original ------------------
From:  "Tony Lu";<zlu@ezchip.com>;
Date:  Jul 9, 2015
To:  "'Thomas Monjalon'"<thomas.monjalon@6wind.com>; 
Cc:  "dev"<dev@dpdk.org>; 
Subject:  Re: [dpdk-dev] [PATCH v3 04/12] eal: allow empty compile time flags



>-----Original Message-----
>From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
>Sent: Thursday, July 09, 2015 8:46 AM
>To: Zhigang Lu
>Cc: dev@dpdk.org
>Subject: Re: [dpdk-dev] [PATCH v3 04/12] eal: allow empty compile time
flags
>
>2015-07-06 16:51, Zhigang Lu:
>> The rte_cpu_check_supported() code breaks with a "comparison is always
>> false due to limited range of data type" when the compile_time_flags[]
>> array is empty.  Assigning the array dimension to a local variable
>> apparently solves this.
>

Can someone explain why this feature is needed?
I think it should be removed since it bring more troubles than benifit for the common users.
diff mbox

Patch

diff --git a/lib/librte_eal/common/eal_common_cpuflags.c b/lib/librte_eal/common/eal_common_cpuflags.c
index 6fd360c..8ba7b30 100644
--- a/lib/librte_eal/common/eal_common_cpuflags.c
+++ b/lib/librte_eal/common/eal_common_cpuflags.c
@@ -30,6 +30,7 @@ 
  *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
  *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
+#include <rte_common.h>
 #include <rte_cpuflags.h>
 
 /*
@@ -62,10 +63,10 @@  rte_cpu_check_supported(void)
 	static const enum rte_cpu_flag_t compile_time_flags[] = {
 			RTE_COMPILE_TIME_CPUFLAGS
 	};
-	unsigned i;
+	unsigned count = RTE_DIM(compile_time_flags), i;
 	int ret;
 
-	for (i = 0; i < sizeof(compile_time_flags)/sizeof(compile_time_flags[0]); i++) {
+	for (i = 0; i < count; i++) {
 		ret = rte_cpu_get_flag_enabled(compile_time_flags[i]);
 
 		if (ret < 0) {