Re: [HEADS-UP] Problem with clang in 9-stable [was: r268244 (stable/9) seems to break sysctl hw.ncpu]

2014-07-05 Thread David Wolfskill
On Sat, Jul 05, 2014 at 01:49:44PM +0100, David Chisnall wrote:
 On 4 Jul 2014, at 19:18, David Wolfskill da...@catwhisker.org wrote:
 
  clang -O2 -pipe  -std=gnu99 -Qunused-arguments  -fstack-protector 
  -Wsystem-headers -Werror -Wall -Wno-format-y2k -W -Wno-unused-parameter 
  -Wstrict-prototypes -Wmissing-prototypes -Wpointer-arith -Wno-uninitialized 
  -Wno-pointer-sign -Wno-empty-body -Wno-string-plus-int 
  -Wno-unused-const-variable -Wno-tautological-compare -Wno-unused-value 
  -Wno-parentheses-equality -Wno-unused-function -Wno-enum-conversion  -o 
  sysctl sysctl.o 
 
 This compile line is turning off a lot of warnings.  In particular, 
 -Wno-uninitialized and -Wno-parentheses-equality are likely to hide warnings 
 that refer to real errors.  It sounds like this case was one of them - if 
 these warnings were on then we'd have got a build failure rather than an 
 executable that depended on undefined behaviour.
 

So I checked src/sbin/sysctl/Makefile first; it's fairly vanilla:

#   @(#)Makefile8.1 (Berkeley) 6/6/93
# $FreeBSD: stable/9/sbin/sysctl/Makefile 203917 2010-02-15 14:08:06Z
uqs $

PROG=   sysctl
WARNS?= 3
MAN=sysctl.8

.include bsd.prog.mk

And the -Wno-uninitialized (at least) comes from bsd.sys.mk:

.if ${WARNS} = 2  ${WARNS} = 4
# XXX Delete -Wuninitialized by default for now -- the compiler doesn't
# XXX always get it right.
CWARNFLAGS+=-Wno-uninitialized
.endif # WARNS =2  WARNS = 4

A bit later, we see the origin of -Wno-parentheses-equality:

# Clang has more warnings enabled by default, and when using -Wall, so if WARNS
# is set to low values, these have to be disabled explicitly.
.if ${COMPILER_TYPE} == clang  !defined(EARLY_BUILD)
.if ${WARNS} = 6
CWARNFLAGS+=-Wno-empty-body -Wno-string-plus-int -Wno-unused-const-variable
.endif # WARNS = 6
.if ${WARNS} = 3
CWARNFLAGS+=-Wno-tautological-compare -Wno-unused-value\
-Wno-parentheses-equality -Wno-unused-function 
-Wno-enum-conversion
.endif # WARNS = 3
.if ${WARNS} = 2
CWARNFLAGS+=-Wno-switch -Wno-switch-enum -Wno-knr-promoted-parameter
.endif # WARNS = 2
.if ${WARNS} = 1
CWARNFLAGS+=-Wno-parentheses
.endif # WARNS = 1
.if defined(NO_WARRAY_BOUNDS)
CWARNFLAGS+=-Wno-array-bounds
.endif # NO_WARRAY_BOUNDS
.endif # CLANG


I don't know that there's likely to be a huge amount of interest
in addressing the issue for stable/9, but stable/10 looks similar,
and while I see some differences in head, the code in head's
bsd.sys.mk may well be functionally equivalent.

I'm happy to help test if someone wants to put together patches to
(at least) reduce the extent to which we have executables depending
on undefined behavior.

Peace,
david
-- 
David H. Wolfskill  da...@catwhisker.org
Taliban: Evil cowards with guns afraid of truth from a 14-year old girl.

See http://www.catwhisker.org/~david/publickey.gpg for my public key.


pgpclOWC9pp0z.pgp
Description: PGP signature


Re: [HEADS-UP] Problem with clang in 9-stable [was: r268244 (stable/9) seems to break sysctl hw.ncpu]

