Re: [PATCH][2.4.28-pre3] I2C driver core gcc-3.4 fixes

Re: [PATCH][2.4.28-pre3] I2C driver core gcc-3.4 fixes

[Mikael Pettersson]

On Sun, 12 Sep 2004 15:44:29 +0200, Jean Delvare <khali@linux-fr.org> wrote:
>> This patch fixes gcc-3.4 cast-as-lvalue warnings in the 2.4.28-pre3
>> kernel's I2C driver core. The i2c-core.c change is from the 2.6
>> kernel, the i2c-proc.c changes are new since the 2.6 code is
>> different.
>> (...)
>> --- linux-2.4.28-pre3/drivers/i2c/i2c-proc.c.~1~	2004-02-18 15:16:22.000000000 +0100
>> +++ linux-2.4.28-pre3/drivers/i2c/i2c-proc.c	2004-09-12 01:56:20.000000000 +0200
>> (...)
>> @@ -287,7 +287,7 @@
>>  			if(copy_to_user(buffer, BUF, buflen))
>>  				return -EFAULT;
>>  			curbufsize += buflen;
>> -			(char *) buffer += buflen;
>> +			buffer += buflen;
>>  		}
>>  	*lenp = curbufsize;
>>  	filp->f_pos += curbufsize;
>
>Looks like arithmetics on void* to me, so while removing a warning you
>add a different one. Same for all other "fixes" later in the patch.
>
>It doesn't look to me like you are fixing the code, only hiding the
>warnings. I am not really confident you aren't breaking things while
>doing this.

Yes, it results in code doing void* pointer arithmetic, but
the kernel uses that particular gcc extension in a lot of
places. It's ugly but known to work exactly like char*.

However, I'm no fan of void* arithmetic. Would code like
buffer = (void*)((char*)buffer + buflen); make you happier?

>After a quick look at the code I'd say that the buffer-like parameters
>involved should be declared as char* instead of void* in the first
>place, which would effectively make all further casts unnecessary, and
>still work exactly as before.

Maybe, but that's potentially a much larger change. I'm just
looking for the minimal changes to make the 2.4 kernel safe for
gcc-3.4 and later (cast-as-lvalue is an error in gcc-3.5/4.0).

/Mikael

Re: [PATCH][2.4.28-pre3] I2C driver core gcc-3.4 fixes

[Jean Delvare]

> Yes, it results in code doing void* pointer arithmetic, but
> the kernel uses that particular gcc extension in a lot of
> places. It's ugly but known to work exactly like char*.

OK, I didn't know that. Thanks for the info.

> However, I'm no fan of void* arithmetic. Would code like
> buffer = (void*)((char*)buffer + buflen); make you happier?

Well, it makes things even uglier IMHO, so let's not do it.

> >After a quick look at the code I'd say that the buffer-like
> >parameters involved should be declared as char* instead of void* in
> >the first place, which would effectively make all further casts
> >unnecessary, and still work exactly as before.
> 
> Maybe, but that's potentially a much larger change. I'm just
> looking for the minimal changes to make the 2.4 kernel safe for
> gcc-3.4 and later (cast-as-lvalue is an error in gcc-3.5/4.0).

I'm not much in favor of "fixing" old code just to make it gcc-3.5
compliant, especially when at the same time we won't clean it up,
potentially resulting in less readable code. I would be perfectly happy
with being able to compile 2.4 kernels with gcc versions up to 3.3 and
not above. What's the exact benefit of changing old and stable code in
the end of its life cycle for it to support future compilers? I don't
get it (but at the same time I am not the one deciding here).

This was a general comment. For the specific changes you propose, if
void* works like char* when it comes to arithmetics, it looks safe and
as such is (technically) fine with me.

Thanks.

-- 
Jean "Khali" Delvare
http://khali.linux-fr.org/

Re: [PATCH][2.4.28-pre3] I2C driver core gcc-3.4 fixes

