cvs commit: src/lib/libkse Makefile src/lib/libthr Makefile src/usr.bin/netstat Makefile src/usr.sbin/acpi/acpidb Makefile src/usr.sbin/kldxref Makefile src/usr.sbin/nscd Makefile src/usr.sbin/rpc.yppasswdd Makefile

[John Birrell]

On Tue, Nov 20, 2007 at 08:25:54AM +0000, Bjoern A. Zeeb wrote:
> On Tue, 20 Nov 2007, John Birrell wrote:
> 
> Hi,
> 
> >jb          2007-11-20 02:07:30 UTC
> >
> > FreeBSD src repository
> >
> > Modified files:
> >   lib/libkse           Makefile
> >   lib/libthr           Makefile
> >   usr.bin/netstat      Makefile
> >   usr.sbin/acpi/acpidb Makefile
> >   usr.sbin/kldxref     Makefile
> >   usr.sbin/nscd        Makefile
> >   usr.sbin/rpc.yppasswdd Makefile
> > Log:
> > These are the things that the tinderbox has problems with because it
> > doesn't use the default CFLAGS which contain -fno-strict-aliasing.
> >
> > Until the code is cleaned up, just add -fno-strict-aliasing to the
> > CFLAGS of these for the tinderboxes' sake, allowing the rest of the
> > tree to have -Werror enabled again.
> 
> I have the feeling that adding -fno-strict-aliasing just for the
> tinderbox to be happy is not the right solution, even if it is temporary.
> 
> Isn't lowering WARNS a better temporary solution?

My theory is that what is committed to CVS is the most important.

I think that I can achieve a better result by leaving the WARNS settings
committed by others to CVS rather than bowing to the personal custom
CVS flags set by the one person who hosts the tinderboxes on behalf
of the project.

I appreciate the contribution that the tinderboxes make to the project.
I think we _need_ them.

The thing that I take issue with is holding off re-enabling the WARNS
-Werror checks which used to serve us well simply because there are
custom CFLAGS set on the tinderboxes which cause non-fatal errors
that nobody _ever_ _looks_ _at_. The errors are non-fatal because
-Werror was disabled.

Only the tinderbox errors are reported to the mailing lists. So if
-Werror is never re-enabled, I have to ask... is it better that you
all never see the problem warnings for _all_ things, not just
strict aliasing warnings, or that you have the warm fuzzy feeling
that if the tinderbox builds don't fail, then the code is wonderful?

As an exercise to the readers of this message, please take a look
at src/lib/libthr, our preferred threading library. Remove the
-fno-strict-aliasing CFLAGS setting that I added. Build the library
using "CFLAGS=-02 -pipe" in /etc/make.conf like the tinderbox
uses. Notice the compilation error you get in
src/lib/libthr/thread/thr_sem.c. You will see that the compiler
is complaining about: 

        do {
                while ((val = (*sem)->count) > 0) {
                        if (atomic_cmpset_acq_int(&(*sem)->count, val, val - 1))
                                return (0);
                }
                if (abstime == NULL) {
                        errno = EINVAL;
                        return (-1);
                }
                clock_gettime(CLOCK_REALTIME, &ts);
                TIMESPEC_SUB(&ts2, abstime, &ts);
                _thr_cancel_enter(curthread);
                retval = _thr_umtx_wait((umtx_t *)&(*sem)->count, 0, &ts2);
                _thr_cancel_leave(curthread);
        } while (retval == 0);

... actually the cast in _thr_umtx_wait.

The problem here is that umtx functions work on u_longs whereas
semaphores have u_int32_t as the type for count.

On amd64 (on which I am typing this message), sizeof(long) != sizeof(u_int32_t),
so we have a problem.

By choosing to have -fno-strict-aliasing set by default in CVS we are
choosing to ignore this problem. The compiler follows our code and
performs the cast on the basis that it'll all be right in the end. But
will it?

On the tinderbox, des@ has chosen to report strict aliasing failures
because he is aware of the problems like the one I describe above.

So who is "right"?

We have CVS set to default to -fno-strict-aliasing and we choose, therefore
to ignore casts which actually shoot us in the foot.

I would prefer that the tinderboxes run with the same defaults that
we have in CVS, however, from what I have seen today, I think that we
may need to rethink our defaults.

So back to my goal.... I would like to see -Werror re-instated ASAP,
but I don't want to break the tinderboxes. By adding -fno-strict-aliasing
to CFLAGS in certain Makefiles, I am effectively flagging these as
"work required here". For everything else I can enable -Werror and
all subsequent builds will check for regressions from that.

--
John Birrell