LinuxLists.cc - [PATCH] s390 (7/13): gcc 3.3 adaptions.

[permalink] [raw]

Subject: Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

On Mon, 24 Feb 2003, Martin Schwidefsky wrote:

> updates for compiling with gcc-3.3pre
>

[SNIPPED...]
> - Don't warn about signed/unsigned comparisions

I think you must keep these warnings in! There are many bugs
that these uncover uncluding loops that don't terminate correctly
but seem to work for "most all" cases. These are the hard-to-find
bugs that hit you six months after release.

size_t i;

while((i = do_forever()) > 0)
;

... do_forever() finally errors out and returns -1 stuck(forever).

Cheers,
Dick Johnson
Penguin : Linux version 2.4.18 on an i686 machine (797.90 BogoMips).
Why is the government concerned about the lunatic fringe? Think about it.

2003-02-24 19:55:27

by Arnd Bergmann

[permalink] [raw]

Subject: Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

Richard B. Johnson wrote:

> I think you must keep these warnings in! There are many bugs
> that these uncover uncluding loops that don't terminate correctly
> but seem to work for "most all" cases. These are the hard-to-find
> bugs that hit you six months after release.

This was my change. Obviously the warning is a good idea in general,
but I don't see the point of scrolling through hundreds of lines
with the same warning in someone else's code. I actually plan to fix
these warnings in arch/s390 and drivers/s390 as well as include/
and make the s390 kernel compile with -Werror, but the rest looks
more like a task for the Janitors. Note that before gcc-3.3,
-Wsign-compare has not been part of -Wall.

Arnd <><

2003-02-24 20:36:01

[permalink] [raw]

Subject: Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

On Mon, 24 Feb 2003, Arnd Bergmann wrote:

> Richard B. Johnson wrote:
>
> > I think you must keep these warnings in! There are many bugs
> > that these uncover uncluding loops that don't terminate correctly
> > but seem to work for "most all" cases. These are the hard-to-find
> > bugs that hit you six months after release.
>
> This was my change. Obviously the warning is a good idea in general,
> but I don't see the point of scrolling through hundreds of lines
> with the same warning in someone else's code. I actually plan to fix
> these warnings in arch/s390 and drivers/s390 as well as include/
> and make the s390 kernel compile with -Werror, but the rest looks
> more like a task for the Janitors. Note that before gcc-3.3,
> -Wsign-compare has not been part of -Wall.
>

Yep. There are a lot of things, not part of -Wall. To find 'need coffee'
errors, I use:

-Wall -Werror -Wstrict-prototypes -Wwrite-strings -Wshadow \
-Wsign-compare -Wno-trigraphs

plus...
-fno-builtin -fno-common -fomit-frame-pointer.

I also use -march=1486 so it will boot. Some machines don't like
`cmov` and other stuff the compiler may emit.

Cheers,
Dick Johnson
Penguin : Linux version 2.4.18 on an i686 machine (797.90 BogoMips).
Why is the government concerned about the lunatic fringe? Think about it.

2003-02-24 20:57:34

[permalink] [raw]

Subject: Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

On Mon, 24 Feb 2003, Richard B. Johnson wrote:
>
> I think you must keep these warnings in! There are many bugs
> that these uncover uncluding loops that don't terminate correctly
> but seem to work for "most all" cases. These are the hard-to-find
> bugs that hit you six months after release.

At least historically gcc has been so f*cking bad at the "unsigned vs
signed" warnings that they are totally useless.

Maybe things are better in gcc-3.3.

Maybe not.

> size_t i;
>
> while((i = do_forever()) > 0)
> ;
>
> ... do_forever() finally errors out and returns -1 stuck(forever).

Does gcc still warn about things like

#define COUNT (sizeof(array)/sizeof(element))

int i;
for (i = 0; i < COUNT; i++)
...

where COUNT is obviously unsigned (because sizeof is size_t and thus
unsigned)?

Gcc used to complain about things like that, which is a FUCKING DISASTER.

