|
Replies:
4
-
Last Post:
Mar 23, 2007 7:44 PM
by: wesolows
|
|
|
Posts:
17
From:
US
Registered:
7/19/06
|
|
|
|
webrev for gcc4/gccfss support in ON
Posted:
Feb 23, 2007 4:18 PM
|
|
Hi,
hopefully some of you are familiar with
"Support gcc4/GCCfss in Solaris" project:
http://www.opensolaris.org/os/project/gccfss-on/
here is the first webrev for it (corresponding CR 6515400):
http://cr.grommit.com/~alex/webrev/
the issues it's covering are:
. gcc 4.x don't like static function prototypes declared inside function
bodies, so those need to be moved out
. unsigned_var >= 0 and unsigned_var < 0 comparisons need to be casted to
signed types otherwise gcc 4.x optimize them out and cause bugs
like CR 6513314
. 64-bit types need to be printed via %llx notation and not via %lx
. rpcgen must emit declaration of struct FOO before it can be used in
declaration of array of struct FOO
. fix constructs undefined in C like: ar[i] =]]
|
|
|
Posts:
818
From:
San Francisco CA US
Registered:
3/9/05
|
|
|
|
Re: webrev for gcc4/gccfss support in ON
Posted:
Feb 23, 2007 4:33 PM
in response to: alexey
|
|
On Fri, Feb 23, 2007 at 04:18:45PM -0800, Alexey Starovoytov wrote:
> here is the first webrev for it (corresponding CR 6515400):
> http://cr.grommit.com/~alex/webrev/
I'll take a look at this next week. I do have a couple general
comments, though.
> . unsigned_var >= 0 and unsigned_var < 0 comparisons need to be casted to
> signed types otherwise gcc 4.x optimize them out and cause bugs
> like CR 6513314
If that's the case, it's not unlikely that the variable should be
declared signed. There are cases in which this isn't possible; for
example, where struct members are defined to be unsigned by a standard
yet we need to treat them as signed. But usually if you really need
to compare something >= 0, it should just be signed.
> . 64-bit types need to be printed via %llx notation and not via %lx
That's not correct. To print out uint64_t variables reliably, either
case the variable to long long and use 'll', or use PRIx64 instead of
%llx. I mainly did the latter when we did this for gcc3; I'm a bit
surprised that new ones could have snuck in. Is it possible that the
typedef for [u]int64_t is not getting established correctly with gcc4,
perhaps due to preprocessor definition changes?
> . fix constructs undefined in C like: ar[i] = i++
Yes, please. Ugh.
--
Keith M Wesolowski "Sir, we're surrounded!"
FishWorks "Excellent; we can attack in any direction!"
_______________________________________________
tools-gcc mailing list
tools-gcc at opensolaris dot org
|
|
|
|
|
Posts:
17
From:
US
Registered:
7/19/06
|
|
|
|
Re: webrev for gcc4/gccfss support in ON
Posted:
Feb 23, 2007 5:28 PM
in response to: wesolows
|
|
On Fri, 23 Feb 2007, Keith M Wesolowski wrote:
> I'll take a look at this next week. I do have a couple general
> comments, though.
Thanks!
> > . unsigned_var >= 0 and unsigned_var < 0 comparisons need to be casted to
> > signed types otherwise gcc 4.x optimize them out and cause bugs
> > like CR 6513314
>
> If that's the case, it's not unlikely that the variable should be
> declared signed. There are cases in which this isn't possible; for
> example, where struct members are defined to be unsigned by a standard
> yet we need to treat them as signed. But usually if you really need
> to compare something >= 0, it should just be signed.
I agree. I tried to change the original type in some places, but
it turned out that the usage is predominantly unsigned-like, though
in some places the fix could be different.
Like in ttymux_ioctl.c 'plen' and 'alen' should be just plain 'int'
insted of 'size_t'.
> > . 64-bit types need to be printed via %llx notation and not via %lx
>
> That's not correct. To print out uint64_t variables reliably, either
> case the variable to long long and use 'll', or use PRIx64 instead of
> %llx. I mainly did the latter when we did this for gcc3; I'm a bit
> surprised that new ones could have snuck in. Is it possible that the
> typedef for [u]int64_t is not getting established correctly with gcc4,
> perhaps due to preprocessor definition changes?
Hmm. It turned out to be an interesting issue.
I suspect that gcc 3.4 is buggy.
It considers the type of expression 'long + long long' in 64-bit mode
as 'long' type, whereas gcc 4.x thinks it is 'long long' type.
Example:
void foo(long a, long long b)
{
printf ("%llx\n", a+b);
}
such code will complain with gcc 3, but will not complain with gcc 4,
and vice versa if you change %llx to %lx
Alex.
_______________________________________________
tools-gcc mailing list
tools-gcc at opensolaris dot org
|
|
|
|
|
Posts:
17
From:
US
Registered:
7/19/06
|
|
|
|
Re: webrev for gcc4/gccfss support in ON
Posted:
Mar 15, 2007 5:38 PM
in response to: alexey
|
|
Hi,
updated webrev with more recent ON sources and integrated
comments regarding type casts:
http://cr.grommit.com/~alex/webrev/
Some other comments below:
On Fri, 23 Feb 2007, Alexey Starovoytov wrote:
> On Fri, 23 Feb 2007, Keith M Wesolowski wrote:
>
> > I'll take a look at this next week. I do have a couple general
> > comments, though.
>
> Thanks!
>
> > > . unsigned_var >= 0 and unsigned_var < 0 comparisons need to be casted to
> > > signed types otherwise gcc 4.x optimize them out and cause bugs
> > > like CR 6513314
> >
> > If that's the case, it's not unlikely that the variable should be
> > declared signed. There are cases in which this isn't possible; for
> > example, where struct members are defined to be unsigned by a standard
> > yet we need to treat them as signed. But usually if you really need
> > to compare something >= 0, it should just be signed.
>
> I agree. I tried to change the original type in some places, but
> it turned out that the usage is predominantly unsigned-like, though
> in some places the fix could be different.
> Like in ttymux_ioctl.c 'plen' and 'alen' should be just plain 'int'
> insted of 'size_t'.
Integrated such changes in the webrev.
Now only struct memebers are casted. Regular variables are changed
to signed types where possible.
Since last webrev the absolutely trivial changes are in:
sysiosbus.c, fasttrap_isa.c, ttymux_ioctl.c
>
> > > . 64-bit types need to be printed via %llx notation and not via %lx
> >
> > That's not correct. To print out uint64_t variables reliably, either
> > case the variable to long long and use 'll', or use PRIx64 instead of
> > %llx. I mainly did the latter when we did this for gcc3; I'm a bit
> > surprised that new ones could have snuck in. Is it possible that the
> > typedef for [u]int64_t is not getting established correctly with gcc4,
> > perhaps due to preprocessor definition changes?
>
> Hmm. It turned out to be an interesting issue.
> I suspect that gcc 3.4 is buggy.
> It considers the type of expression 'long + long long' in 64-bit mode
> as 'long' type, whereas gcc 4.x thinks it is 'long long' type.
>
> Example:
> void foo(long a, long long b)
> {
> printf ("%llx\n", a+b);
> }
>
> such code will complain with gcc 3, but will not complain with gcc 4,
> and vice versa if you change %llx to %lx
>
> Alex.
The %lx -> %llx changes are correct.
As demonstrated by the above example, the /usr/sfw/bin/gcc is buggy and
Solaris should not keep the changes that workaround the known issues of
old gcc compiler.
Alex.
_______________________________________________
tools-gcc mailing list
tools-gcc at opensolaris dot org
|
|
|
|
|
Posts:
818
From:
San Francisco CA US
Registered:
3/9/05
|
|
|
|
Re: webrev for gcc4/gccfss support in ON
Posted:
Mar 23, 2007 7:44 PM
in response to: alexey
|
|
On Thu, Mar 15, 2007 at 05:38:33PM -0700, Alexey Starovoytov wrote:
> updated webrev with more recent ON sources and integrated
> comments regarding type casts:
>
> http://cr.grommit.com/~alex/webrev/
Sorry for taking so long. I've reviewed every change in every file;
however, I point out a few places where I'm not knowledgeable enough
to give an adequate review.
usr/src/Makefile.master:
120-121: __GCCFSS should imply __GNUC and __GNUC64 by default.
137-139: This is not the way we address problems of this type. You
should fix rpcgen separately, first, and preannounce a flag
day. Wait a build, then putback your gcc wad and send a
formal flag day notice then.
usr/src/cmd/bnu/grades.c:
54: Since you're fixing this anyway, please give lcase() an actual
prototype rather than just a K&R-style declaration (yes, I know,
the whole thing is K&R, but one ought to fix what one can).
usr/src/cmd/bnu/uucp.c:
54: Likewise.
usr/src/cmd/bnu/uux.c:
64: Likewise.
usr/src/cmd/dfs.cmds/general/general.c:
60: Likewise.
usr/src/cmd/fm/modules/common/eversholt/iexpr.c:
How did we miss these? I know the problem with this function;
sometimes it's __noreturn and sometimes not based on the flags. But I
see a number of places where we fixed this before in the same way you
did. Your changes look ok, but I don't understand why we missed them
previously.
usr/src/cmd/fmli/menu/mcurrent.c:
48: As for usr/src/cmd/bnu/grades.c.
usr/src/cmd/fmli/menu/stmenu.c:
49: Likewise.
50: Wouldn't it be better to just remove this function and use
strncasecmp(3c) instead?
usr/src/cmd/fmli/oh/if_form.c:
195: As for usr/src/cmd/bnu/grades.c.
usr/src/cmd/fmli/oh/if_init.c:
228-229: Likewise.
usr/src/cmd/fmli/oh/scram.c:
72: Likewise.
374: Just pass NULL.
usr/src/cmd/fmli/oh/slk.c:
180: As for usr/src/cmd/bnu/grades.c.
usr/src/cmd/fmli/qued/multiline.c:
46: Likewise.
usr/src/cmd/hal/hald/Makefile:
52: What's the real bug here? Is there a plan for fixing it? Just
doing -w is...pretty bad. I think we did it for perl only because
there were one or two bugs that were difficult to fix.
usr/src/cmd/ipf/lib/common/ipft_ef.c:
122-123: The comment needs to be an explanation of what's actually
happening here, and this semantic change needs to be reviewed
by an ipfilter expert (if it hasn't already).
usr/src/cmd/ipf/lib/common/ipft_td.c:
142-143: Likewise.
usr/src/cmd/krb5/kadmin/gui/native/Kadmin.c:
301: No need for the comment.
usr/src/cmd/ldapcachemgr/cachemgr.c:
69, 875: client_killserver() should be given a proper prototype both
places.
usr/src/cmd/mailx/hdr/glob.h:
114: Why is this necessary?
usr/src/cmd/mdb/common/libstand/ctime.c:
34: As for usr/src/cmd/bnu/grades.c.
usr/src/cmd/mdb/common/mdb/mdb_lex.l:
63: This change is a bit worrisome; you're exposing __iob to the
entire kernel's namespace. Please ask Mike or another mdb export
to review this (if you haven't already).
usr/src/cmd/pcmciad/pcmciad.c:
256, 1961: As for usr/src/cmd/ldapcachemgr/cachemgr.c.
usr/src/cmd/pg/pg.c:
110, 1056: Likewise.
usr/src/cmd/sgs/size/common/process.c:
45: Drop the 'num' in the prototype.
usr/src/cmd/ttymon/tmextern.h:
40: Lose the comment. Such things become out of date and useless
almost immediately.
usr/src/cmd/ttymon/tmstruct.h:
Should be _TMSTRUCT_H, not __.
usr/src/cmd/ttymon/ttymon.c:
64: As for usr/src/cmd/bnu/grades.c.
usr/src/cmd/vi/port/ex_cmdsub.c:
1736: Lose the declaration include <stdlib.h> instead.
1740: The argument should be cast to (char *), not (const char *).
usr/src/cmd/vi/port/ex_voper.c:
59: As for usr/src/cmd/bnu/grades.c.
usr/src/cmd/vi/port/exrecover.c:
90-91, 393-394, 451: Just change qucmp and its prototype declaration
to match what qsort wants. It has no other
callers.
usr/src/cmd/ypcmd/ypupdated/rpc.ypupdated.c:
62: As for usr/src/cmd/bnu/grades.c.
usr/src/lib/crypt_modules/bsdbf/arc4random.c:
107: u_char is not to be used. Use uchar_t instead.
usr/src/lib/libbc/libc/gen/common/usleep.c:
25: As for usr/src/cmd/bnu/grades.c.
usr/src/lib/libnsl/rpc/svc_vc.c:
123: Likewise.
usr/src/lib/libresolv/res_gethost.c:
62-63: Likewise.
usr/src/psm/stand/cpr/sparcv9/sun4u/cprboot.c:
443: As noted previously, until gcc 3.4 is fixed in SFW, you need to
be sure this still builds cleanly with 3.4.3. Creating local
long longs or casting seem like reasonable options.
usr/src/tools/cw/cw.c
579-583: For gccfss, is -Wa,... a meaningful option? That is, if we
get it, should we abort?
599: You need to use (and fix) the CC() macro. Otherwise, using
gccfss as a shadow compiler won't work. Another question: if
-_gccfss is given, what should the shadow compiler be?
730-732: Shouldn't these be conditional on is_gccfss? Otherwise
-_gccfss=foo will be passed even when the compiler option is
-_gcc. I don't think that's what we want.
Generally: Are there any -W2 and such options you would *not* want to
pass through? If not, you should just do
if (strncmp(arg, "-W2,", 4) == 0 && is_gccfss) {
newae(ctx->i_ae, arg);
break;
}
and so on at the top of case 'W'.
usr/src/tools/tokenize/asmsubr.s,
usr/src/tools/tokenize/forth_preload.c:
I confess I don't know what tokenize does. I presume you're trying to
make it v8plus-safe? Someone else needs to review this, if that's not
already been done.
usr/src/uts/common/sys/mode.h:
45: As for usr/src/cmd/ttymon/tmextern.h.
usr/src/uts/sun4/io/px/px_pec.c:
272: Lose the comment.
usr/src/uts/sun4u/io/pci/pcisch.c:
2973-2974: As for usr/src/psm/stand/cpr/sparcv9/sun4u/cprboot.c. This
one's a bit stranger, though, since we don't have two
values being added together.
usr/src/uts/sun4u/opl/io/drmach.c:
2290: As for usr/src/uts/sun4u/io/pci/pcisch.c.
usr/src/uts/sun4u/serengeti/io/sbdp_cpu.c:
413: As for usr/src/psm/stand/cpr/sparcv9/sun4u/cprboot.c.
usr/src/uts/sun4u/starcat/io/drmach.c:
3199: Likewise.
6871: Likewise.
usr/src/uts/sun4u/starcat/io/schpc.c:
2068-2075: Indeed so. Have the people who own this commented?
usr/src/uts/sun4u/starfire/io/drmach.c:
2959: As for usr/src/psm/stand/cpr/sparcv9/sun4u/cprboot.c.
usr/src/uts/sun4u/vm/zulu_hat.c:
1161: Use uintptr_t instead of size_t. Lose the comment.
--
Keith M Wesolowski "Sir, we're surrounded!"
FishWorks "Excellent; we can attack in any direction!"
_______________________________________________
tools-gcc mailing list
tools-gcc at opensolaris dot org
|
|
|
|
|