2014-07-05 Thread Dimitry Andric
On 05 Jul 2014, at 14:49, David Chisnall thera...@theravensnest.org wrote:
 On 4 Jul 2014, at 19:18, David Wolfskill da...@catwhisker.org wrote:
 
 clang -O2 -pipe  -std=gnu99 -Qunused-arguments  -fstack-protector 
 -Wsystem-headers -Werror -Wall -Wno-format-y2k -W -Wno-unused-parameter 
 -Wstrict-prototypes -Wmissing-prototypes -Wpointer-arith -Wno-uninitialized 
 -Wno-pointer-sign -Wno-empty-body -Wno-string-plus-int 
 -Wno-unused-const-variable -Wno-tautological-compare -Wno-unused-value 
 -Wno-parentheses-equality -Wno-unused-function -Wno-enum-conversion  -o 
 sysctl sysctl.o 
 
 This compile line is turning off a lot of warnings.  In particular, 
 -Wno-uninitialized and -Wno-parentheses-equality are likely to hide warnings 
 that refer to real errors.

Interestingly, -Wno-uninitialized has been in bsd.sys.mk since r76861,
and the accompanying comment (XXX Delete -Wuninitialized by default for
now -- the compiler doesn't always get it right) has never been
changed. :-)

It is probably time to re-enable that warning after 13 years, at least.

I'm not so sure about -Wno-parentheses-equality, because that might give
quite a few false positives, especially in old, contributed code.

-Dimitry



signature.asc
Description: Message signed with OpenPGP using GPGMail


Re: [HEADS-UP] Problem with clang in 9-stable [was: r268244 (stable/9) seems to break sysctl hw.ncpu]

2014-07-05 Thread David Chisnall
On 5 Jul 2014, at 14:07, Dimitry Andric d...@freebsd.org wrote:

 Interestingly, -Wno-uninitialized has been in bsd.sys.mk since r76861,
 and the accompanying comment (XXX Delete -Wuninitialized by default for
 now -- the compiler doesn't always get it right) has never been
 changed. :-)
 
 It is probably time to re-enable that warning after 13 years, at least.

It probably only wants enabling for clang.  GCC (at least, GCC 4.2.1) performs 
this analysis based on analyses run by the optimisers and so the warnings are 
dependent on optimisation level.

David

___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org


Re: [HEADS-UP] Problem with clang in 9-stable [was: r268244 (stable/9) seems to break sysctl hw.ncpu]

2014-07-04 Thread David Wolfskill
On Fri, Jul 04, 2014 at 08:09:21PM +0200, Hans Petter Selasky wrote:
 Hi,
 
 Can you try to reproduce this:
 
 Checkout 9-stable's src/sbin/sysctl only.
 
 cd /usr/9-stable/src/sbin/sysctl
 
 env CC=clang make clean all
 ./sysctl -n hw.ncpu
 
 Then:
 
 env CC=gcc make clean all
 ./sysctl -n hw.ncpu
 
 
 clang --version
 FreeBSD clang version 3.2 (tags/RELEASE_32/final 170710) 20121221
 Target: x86_64-unknown-freebsd9.1
 Thread model: posix
 
 gcc --version
 gcc (GCC) 4.2.1 20070831 patched [FreeBSD]
 Copyright (C) 2007 Free Software Foundation, Inc.
 

Hmm... I should have mentioned this part earlier (sorry!): I've been
using clang as the system compiler since about ... autumn 2012 (long ago
enough that I forgot that it might be odd for stable/9).

 When compiling the code with clang, no output is happening. When 
 compiling with gcc, the FreeBSD 9-stable sysctl operates correctly. How 
 do we proceed?
 
 Can more people check this and reproduce?

Well, here's what I see:

g1-252(9.3-P)[22] svn co file:///svn/freebsd/src/base/stable/9/sbin/sysctl
Asysctl/sysctl.c
Asysctl/sysctl.8
Asysctl/Makefile
 U   sysctl