Any compiler that complains about the above should be shot in the head,
and the warning should be killed.

Linus

2003-02-24 21:20:14

[permalink] [raw]

Subject: Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

On Mon, 24 Feb 2003, Linus Torvalds wrote:

>
> On Mon, 24 Feb 2003, Richard B. Johnson wrote:
> >
> > I think you must keep these warnings in! There are many bugs
> > that these uncover uncluding loops that don't terminate correctly
> > but seem to work for "most all" cases. These are the hard-to-find
> > bugs that hit you six months after release.
>
> At least historically gcc has been so f*cking bad at the "unsigned vs
> signed" warnings that they are totally useless.
>
> Maybe things are better in gcc-3.3.
>
> Maybe not.
>
>
> > size_t i;
> >
> > while((i = do_forever()) > 0)
> > ;
> >
> > ... do_forever() finally errors out and returns -1 stuck(forever).
>
> Does gcc still warn about things like
>
> #define COUNT (sizeof(array)/sizeof(element))
>
> int i;
> for (i = 0; i < COUNT; i++)
> ...
>
> where COUNT is obviously unsigned (because sizeof is size_t and thus
> unsigned)?
>
> Gcc used to complain about things like that, which is a FUCKING DISASTER.
>
> Any compiler that complains about the above should be shot in the head,
> and the warning should be killed.
>
> Linus
>

Well yes. And it's correct. Variable `i' in the following code
should be a 'size_t'.

Script started on Mon Feb 24 16:23:19 2003
# cat xxx.c
#define ArraySize(a) (sizeof(a) / sizeof(a[0]))
int foo[12] = {0,};
int count()
{
int i;
for(i=0; i< ArraySize(foo); i++)
;
return i;
}