[Marcelo Tosatti]

On Sun, Sep 12, 2004 at 05:43:12PM +0200, Jean Delvare wrote:
> > Yes, it results in code doing void* pointer arithmetic, but
> > the kernel uses that particular gcc extension in a lot of
> > places. It's ugly but known to work exactly like char*.
> 
> OK, I didn't know that. Thanks for the info.
> 
> > However, I'm no fan of void* arithmetic. Would code like
> > buffer = (void*)((char*)buffer + buflen); make you happier?
> 
> Well, it makes things even uglier IMHO, so let's not do it.
> 
> > >After a quick look at the code I'd say that the buffer-like
> > >parameters involved should be declared as char* instead of void* in
> > >the first place, which would effectively make all further casts
> > >unnecessary, and still work exactly as before.
> > 
> > Maybe, but that's potentially a much larger change. I'm just
> > looking for the minimal changes to make the 2.4 kernel safe for
> > gcc-3.4 and later (cast-as-lvalue is an error in gcc-3.5/4.0).
> 
> I'm not much in favor of "fixing" old code just to make it gcc-3.5
> compliant, especially when at the same time we won't clean it up,
> potentially resulting in less readable code. I would be perfectly happy
> with being able to compile 2.4 kernels with gcc versions up to 3.3 and
> not above. What's the exact benefit of changing old and stable code in
> the end of its life cycle for it to support future compilers? I don't
> get it (but at the same time I am not the one deciding here).

I'm applying these gcc 3.4 changes because they look straightforward to me
and I quite a lot of people were complaining about this.

I dont pretend to accept any further (gcc 3.5 and beyond) changes.

> This was a general comment. For the specific changes you propose, if
> void* works like char* when it comes to arithmetics, it looks safe and
> as such is (technically) fine with me.

Mikael?

Re: [PATCH][2.4.28-pre3] I2C driver core gcc-3.4 fixes

[Jean Delvare]

> This patch fixes gcc-3.4 cast-as-lvalue warnings in the 2.4.28-pre3
> kernel's I2C driver core. The i2c-core.c change is from the 2.6
> kernel, the i2c-proc.c changes are new since the 2.6 code is
> different.
> (...)
> --- linux-2.4.28-pre3/drivers/i2c/i2c-proc.c.~1~	2004-02-18 15:16:22.000000000 +0100
> +++ linux-2.4.28-pre3/drivers/i2c/i2c-proc.c	2004-09-12 01:56:20.000000000 +0200
> (...)
> @@ -287,7 +287,7 @@
>  			if(copy_to_user(buffer, BUF, buflen))
>  				return -EFAULT;
>  			curbufsize += buflen;
> -			(char *) buffer += buflen;
> +			buffer += buflen;
>  		}
>  	*lenp = curbufsize;
>  	filp->f_pos += curbufsize;

Looks like arithmetics on void* to me, so while removing a warning you
add a different one. Same for all other "fixes" later in the patch.

It doesn't look to me like you are fixing the code, only hiding the
warnings. I am not really confident you aren't breaking things while
doing this.

After a quick look at the code I'd say that the buffer-like parameters
involved should be declared as char* instead of void* in the first
place, which would effectively make all further casts unnecessary, and
still work exactly as before.

Thanks.

-- 
Jean "Khali" Delvare
http://khali.linux-fr.org/

[PATCH][2.4.28-pre3] I2C driver core gcc-3.4 fixes

[Mikael Pettersson]

This patch fixes gcc-3.4 cast-as-lvalue warnings in the 2.4.28-pre3
kernel's I2C driver core. The i2c-core.c change is from the 2.6 kernel,
the i2c-proc.c changes are new since the 2.6 code is different.

/Mikael