Checked out revision 268249.
g1-252(9.3-P)[23] cd sysctl/
g1-252(9.3-P)[24] env CC=clang make clean all
rm -f sysctl sysctl.o sysctl.8.gz sysctl.8.cat.gz
Warning: Object directory not changed from original /tmp/hps/sysctl
clang -O2 -pipe  -std=gnu99 -Qunused-arguments  -fstack-protector 
-Wsystem-headers -Werror -Wall -Wno-format-y2k -W -Wno-unused-parameter 
-Wstrict-prototypes -Wmissing-prototypes -Wpointer-arith -Wno-uninitialized 
-Wno-pointer-sign -Wno-empty-body -Wno-string-plus-int 
-Wno-unused-const-variable -Wno-tautological-compare -Wno-unused-value 
-Wno-parentheses-equality -Wno-unused-function -Wno-enum-conversion -c sysctl.c
clang -O2 -pipe  -std=gnu99 -Qunused-arguments  -fstack-protector 
-Wsystem-headers -Werror -Wall -Wno-format-y2k -W -Wno-unused-parameter 
-Wstrict-prototypes -Wmissing-prototypes -Wpointer-arith -Wno-uninitialized 
-Wno-pointer-sign -Wno-empty-body -Wno-string-plus-int 
-Wno-unused-const-variable -Wno-tautological-compare -Wno-unused-value 
-Wno-parentheses-equality -Wno-unused-function -Wno-enum-conversion  -o sysctl 
sysctl.o 
gzip -cn sysctl.8  sysctl.8.gz
g1-252(9.3-P)[25] ./sysctl -n hw.ncpu

g1-252(9.3-P)[26] env CC=gcc make clean all
rm -f sysctl sysctl.o sysctl.8.gz sysctl.8.cat.gz
Warning: Object directory not changed from original /tmp/hps/sysctl
clang -O2 -pipe  -std=gnu99 -Qunused-arguments  -fstack-protector 
-Wsystem-headers -Werror -Wall -Wno-format-y2k -W -Wno-unused-parameter 
-Wstrict-prototypes -Wmissing-prototypes -Wpointer-arith -Wno-uninitialized 
-Wno-pointer-sign -Wno-empty-body -Wno-string-plus-int 
-Wno-unused-const-variable -Wno-tautological-compare -Wno-unused-value 
-Wno-parentheses-equality -Wno-unused-function -Wno-enum-conversion -c sysctl.c
clang -O2 -pipe  -std=gnu99 -Qunused-arguments  -fstack-protector 
-Wsystem-headers -Werror -Wall -Wno-format-y2k -W -Wno-unused-parameter 
-Wstrict-prototypes -Wmissing-prototypes -Wpointer-arith -Wno-uninitialized 
-Wno-pointer-sign -Wno-empty-body -Wno-string-plus-int 
-Wno-unused-const-variable -Wno-tautological-compare -Wno-unused-value 
-Wno-parentheses-equality -Wno-unused-function -Wno-enum-conversion  -o sysctl 
sysctl.o 
gzip -cn sysctl.8  sysctl.8.gz
g1-252(9.3-P)[27] ./sysctl -n hw.ncpu

g1-252(9.3-P)[28] 

 When compiling the -10 and -11 version of sysctl with clang and gcc, 
 everything is fine!

That's moderately seriously kinky  And not in a Good Way. :-/

 Thank you!

At least I have some evidence that I might not be merely hallucinating

Peace,
david
-- 
David H. Wolfskill  da...@catwhisker.org
Taliban: Evil cowards with guns afraid of truth from a 14-year old girl.

See http://www.catwhisker.org/~david/publickey.gpg for my public key.


pgpUqV_GqtL6t.pgp
Description: PGP signature


Re: [HEADS-UP] Problem with clang in 9-stable [was: r268244 (stable/9) seems to break sysctl hw.ncpu]

2014-07-04 Thread Hans Petter Selasky

Hi,

I've reverted the MFC of r267960:
http://svnweb.freebsd.org/changeset/base/268263

I did some checking and the patch makes access to a non-initialized 
variable, due to other missing patches prior to mine, probably making 
compiler differences show up on my side. It appears the patch cannot be 
ported to 9-stable then. So I'll just leave it there.


David: Thank you for your error-report.

--HPS
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to freebsd-current-unsubscr...@freebsd.org