# gcc -Wall -Wsign-compare -c -o xxx.o xxx.c
xxx.c: In function `count':
xxx.c:11: warning: comparison between signed and unsigned
# exit
exit
Script done on Mon Feb 24 16:23:50 2003

... and we all know that it's dumb to do:
for(i=0; i< (int) ArraySize(foo); i++)
... just to placate a compiler. But the compiler did find what
could-have-been a real problem so the compiler is, indeed, correct.

Maybe what is needed is -Wsign-compare and -Wsign-strict, where
-Wsign-compare warns of obvious errors that could cause continuous
loops, etc., and -Wsign-strict acts like the current -Wsign-compare.
I agree that the current -Wsign-compare is way too strict for the
usual coding style where most int compares are "don't care" things
that are not worth casts.

Cheers,
Dick Johnson
Penguin : Linux version 2.4.18 on an i686 machine (797.90 BogoMips).
Why is the government concerned about the lunatic fringe? Think about it.

2003-02-24 21:17:40

by Jeff Garzik

[permalink] [raw]

Subject: Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

On Mon, Feb 24, 2003 at 01:02:39PM -0800, Linus Torvalds wrote:
> Does gcc still warn about things like
>
> #define COUNT (sizeof(array)/sizeof(element))
>
> int i;
> for (i = 0; i < COUNT; i++)
> ...
>
> where COUNT is obviously unsigned (because sizeof is size_t and thus
> unsigned)?
>
> Gcc used to complain about things like that, which is a FUCKING DISASTER.
>
> Any compiler that complains about the above should be shot in the head,
> and the warning should be killed.

Maybe... I suppose it's an implementation issue, because the lack of
signedness issues is probably only noticeable after data value analysis.

Playing devil's advocate here, I actually don't mind it warning for a
scenarion like this, because quite often it indicates an area where, if
s/int/unsigned int/ is performed, the compiler could potentially do a
better job of optimizing.

I agree your above specific example shouldn't trigger a warning
[implementation excuses aside].

Jeff

2003-02-24 21:28:01

by Adrian Bunk

[permalink] [raw]

Subject: Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

On Mon, Feb 24, 2003 at 01:02:39PM -0800, Linus Torvalds wrote:
>...
> Does gcc still warn about things like
>
> #define COUNT (sizeof(array)/sizeof(element))
>
> int i;
> for (i = 0; i < COUNT; i++)
> ...
>
> where COUNT is obviously unsigned (because sizeof is size_t and thus
> unsigned)?
>
> Gcc used to complain about things like that, which is a FUCKING DISASTER.
>
> Any compiler that complains about the above should be shot in the head,
> and the warning should be killed.

In the program below -Wall doesn't warn in neither 2.95 nor 3.2. You
need -Wsign-compare to get a warning. IMHO a warning is useful in this
example, some people might wrongly assume that the "Hello, world!" would
be printed more than once.

<-- snip -->

#include <stdio.h>

int array[] = {1, 2, 3, 4, 5};

#define COUNT (sizeof(array)/sizeof(array[0]))

int main()
{
int i;

for (i = 0; i < COUNT; i++)
{
i -= 2;
printf("Hello, world!\n");
}

return(0);
}

<-- snip -->

> Linus

cu
Adrian

--

"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed

2003-02-24 21:25:48

[permalink] [raw]

Subject: Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

Linus Torvalds <[email protected]> writes:

|> Does gcc still warn about things like
|>
|> #define COUNT (sizeof(array)/sizeof(element))
|>
|> int i;
|> for (i = 0; i < COUNT; i++)
|> ...
|>
|> where COUNT is obviously unsigned (because sizeof is size_t and thus
|> unsigned)?
|>
|> Gcc used to complain about things like that, which is a FUCKING DISASTER.

How can you distinguish that from other occurrences of (int)<(size_t)?

Andreas.

--
Andreas Schwab, SuSE Labs, [email protected]
SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 N?rnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."

2003-02-24 21:43:27

by Jeff Garzik

[permalink] [raw]

Subject: Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

On Mon, Feb 24, 2003 at 10:35:24PM +0100, Andreas Schwab wrote:
> Linus Torvalds <[email protected]> writes:
>
> |> Does gcc still warn about things like
> |>
> |> #define COUNT (sizeof(array)/sizeof(element))
> |>
> |> int i;
> |> for (i = 0; i < COUNT; i++)
> |> ...
> |>
> |> where COUNT is obviously unsigned (because sizeof is size_t and thus
> |> unsigned)?
> |>
> |> Gcc used to complain about things like that, which is a FUCKING DISASTER.
>
> How can you distinguish that from other occurrences of (int)<(size_t)?

The bounds are obviously constant and unsigned at compile time.

Jeff

2003-02-24 21:57:14

[permalink] [raw]

Subject: Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

Jeff Garzik <[email protected]> writes:

|> On Mon, Feb 24, 2003 at 10:35:24PM +0100, Andreas Schwab wrote:
|> > Linus Torvalds <[email protected]> writes:
|> >
|> > |> Does gcc still warn about things like
|> > |>
|> > |> #define COUNT (sizeof(array)/sizeof(element))
|> > |>
|> > |> int i;
|> > |> for (i = 0; i < COUNT; i++)
|> > |> ...
|> > |>
|> > |> where COUNT is obviously unsigned (because sizeof is size_t and thus
|> > |> unsigned)?
|> > |>
|> > |> Gcc used to complain about things like that, which is a FUCKING DISASTER.
|> >
|> > How can you distinguish that from other occurrences of (int)<(size_t)?
|>
|> The bounds are obviously constant and unsigned at compile time.

But that's not the point. It's the runtime value of i that gets converted
(to unsigned), not the compile time value of COUNT. Thus if i ever gets
negative you have a problem.

Andreas.

--
Andreas Schwab, SuSE Labs, [email protected]
SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 N?rnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."

2003-02-24 22:12:40

by Jörn Engel

[permalink] [raw]

Subject: Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

On Mon, 24 February 2003 23:07:25 +0100, Andreas Schwab wrote:
>
> Jeff Garzik <[email protected]> writes:
>
> |> On Mon, Feb 24, 2003 at 10:35:24PM +0100, Andreas Schwab wrote:
> |> > Linus Torvalds <[email protected]> writes:
> |> >
> |> > |> Does gcc still warn about things like
> |> > |>
> |> > |> #define COUNT (sizeof(array)/sizeof(element))
> |> > |>
> |> > |> int i;
> |> > |> for (i = 0; i < COUNT; i++)
> |> > |> ...
> |> > |>
> |> > |> where COUNT is obviously unsigned (because sizeof is size_t and thus
> |> > |> unsigned)?
> |> > |>
> |> > |> Gcc used to complain about things like that, which is a FUCKING DISASTER.
> |> >
> |> > How can you distinguish that from other occurrences of (int)<(size_t)?
> |>
> |> The bounds are obviously constant and unsigned at compile time.
>
> But that's not the point. It's the runtime value of i that gets converted
> (to unsigned), not the compile time value of COUNT. Thus if i ever gets
> negative you have a problem.

COUNT is constant and COUNT < INT_MAX. gcc can cast the constant bound
to the variable's type to fix this problem.
Or, gcc can see that i starts with 0, it's value monotonously
increases and will never wrap around due to COUNT < INT_MAX. Not a
cheap test, but still possible.

J?rn

--
My second remark is that our intellectual powers are rather geared to
master static relations and that our powers to visualize processes
evolving in time are relatively poorly developed.
-- Edsger W. Dijkstra

2003-02-24 22:21:28

by Jörn Engel

[permalink] [raw]

Subject: Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

On Mon, 24 February 2003 23:21:44 +0100, J?rn Engel wrote:
>
> COUNT is constant and COUNT < INT_MAX. gcc can cast the constant bound
> to the variable's type to fix this problem.

Ok, Adrians example proved this approach stupid. But the one below
remains useful and catches one example, but not the other.

> Or, gcc can see that i starts with 0, it's value monotonously
> increases and will never wrap around due to COUNT < INT_MAX. Not a
> cheap test, but still possible.

J?rn

--
ticks = jiffies;
while (ticks == jiffies);
ticks = jiffies;
-- /usr/src/linux/init/main.c

2003-02-24 22:24:01

[permalink] [raw]

Subject: Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

On Mon, 24 Feb 2003, Andreas Schwab wrote:
> |>
> |> Gcc used to complain about things like that, which is a FUCKING DISASTER.
>
> How can you distinguish that from other occurrences of (int)<(size_t)?

Which is indeed my point. If you cannot distinguish it from incorrect
uses, you shouldn't be warnign the user, because the compiler obviously
doesn't know enough to make a sufficiently educated guess.

That said, a good compiler _can_ make a good warning. But to do so, you
have to actually do value analysis, instead of just blindly warning about
code that is obviously correct to a human.

Until gcc does sufficient value analysis, that signed warning is annoying,
worthless and a damn pain in the ass.

Linus

2003-02-24 22:29:34

by Jakub Jelinek

[permalink] [raw]

Subject: Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

2003-02-24 22:34:36

[permalink] [raw]

Subject: Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

On Mon, 24 Feb 2003, Andreas Schwab wrote:
>
> But that's not the point. It's the runtime value of i that gets converted
> (to unsigned), not the compile time value of COUNT. Thus if i ever gets
> negative you have a problem.

The point is that the compiler should see that the run-time value of i is
_obviously_never_negative_ and as such the warning is total and utter
crap.

The compiler actually does end up doing some of that kind of analysis when
optimizing, since it is required for some of the loop optimizations
anyway. But the warning is emitted before the analysis has taken place.

In other words, gcc is stupidly warning about something that is obviously
not an error. And it is obviously not an error because:

- array indexes are "int". They aren't size_t, and never have been. Thus
it is _correct_ to use "int" for the index. You write

int array[5];

and you do NOT write

int array[5UL];

and anybody who writes 'array[5UL]' is considered a stupid git and a
geek. Face it.

Claiming that you should index an array with a size_t is crap.

- the way this has traditionally always been done is the example I
posted. Warning about correct, obvious, and traditional code is WRONG,
if it causes the programmer to have to write something _less_ obvious
just to make a stupid compiler warning go away makes the warnign WORSE
THAN USELESS.

And yes, gcc could do the work necessary to only give the warning if it
actually has reason to believe that the code is wrong. As it is, it gives
the warning for code that is good.

Linus

2003-02-24 23:15:29

[permalink] [raw]

Subject: Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

On 25 Feb 2003, Alan Cox wrote:
> On Mon, 2003-02-24 at 22:39, Linus Torvalds wrote:
> > And yes, gcc could do the work necessary to only give the warning if it
> > actually has reason to believe that the code is wrong. As it is, it gives
> > the warning for code that is good.
>
> gcc gives the warning only when you ask it to annoy you. Seems a good trade off.

That _used_ to be true.

Look at the subject line. gcc-3.3 gives the warning for -Wall.

Linus

2003-02-24 23:08:58

by Alan

[permalink] [raw]

Subject: Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

On Mon, 2003-02-24 at 22:39, Linus Torvalds wrote:
> And yes, gcc could do the work necessary to only give the warning if it
> actually has reason to believe that the code is wrong. As it is, it gives
> the warning for code that is good.

gcc gives the warning only when you ask it to annoy you. Seems a good trade off. There
are about 15 bug fixes in 2.4.21-pre4ac4,ac5,ac6 solely from that, all real bugs and some
very non obvious

2003-02-24 23:28:54

by Alan

[permalink] [raw]

Subject: Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

On Mon, 2003-02-24 at 23:20, Linus Torvalds wrote:
> On 25 Feb 2003, Alan Cox wrote:
> > On Mon, 2003-02-24 at 22:39, Linus Torvalds wrote:
> > > And yes, gcc could do the work necessary to only give the warning if it
> > > actually has reason to believe that the code is wrong. As it is, it gives
> > > the warning for code that is good.
> >
> > gcc gives the warning only when you ask it to annoy you. Seems a good trade off.
>
> That _used_ to be true.
>
> Look at the subject line. gcc-3.3 gives the warning for -Wall.

gcc-3.3 doesnt exist yet. Maybe it wont do that now 8)

2003-02-24 23:42:54

by John Alvord

[permalink] [raw]

Subject: Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

On Mon, 24 Feb 2003 17:39:34 -0500, Jakub Jelinek <[email protected]>
wrote:

>On Mon, Feb 24, 2003 at 10:35:24PM +0100, Andreas Schwab wrote:
>> Linus Torvalds <[email protected]> writes:
>>
>> |> Does gcc still warn about things like
>> |>
>> |> #define COUNT (sizeof(array)/sizeof(element))
>> |>
>> |> int i;
>> |> for (i = 0; i < COUNT; i++)
>> |> ...
>> |>
>> |> where COUNT is obviously unsigned (because sizeof is size_t and thus
>> |> unsigned)?
>> |>
>> |> Gcc used to complain about things like that, which is a FUCKING DISASTER.
>>
>> How can you distinguish that from other occurrences of (int)<(size_t)?
>
>Value range propagation pass, then warn?

I know it is stupid/unnecessary etc, but you could do

#if COUNT > INT_MAX
#error you idiot...
#endif

int i;
for(i =0; i < (int)COUNT; i++)
...

where the #if was placed in whatever header COUNT was defined.

and have safe code with no runtime overhead and looking only mildly
idiotic.

john alvord

2003-02-24 23:45:55

[permalink] [raw]

Subject: Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

On 25 Feb 2003, Alan Cox wrote:
>
> gcc-3.3 doesnt exist yet. Maybe it wont do that now 8)

Right now there are some other problems with gcc-3.3 too, ie the inlining
is apparently broken enough that we'll either have to start using
__attribute__((force_inline)) or we'd better hope that the gcc people
decide to take the "inline" keyword more seriously (it's being discussed
on the gcc lists, so we'll see)

But yes, these are all obviously with "early versions", and it may be that
it changes before the real release.

Linus

2003-02-25 05:14:24

by Chris Wedgwood

[permalink] [raw]

Subject: Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

On Mon, Feb 24, 2003 at 02:39:36PM -0800, Linus Torvalds wrote:

> Claiming that you should index an array with a size_t is crap.

it's broken in general; there is *lots* of code which does things like
"foo[-1] = bar" (string parsers for example)

--cw

2003-02-25 12:09:11

by Jamie Lokier

[permalink] [raw]

Subject: Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

Chris Wedgwood wrote:
> On Mon, Feb 24, 2003 at 02:39:36PM -0800, Linus Torvalds wrote:
>
> > Claiming that you should index an array with a size_t is crap.
>
> it's broken in general; there is *lots* of code which does things like
> "foo[-1] = bar" (string parsers for example)

Yeah, the index should be ptrdiff_t :)

-- Jamie

2003-02-25 12:23:57

[permalink] [raw]

Subject: Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

J?rn Engel <[email protected]> writes:

|> On Mon, 24 February 2003 23:07:25 +0100, Andreas Schwab wrote:
|> >
|> > Jeff Garzik <[email protected]> writes:
|> >
|> > |> On Mon, Feb 24, 2003 at 10:35:24PM +0100, Andreas Schwab wrote:
|> > |> > Linus Torvalds <[email protected]> writes:
|> > |> >
|> > |> > |> Does gcc still warn about things like
|> > |> > |>
|> > |> > |> #define COUNT (sizeof(array)/sizeof(element))
|> > |> > |>
|> > |> > |> int i;
|> > |> > |> for (i = 0; i < COUNT; i++)
|> > |> > |> ...
|> > |> > |>
|> > |> > |> where COUNT is obviously unsigned (because sizeof is size_t and thus
|> > |> > |> unsigned)?
|> > |> > |>
|> > |> > |> Gcc used to complain about things like that, which is a FUCKING DISASTER.
|> > |> >
|> > |> > How can you distinguish that from other occurrences of (int)<(size_t)?
|> > |>
|> > |> The bounds are obviously constant and unsigned at compile time.
|> >
|> > But that's not the point. It's the runtime value of i that gets converted
|> > (to unsigned), not the compile time value of COUNT. Thus if i ever gets
|> > negative you have a problem.
|>
|> COUNT is constant and COUNT < INT_MAX. gcc can cast the constant bound
|> to the variable's type to fix this problem.

This is not how C works.

Andreas.

--
Andreas Schwab, SuSE Labs, [email protected]
SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 N?rnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."

2003-02-25 12:30:12

[permalink] [raw]

Subject: Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

Linus Torvalds <[email protected]> writes:

|> On Mon, 24 Feb 2003, Andreas Schwab wrote:
|> >
|> > But that's not the point. It's the runtime value of i that gets converted
|> > (to unsigned), not the compile time value of COUNT. Thus if i ever gets
|> > negative you have a problem.
|>
|> The point is that the compiler should see that the run-time value of i is
|> _obviously_never_negative_ and as such the warning is total and utter
|> crap.

This requires a complete analysis of the loop body, which means that the
warning must be moved down from the front end (the common type of the
operands only depends on the type of the operands, not of any current
value of the expressions).

|> The compiler actually does end up doing some of that kind of analysis when
|> optimizing, since it is required for some of the loop optimizations
|> anyway. But the warning is emitted before the analysis has taken place.
|>
|> In other words, gcc is stupidly warning about something that is obviously
|> not an error. And it is obviously not an error because:
|>
|> - array indexes are "int". They aren't size_t, and never have been. Thus
|> it is _correct_ to use "int" for the index. You write
|>
|> int array[5];
|>
|> and you do NOT write
|>
|> int array[5UL];
|>
|> and anybody who writes 'array[5UL]' is considered a stupid git and a
|> geek. Face it.

But array[-1] is wrong. An array can never have a negative index (I'm
*not* talking about pointers).

Andreas.

--
Andreas Schwab, SuSE Labs, [email protected]
SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 N?rnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."

2003-02-25 15:20:28

[permalink] [raw]

Subject: Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

On Tue, 25 Feb 2003, Andreas Schwab wrote:
> |>
> |> The point is that the compiler should see that the run-time value of i is
> |> _obviously_never_negative_ and as such the warning is total and utter
> |> crap.
>
> This requires a complete analysis of the loop body, which means that the
> warning must be moved down from the front end (the common type of the
> operands only depends on the type of the operands, not of any current
> value of the expressions).

So? Gcc does that anyway. _Any_ good compiler has to.

And if the compiler isn't good enough to do it, then the compiler
shouldn't be warning about something that it hasn't got a clue about.

> |> and anybody who writes 'array[5UL]' is considered a stupid git and a
> |> geek. Face it.
>
> But array[-1] is wrong. An array can never have a negative index (I'm
> *not* talking about pointers).

You're wrong.

Yes, when declaring an array, you cannot use "array[-1]". But that's not
because the thing is unsigned: the standard says that the array
declaration has to be a "integer value larger than zero". It is not
unsigned: it's _positive_.

However, in _indexing_ an array (as opposed to declaring it), "array[-1]"
is indeed perfectly fine, and is defined by the C language to be exactly
the same as "*(array-1)". And negative values are perfectly fine, even for
arrays. Trivial example:

int x[2][2];

int main(int argc, char **argv)
{
return x[1][-1];
}

the above is actually a well-defined C program, and 100%
standards-conforming ("strictly conforming").

Linus

2003-02-25 15:29:41

[permalink] [raw]

Subject: Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

Linus Torvalds <[email protected]> writes:

|> On Tue, 25 Feb 2003, Andreas Schwab wrote:
|> > |>
|> > |> The point is that the compiler should see that the run-time value of i is
|> > |> _obviously_never_negative_ and as such the warning is total and utter
|> > |> crap.
|> >
|> > This requires a complete analysis of the loop body, which means that the
|> > warning must be moved down from the front end (the common type of the
|> > operands only depends on the type of the operands, not of any current
|> > value of the expressions).
|>
|> So? Gcc does that anyway. _Any_ good compiler has to.

But the point is that determining the common type does not require _any_
kind of data flow analysis, and this is the place where the unsigned
warning is generated.

|> Trivial example:
|>
|> int x[2][2];
|>
|> int main(int argc, char **argv)
|> {
|> return x[1][-1];
|> }
|>
|>
|> the above is actually a well-defined C program, and 100%
|> standards-conforming ("strictly conforming").

This isn't as trivial as it seems. Look in comp.std.c for recent
discussions on this topic (out-of-array references).

Andreas.

--
Andreas Schwab, SuSE Labs, [email protected]
SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 N?rnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."

2003-02-25 15:56:14

[permalink] [raw]

Subject: Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

On Tue, 25 Feb 2003, Andreas Schwab wrote:
> |>
> |> So? Gcc does that anyway. _Any_ good compiler has to.
>
> But the point is that determining the common type does not require _any_
> kind of data flow analysis, and this is the place where the unsigned
> warning is generated.

Go back and read my mail. In fact, go back and read just about _any_ of my
mails on the subject. Gop back and read the part you snipped:

"And if the compiler isn't good enough to do it, then the compiler
shouldn't be warning about something that it hasn't got a clue about."

In other words, either gcc should do it right, or it should not be done at
all. Right now the warning IS GENERATED IN THE WRONG PLACE. Your argument
seems to be "but that's the place it is generated" is a total
non-argument. It's just stating a fact, and it's exactly that fact that
causes the BUG IN GCC.

> |> Trivial example:
> |>
> |> int x[2][2];
> |>
> |> int main(int argc, char **argv)
> |> {
> |> return x[1][-1];
> |> }
> |>
> |>
> |> the above is actually a well-defined C program, and 100%
> |> standards-conforming ("strictly conforming").
>
> This isn't as trivial as it seems. Look in comp.std.c for recent
> discussions on this topic (out-of-array references).

It is as trivial as it seems, and this is _not_ an out-of-array reference.
Sorry. C very clearly specifies the layout of arrays (6.5.2.1: last
subscript varies fastest, together with sizeof(array) = sizeof(entry)*n),
which _guarantees_ that the pointers involved in the above calculations
are all within the object, and thus there is _zero_ undefined behaviour.

So if they argued about this on comp.std.c, they either had clueless
people there (in addition to the ones that obviously aren't ;), _or_ you
misunderstood the argument.

Linus

2003-02-25 16:08:50

[permalink] [raw]

Subject: Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

On Tue, 25 Feb 2003, Linus Torvalds wrote:
>
> return x[1][-1];

Btw, don't get me wrong. I don't think the above is really code that
should survive, and if the compiler were to generate a warning for
something like that, where the subscripts are clearly out of the range
that they were in the declaration, then I'd be entirely supportive of
that.

I don't know how we got side-tracked to negative subscripts. They are
clearly legal with pointers, but that wasn't even the issue: the code that
generated the "signed/unsigned" warning didn't use any negative
subscripts, never had, and never will.

And unlike the abomination above, the code that generates the warning is
the _clearest_ version of code you can humanly write. And THAT is the
problem with the warning: there's no way to avoid the warning without
making the source code _worse_ in some way.

And _that_ is what my argument really boils down to. Nothing else.

Linus

2003-02-25 16:05:38

[permalink] [raw]

Subject: Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

On Tue, 25 Feb 2003, Linus Torvalds wrote:

>
> On Tue, 25 Feb 2003, Andreas Schwab wrote:
> > |>
> > |> The point is that the compiler should see that the run-time value of i is
> > |> _obviously_never_negative_ and as such the warning is total and utter
> > |> crap.
> >
> > This requires a complete analysis of the loop body, which means that the
> > warning must be moved down from the front end (the common type of the
> > operands only depends on the type of the operands, not of any current
> > value of the expressions).
>
> So? Gcc does that anyway. _Any_ good compiler has to.
>
> And if the compiler isn't good enough to do it, then the compiler
> shouldn't be warning about something that it hasn't got a clue about.
>
> > |> and anybody who writes 'array[5UL]' is considered a stupid git and a
> > |> geek. Face it.
> >
> > But array[-1] is wrong. An array can never have a negative index (I'm
> > *not* talking about pointers).
>
> You're wrong.
>
> Yes, when declaring an array, you cannot use "array[-1]". But that's not
> because the thing is unsigned: the standard says that the array
> declaration has to be a "integer value larger than zero". It is not
> unsigned: it's _positive_.
>
> However, in _indexing_ an array (as opposed to declaring it), "array[-1]"
> is indeed perfectly fine, and is defined by the C language to be exactly
> the same as "*(array-1)". And negative values are perfectly fine, even for
> arrays. Trivial example:
>
> int x[2][2];
>
> int main(int argc, char **argv)
> {
> return x[1][-1];
> }
>
>
> the above is actually a well-defined C program, and 100%
> standards-conforming ("strictly conforming").
>
> Linus
>

I'm glad you tackled that. I was going to write a response, but
backed off because I'm getting meek in my old age. I do wish to
add that, even in ix86 assembly language, indexing by using a
register and a memory oprand is a signed displacement, i.e,

here: .byte 0
foo: .byte 0

movl $0xffffffff, %ebx
movl foo,(%ebx), %eax

... accesses a memory location one byte before label foo, i.e.,
foo[-1] or here[0]. This means that with a N-bit addressing,
indexed addressing cannot access more than 2^(N-1) memory locations.

Something to consider when trying to access large arrays. For
instance, if an operating system were to allow flat-mode access from
0x00000000 to 0xffffffff memory addresses in 32-bit user-mode, could
you actually use all that address space in 'C'?

Cheers,
Dick Johnson
Penguin : Linux version 2.4.18 on an i686 machine (797.90 BogoMips).
Why is the government concerned about the lunatic fringe? Think about it.

2003-02-25 16:16:31