--- linux-2.4.28-pre3/drivers/i2c/i2c-core.c.~1~	2004-02-18 15:16:22.000000000 +0100
+++ linux-2.4.28-pre3/drivers/i2c/i2c-core.c	2004-09-12 01:56:20.000000000 +0200
@@ -750,7 +750,7 @@
 		msg.addr   = client->addr;
 		msg.flags = client->flags & I2C_M_TEN;
 		msg.len = count;
-		(const char *)msg.buf = buf;
+		msg.buf = (char *)buf;
 	
 		DEB2(printk(KERN_DEBUG "i2c-core.o: master_send: writing %d bytes on %s.\n",
 			count,client->adapter->name));
--- linux-2.4.28-pre3/drivers/i2c/i2c-proc.c.~1~	2004-02-18 15:16:22.000000000 +0100
+++ linux-2.4.28-pre3/drivers/i2c/i2c-proc.c	2004-09-12 01:56:20.000000000 +0200
@@ -205,7 +205,7 @@
 		table = i2c_entries[id]->ctl_table;
 		unregister_sysctl_table(i2c_entries[id]);
 		/* 2-step kfree needed to keep gcc happy about const points */
-		(const char *) temp = table[4].procname;
+		temp = (char *) table[4].procname;
 		kfree(temp);
 		kfree(table);
 		i2c_entries[id] = NULL;
@@ -287,7 +287,7 @@
 			if(copy_to_user(buffer, BUF, buflen))
 				return -EFAULT;
 			curbufsize += buflen;
-			(char *) buffer += buflen;
+			buffer += buflen;
 		}
 	*lenp = curbufsize;
 	filp->f_pos += curbufsize;
@@ -318,7 +318,7 @@
 					     sizeof(struct
 						    i2c_chips_data)))
 					return -EFAULT;
-				(char *) oldval +=
+				oldval +=
 				    sizeof(struct i2c_chips_data);
 				nrels++;
 			}
@@ -473,7 +473,7 @@
 		       !((ret=get_user(nextchar, (char *) buffer))) &&
 		       isspace((int) nextchar)) {
 			bufsize--;
-			((char *) buffer)++;
+			buffer++;
 		}
 
 		if (ret)
@@ -492,7 +492,7 @@
 		    && (nextchar == '-')) {
 			min = 1;
 			bufsize--;
-			((char *) buffer)++;
+			buffer++;
 		}
 		if (ret)
 			return -EFAULT;
@@ -503,7 +503,7 @@
 		       isdigit((int) nextchar)) {
 			res = res * 10 + nextchar - '0';
 			bufsize--;
-			((char *) buffer)++;
+			buffer++;
 		}
 		if (ret)
 			return -EFAULT;
@@ -517,7 +517,7 @@
 		if (bufsize && (nextchar == '.')) {
 			/* Skip the dot */
 			bufsize--;
-			((char *) buffer)++;
+			buffer++;
 
 			/* Read digits while they are significant */
 			while (bufsize && (mag > 0) &&
@@ -526,7 +526,7 @@
 				res = res * 10 + nextchar - '0';
 				mag--;
 				bufsize--;
-				((char *) buffer)++;
+				buffer++;
 			}
 			if (ret)
 				return -EFAULT;
@@ -542,7 +542,7 @@
 		       !((ret=get_user(nextchar, (char *) buffer))) &&
 		       isspace((int) nextchar)) {
 			bufsize--;
-			((char *) buffer)++;
+			buffer++;
 		}
 		if (ret)
 			return -EFAULT;
@@ -574,7 +574,7 @@
 			if(put_user(' ', (char *) buffer))
 				return -EFAULT;
 			curbufsize++;
-			((char *) buffer)++;
+			buffer++;
 		}
 
 		/* Fill BUF with the representation of the next string */
@@ -615,7 +615,7 @@
 		if(copy_to_user(buffer, BUF, buflen))
 			return -EFAULT;
 		curbufsize += buflen;
-		(char *) buffer += buflen;
+		buffer += buflen;
 
 		nr++;
 	}