Re: [PATCH][2.6.6-rc3] gcc-3.4.0 fixes

Re: [PATCH][2.6.6-rc3] gcc-3.4.0 fixes

[Mikael Pettersson]

On Thu, 29 Apr 2004 11:30:25 +0200, Ihar 'Philips' Filipau wrote:
>Mikael Pettersson wrote:
>> This patch fixes three warnings from gcc-3.4.0 in 2.6.6-rc3:
>> - drivers/char/ftape/: use of cast-as-lvalue
>>  		if (get_unaligned((__u32*)ptr)) {
>> -			++(__u32*)ptr;
>> +			ptr += sizeof(__u32);
>>  		} else {
>
>   Can anyone explain what is the problem with this?
>   To me it seems pretty ligitimate code - why it was outlawed in gcc 3.4?
>
>   Previous code was agnostic to type of ptr, but you code presume ptr 
>being char pointer (to effectively increment by 4 bytes).

'ptr' _is_ a char pointer, and the code (visible in the part of
the patch you didn't include) already performed pointer arithmetic
on it relying on it being a char pointer. The old code had no
sane reason at all for updating 'ptr' via a cast-as-lvalue.

cast-as-lvalue is not proper C, has dodgey semantics, and can
always be replaced by proper C.

Re: [PATCH][2.6.6-rc3] gcc-3.4.0 fixes

[H. Peter Anvin]

Followup to:  <200404292146.i3TLkfI0019612@harpo.it.uu.se>
By author:    Mikael Pettersson <mikpe@csd.uu.se>
In newsgroup: linux.dev.kernel
> 
> 'ptr' _is_ a char pointer, and the code (visible in the part of
> the patch you didn't include) already performed pointer arithmetic
> on it relying on it being a char pointer. The old code had no
> sane reason at all for updating 'ptr' via a cast-as-lvalue.
> 
> cast-as-lvalue is not proper C, has dodgey semantics, and can
> always be replaced by proper C.
> 

I don't see how it has dodgey semantics for cast of pointers or
[u]intptr_t to pointers.

IMNSHO the fact that it breaks C++ isn't a good reason to outlaw a
long-documented extension for C.

	-hpa

Re: [PATCH][2.6.6-rc3] gcc-3.4.0 fixes

[Mikael Pettersson]

H. Peter Anvin writes:
 > Followup to:  <200404292146.i3TLkfI0019612@harpo.it.uu.se>
 > By author:    Mikael Pettersson <mikpe@csd.uu.se>
 > In newsgroup: linux.dev.kernel
 > > 
 > > 'ptr' _is_ a char pointer, and the code (visible in the part of
 > > the patch you didn't include) already performed pointer arithmetic
 > > on it relying on it being a char pointer. The old code had no
 > > sane reason at all for updating 'ptr' via a cast-as-lvalue.
 > > 
 > > cast-as-lvalue is not proper C, has dodgey semantics, and can
 > > always be replaced by proper C.
 > > 
 > 
 > I don't see how it has dodgey semantics for cast of pointers or
 > [u]intptr_t to pointers.

You're assuming pointers have uniform representation.
C makes no such guarantees, and machines _have_ had
different types of representations in the past.
Some not-so-obsolete 64-bit machines in effect use fat
representations for pointers to functions (descriptors),
but they usually cheat and use pointers to the descriptors
instead. However, a C implementation could legally
represent a function pointer as a 128-bit value, while
data pointers remain 64 bits.

A cast fundamentally involves an assignment conversion,
a copy to a temporary, and it yields an rvalue.
Even if we allow its use as an lvalue, the semantics
would still be to assign the copy not the original.
So cast-as-lvalue as gcc implemented it changed two
major aspects of the semantics. Call me conservative
if you like, but that's simply not C any more.

Other gcc extensions, such as __inline__, __attribute__,
and __asm__, do provide useful and sensible features.
The issue with cast-as-lvalue is that it is neither
necessary nor does it promote maintainable and portable code.

Remember "the world's not a VAX" and "the world's not
a 68K with 24-bit addresses" lessons of the 80s,

 > IMNSHO the fact that it breaks C++ isn't a good reason to outlaw a
 > long-documented extension for C.

I couldn't care less about C++. There are ample reasons
why it's a bad idea in C itself.

/Mikael

Re: [PATCH][2.6.6-rc3] gcc-3.4.0 fixes

[William Lee Irwin III]

On Tue, Jun 01, 2004 at 04:52:59PM +0200, Mikael Pettersson wrote:
> You're assuming pointers have uniform representation.
> C makes no such guarantees, and machines _have_ had
> different types of representations in the past.
> Some not-so-obsolete 64-bit machines in effect use fat
> representations for pointers to functions (descriptors),
> but they usually cheat and use pointers to the descriptors
> instead. However, a C implementation could legally
> represent a function pointer as a 128-bit value, while
> data pointers remain 64 bits.

IIRC for all types foo, sizeof(foo *) <= sizeof(void *), no?
If so, 128-bit function pointers implies >= 128-bit void pointers.


On Tue, Jun 01, 2004 at 04:52:59PM +0200, Mikael Pettersson wrote:
> A cast fundamentally involves an assignment conversion,
> a copy to a temporary, and it yields an rvalue.
> Even if we allow its use as an lvalue, the semantics
> would still be to assign the copy not the original.
> So cast-as-lvalue as gcc implemented it changed two
> major aspects of the semantics. Call me conservative
> if you like, but that's simply not C any more.

Oh, yeah, lvalue casting is degenerate filth.


-- wli

Re: [PATCH][2.6.6-rc3] gcc-3.4.0 fixes

[Mikael Pettersson]

William Lee Irwin III writes:
 > On Tue, Jun 01, 2004 at 04:52:59PM +0200, Mikael Pettersson wrote:
 > > You're assuming pointers have uniform representation.
 > > C makes no such guarantees, and machines _have_ had
 > > different types of representations in the past.
 > > Some not-so-obsolete 64-bit machines in effect use fat
 > > representations for pointers to functions (descriptors),
 > > but they usually cheat and use pointers to the descriptors
 > > instead. However, a C implementation could legally
 > > represent a function pointer as a 128-bit value, while
 > > data pointers remain 64 bits.
 > 
 > IIRC for all types foo, sizeof(foo *) <= sizeof(void *), no?
 > If so, 128-bit function pointers implies >= 128-bit void pointers.

No, sizeof(foo*) <= sizeof(void*) only holds for data pointers.
The C standard is very explicit about not guaranteeing any
relationship between function pointers and void*. However,
a function pointer can be converted to a pointer to a different
function type and back again, without loss of information.

/Mikael

Re: [PATCH][2.6.6-rc3] gcc-3.4.0 fixes

[Andreas Schwab]

William Lee Irwin III <wli@holomorphy.com> writes:

> On Tue, Jun 01, 2004 at 04:52:59PM +0200, Mikael Pettersson wrote:
>> You're assuming pointers have uniform representation.
>> C makes no such guarantees, and machines _have_ had
>> different types of representations in the past.
>> Some not-so-obsolete 64-bit machines in effect use fat
>> representations for pointers to functions (descriptors),
>> but they usually cheat and use pointers to the descriptors
>> instead. However, a C implementation could legally
>> represent a function pointer as a 128-bit value, while
>> data pointers remain 64 bits.
>
> IIRC for all types foo, sizeof(foo *) <= sizeof(void *), no?

No.  There is no implied relation between data pointers and function
pointers.  The only requirement is that all _function_ pointers smell
alike, because you can convert any function pointer to any other function
pointer and back without losing information.  There is no dedicated
generic function pointer type, any one can function as one.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH][2.6.6-rc3] gcc-3.4.0 fixes

[H. Peter Anvin]

Mikael Pettersson wrote:
> 
> You're assuming pointers have uniform representation.
> C makes no such guarantees, and machines _have_ had
> different types of representations in the past.
 >

And are there *any* such architectures for which gcc are targetted.  I'm not 
advocating advancing this to the C standard, I'm advocating not removing an 
extension which has been documented for a decade.

	-hpa

Re: [PATCH][2.6.6-rc3] gcc-3.4.0 fixes

[H. Peter Anvin]

Mikael Pettersson wrote:
> 
> You're assuming pointers have uniform representation.
> C makes no such guarantees, and machines _have_ had
> different types of representations in the past.
 >

By the way, I am not in any shape, way or form making that assumption - 
although that's presumably how it would be *implemented* in an architecture 
with sane pointers like, to the best of my knowledge ALL gcc targets.

(foo *)bar++;

... should be implemented as ...

({
	foo *tmp1 = (foo *)bar;
	tmp2 = tmp1 + 1;
	bar = __typeof__(bar)tmp2;
	tmp1;
})

	-hpa

Re: [PATCH][2.6.6-rc3] gcc-3.4.0 fixes

[Mikael Pettersson]

H. Peter Anvin writes:
 > Mikael Pettersson wrote:
 > > 
 > > You're assuming pointers have uniform representation.
 > > C makes no such guarantees, and machines _have_ had
 > > different types of representations in the past.
 >  >
 > 
 > By the way, I am not in any shape, way or form making that assumption - 
 > although that's presumably how it would be *implemented* in an architecture 
 > with sane pointers like, to the best of my knowledge ALL gcc targets.
 > 
 > (foo *)bar++;
 > 
 > ... should be implemented as ...
 > 
 > ({
 > 	foo *tmp1 = (foo *)bar;
 > 	tmp2 = tmp1 + 1;
 > 	bar = __typeof__(bar)tmp2;
 > 	tmp1;
 > })

I did an experiment with updating an unsigned short
via a cast-as-lvalue to unsigned char, and gcc did
in fact implement the temp + copy semantics you
describe above. That is,

	unsigned short x = 0xaafe;
	((unsigned char)x)++;

resulted in x == 0x00ff.

So cast-as-lvalue is at least reasonably correctly
implemented in gcc.

Whether it's useful and portable is a another matter :-)

Re: [PATCH][2.6.6-rc3] gcc-3.4.0 fixes

[Linus Torvalds]

On Tue, 1 Jun 2004, Mikael Pettersson wrote:
> 
> You're assuming pointers have uniform representation.

Are we?

I don't see any point where we cast any function pointers to anything 
else.

We cast data pointers all over the place, but that is actually guaranteed
to work in C for some "large enough" integer type, and "unsigned long" is
pretty much it. 

And even function pointers should be safeish. The fact that some broken
architecture (can you say "ia64"?) has totally idiotic calling conventions
and requires the caller to load the GP value is _their_ problem. The
architecture will either die or hide the fact that it's being silly. For
now it's hiding it.

Repeat after me: practice is more important than theory. A _lot_ more 
important.

		Linus

Re: [PATCH][2.6.6-rc3] gcc-3.4.0 fixes

[Chris Wedgwood]

On Tue, Jun 01, 2004 at 10:27:16AM -0700, Linus Torvalds wrote:

> And even function pointers should be safeish. The fact that some
> broken architecture (can you say "ia64"?) has totally idiotic
> calling conventions and requires the caller to load the GP value is
> _their_ problem.

ia64 function pointers are actually pointers to the 128-bit
descriptors so all pointers are still 64-bit.


  --cw

Re: [PATCH][2.6.6-rc3] gcc-3.4.0 fixes

[H. Peter Anvin]

Linus Torvalds wrote:
> 
> I don't see any point where we cast any function pointers to anything 
> else.
> 
> We cast data pointers all over the place, but that is actually guaranteed
> to work in C for some "large enough" integer type, and "unsigned long" is
> pretty much it. 
> 

It would be nice at some point to switch that to [u]intptr_t, before someone 
comes up with 128-bit machines (in other words, no rush whatsoever, but after 
seeing Sony build processors with 128-bit integer registers I'm willing to 
believe it's just a matter of time...)  The other thing about it is that it's 
nice to be explicit about the "pointer-sized integerness" of it all.

> And even function pointers should be safeish. The fact that some broken
> architecture (can you say "ia64"?) has totally idiotic calling conventions
> and requires the caller to load the GP value is _their_ problem. The
> architecture will either die or hide the fact that it's being silly. For
> now it's hiding it.
> 
> Repeat after me: practice is more important than theory. A _lot_ more 
> important.

Indeed.

	-hpa

Re: [PATCH][2.6.6-rc3] gcc-3.4.0 fixes

[Ihar 'Philips' Filipau]

Mikael Pettersson wrote:
> This patch fixes three warnings from gcc-3.4.0 in 2.6.6-rc3:
> - drivers/char/ftape/: use of cast-as-lvalue
>  		if (get_unaligned((__u32*)ptr)) {
> -			++(__u32*)ptr;
> +			ptr += sizeof(__u32);
>  		} else {

   Can anyone explain what is the problem with this?
   To me it seems pretty ligitimate code - why it was outlawed in gcc 3.4?

   Previous code was agnostic to type of ptr, but you code presume ptr 
being char pointer (to effectively increment by 4 bytes).

   So what all this buzz about?

Re: [PATCH][2.6.6-rc3] gcc-3.4.0 fixes

[Paul Wagland]

[-- Attachment #1: Type: text/plain, Size: 1430 bytes --]


On Apr 29, 2004, at 11:30, Ihar 'Philips' Filipau wrote:

> Mikael Pettersson wrote:
>> This patch fixes three warnings from gcc-3.4.0 in 2.6.6-rc3:
>> - drivers/char/ftape/: use of cast-as-lvalue
>>  		if (get_unaligned((__u32*)ptr)) {
>> -			++(__u32*)ptr;
>> +			ptr += sizeof(__u32);
>>  		} else {
>
>   Can anyone explain what is the problem with this?
>   To me it seems pretty ligitimate code - why it was outlawed in gcc 
> 3.4?

http://es-sun2.fernuni-hagen.de/cgi-bin/info2html?(gcc)Subscripting

The ability to manipulate a cast was always a gcc extension, whether by 
design or feature I am not sure. The problem is that it breaks C++ 
templates in a bad way. Here is where the extension is documented for 
GCC 3.3.3

http://gcc.gnu.org/onlinedocs/gcc-3.3.3/gcc/Lvalues.html

This extension is now obsoleted.

>   Previous code was agnostic to type of ptr, but you code presume ptr 
> being char pointer (to effectively increment by 4 bytes).

Yes, and the previous code assumed that that ptr was not a pointer to a 
64 bit value, since dereferencing it on most platforms would then 
explode. I bet that both assumptions are just as safe ;-)  However if 
you really wanted to be safe then you could do

ptr += sizeof (__32) / sizeof (*ptr);

Again, assuming that we are not using a greater then 4 byte ptr*, since 
otherwise it would not increment... But personally I think that this is 
not required.

Cheers,
Paul

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 186 bytes --]

Re: [PATCH][2.6.6-rc3] gcc-3.4.0 fixes

[Denis Vlasenko]

On Thursday 29 April 2004 12:30, Ihar 'Philips' Filipau wrote:
> Mikael Pettersson wrote:
> > This patch fixes three warnings from gcc-3.4.0 in 2.6.6-rc3:
> > - drivers/char/ftape/: use of cast-as-lvalue
> >  		if (get_unaligned((__u32*)ptr)) {
> > -			++(__u32*)ptr;
> > +			ptr += sizeof(__u32);
> >  		} else {
>
>    Can anyone explain what is the problem with this?
>    To me it seems pretty ligitimate code - why it was outlawed in gcc 3.4?

cast is not a lvalue. ++(__u32*)ptr is nonsense, just like ++4.

>    Previous code was agnostic to type of ptr, but you code presume ptr
> being char pointer (to effectively increment by 4 bytes).

This would be agnostic too:

ptr = (void*) ((char*)ptr + sizeof(__u32));
--
vda

Re: [PATCH][2.6.6-rc3] gcc-3.4.0 fixes

[Ihar 'Philips' Filipau]

Denis Vlasenko wrote:
> On Thursday 29 April 2004 12:30, Ihar 'Philips' Filipau wrote:
> 
>>Mikael Pettersson wrote:
>>
>>>This patch fixes three warnings from gcc-3.4.0 in 2.6.6-rc3:
>>>- drivers/char/ftape/: use of cast-as-lvalue
>>> 		if (get_unaligned((__u32*)ptr)) {
>>>-			++(__u32*)ptr;
>>>+			ptr += sizeof(__u32);
>>> 		} else {
>>
>>   Can anyone explain what is the problem with this?
>>   To me it seems pretty ligitimate code - why it was outlawed in gcc 3.4?
> 
> 
> cast is not a lvalue. ++(__u32*)ptr is nonsense, just like ++4.
> 

   Yes. I see. C goes in direction of C++.
   For me cast in C is always "forget original type and assume this type."
   From this standpoint it makes a lot of sense. Is just like
((stuct { char a,b,c,d;})4).a - never tryed it in gcc (3.3 does not 
accept it), but IIRC was possible on old PC C compilers - Turbo C 2/3 - 
not sure name/version, but I used it. (Yes! recalled - M$VC/winbase.h 
use this kind of macros for manipulating bytes/words/dwords/etc)
   And in old C classes it was always said that in C you can convert 
anything to everything, it just size of types must be the same.

   Why not after all?

> 
> ptr = (void*) ((char*)ptr + sizeof(__u32));

   No Nice, But Accepted.

   I have just checked my code base - I do not use this feature in any 
way ;-)

-- 
Ihar 'Philips' Filipau  / with best regards from Saarbruecken.
--                                                           _ _ _
  A programmer is a person who passes as an exacting expert  |_|*|_|
  on the basis of being able to turn out, after innumerable  |_|_|*|
  punching, an infinite series of incomprehensible answers   |*|*|*|
  calculated with micrometric precisions from vague
  assumptions based on debatable figures taken from inconclusive
  documents and carried out on instruments of problematical accuracy
  by persons of dubious reliability and questionable mentality for
  the avowed purpose of annoying and confounding a hopelessly
  defenseless department that was unfortunate enough to ask for the
  information in the first place.
                 -- IEEE Grid newsmagazine

[PATCH][2.6.6-rc3] gcc-3.4.0 fixes

[Mikael Pettersson]

This patch fixes three warnings from gcc-3.4.0 in 2.6.6-rc3:
- arch/i386/pci/pcbios.c: use of "+m" constraint
- drivers/char/ftape/: use of cast-as-lvalue
- drivers/char/ftape/: __attribute__((packed)) on something
  containing only bytes

Compiles cleanly and works for me.

This isn't critical, I'll resend after 2.6.6 final if
you don't want to merge it right now.

/Mikael

diff -ruN linux-2.6.6-rc3/arch/i386/pci/pcbios.c linux-2.6.6-rc3.gcc340-fixes/arch/i386/pci/pcbios.c
--- linux-2.6.6-rc3/arch/i386/pci/pcbios.c	2003-09-09 14:22:28.000000000 +0200
+++ linux-2.6.6-rc3.gcc340-fixes/arch/i386/pci/pcbios.c	2004-04-28 12:21:00.000000000 +0200
@@ -431,11 +431,13 @@
 		"1:"
 		: "=a" (ret),
 		  "=b" (map),
-		  "+m" (opt)
+		  "=m" (opt)
 		: "0" (PCIBIOS_GET_ROUTING_OPTIONS),
 		  "1" (0),
 		  "D" ((long) &opt),
-		  "S" (&pci_indirect));
+		  "S" (&pci_indirect),
+		  "m" (opt)
+		: "memory");
 	DBG("OK  ret=%d, size=%d, map=%x\n", ret, opt.size, map);
 	if (ret & 0xff00)
 		printk(KERN_ERR "PCI: Error %02x when fetching IRQ routing table.\n", (ret >> 8) & 0xff);
diff -ruN linux-2.6.6-rc3/drivers/char/ftape/lowlevel/ftape-bsm.c linux-2.6.6-rc3.gcc340-fixes/drivers/char/ftape/lowlevel/ftape-bsm.c
--- linux-2.6.6-rc3/drivers/char/ftape/lowlevel/ftape-bsm.c	2002-02-20 03:10:58.000000000 +0100
+++ linux-2.6.6-rc3.gcc340-fixes/drivers/char/ftape/lowlevel/ftape-bsm.c	2004-04-28 12:21:00.000000000 +0200
@@ -203,6 +203,7 @@
 	    ft_format_code == fmt_1100ft) {
 		SectorCount *ptr = (SectorCount *)bad_sector_map;
 		unsigned int sector;
+		__u16 *ptr16;
 
 		while((sector = get_sector(ptr++)) != 0) {
 			if ((ft_format_code == fmt_big || 
@@ -218,9 +219,10 @@
 		}
 		/*  Display old ftape's end-of-file marks
 		 */
-		while ((sector = get_unaligned(((__u16*)ptr)++)) != 0) {
+		ptr16 = (__u16*)ptr;
+		while ((sector = get_unaligned(ptr16++)) != 0) {
 			TRACE(ft_t_noise, "Old ftape eof mark: %4d/%2d",
-			      sector, get_unaligned(((__u16*)ptr)++));
+			      sector, get_unaligned(ptr16++));
 		}
 	} else { /* fixed size format */
 		for (i = ft_first_data_segment;
diff -ruN linux-2.6.6-rc3/drivers/char/ftape/lowlevel/ftape-bsm.h linux-2.6.6-rc3.gcc340-fixes/drivers/char/ftape/lowlevel/ftape-bsm.h
--- linux-2.6.6-rc3/drivers/char/ftape/lowlevel/ftape-bsm.h	2002-02-20 03:10:52.000000000 +0100
+++ linux-2.6.6-rc3.gcc340-fixes/drivers/char/ftape/lowlevel/ftape-bsm.h	2004-04-28 12:21:00.000000000 +0200
@@ -47,7 +47,7 @@
  */
 typedef struct NewSectorMap {          
 	__u8 bytes[3];
-} SectorCount __attribute__((packed));
+} SectorCount;
 
 
 /*
diff -ruN linux-2.6.6-rc3/drivers/char/ftape/zftape/zftape-eof.c linux-2.6.6-rc3.gcc340-fixes/drivers/char/ftape/zftape/zftape-eof.c
--- linux-2.6.6-rc3/drivers/char/ftape/zftape/zftape-eof.c	2003-02-24 23:25:37.000000000 +0100
+++ linux-2.6.6-rc3.gcc340-fixes/drivers/char/ftape/zftape/zftape-eof.c	2004-04-28 12:21:00.000000000 +0200
@@ -123,7 +123,7 @@
 	while (ptr + 3 < limit) {
 
 		if (get_unaligned((__u32*)ptr)) {
-			++(__u32*)ptr;
+			ptr += sizeof(__u32);
 		} else {
 			return ptr;
 		}