1250891 - poly1305-donna-x64-sse2-incremental-source.c:29:24: error: expected ';' after top level declarator

Status: RESOLVED FIXED

(NSS :: Build, defect)

Description

[Jan Beich]

coreconf/Werror.mk maybe not recognize Clang when it's called something else.

  $ clang37 -\? 2>&1 >/dev/null | sed -e 's/:.*//;1q'
  clang-3.7

  $ cc -\? 2>&1 >/dev/null | sed -e 's/:.*//;1q'
  cc

  $ clang37
  clang version 3.7.1 (tags/RELEASE_371/final)
  Target: x86_64-unknown-freebsd11.0
  Thread model: posix

  $ cc -v
  FreeBSD clang version 3.8.0 (branches/release_38 260756) (based on LLVM 3.8.0)
  Target: x86_64-unknown-freebsd11.0
  Thread model: posix
  InstalledDir: /usr/bin

  poly1305-donna-x64-sse2-incremental-source.c:29:24: error: expected ';' after top level declarator
  static uint128_t INLINE
			 ^
			 ;
  poly1305-donna-x64-sse2-incremental-source.c:68:2: error: unknown type name 'poly1305_power'
	  poly1305_power P[2];     /* 288 bytes, top 32 bit halves unused = 144 bytes of free storage */
	  ^
  poly1305-donna-x64-sse2-incremental-source.c:80:32: error: redefinition of 'inline' with a different type: 'poly1305_state_internal' (aka 'struct poly1305_state_internal_t') vs 'uint128_t' (aka 'unsigned __int128')
  static poly1305_state_internal INLINE
				 ^
  poly1305-donna-x64-sse2-incremental-source.c:17:16: note: expanded from macro 'INLINE'
  #define INLINE inline
		 ^
  poly1305-donna-x64-sse2-incremental-source.c:29:18: note: previous definition is here
  static uint128_t INLINE
		   ^
  poly1305-donna-x64-sse2-incremental-source.c:17:16: note: expanded from macro 'INLINE'
  #define INLINE inline
		 ^
  poly1305-donna-x64-sse2-incremental-source.c:80:38: error: expected ';' after top level declarator
  static poly1305_state_internal INLINE
				       ^
				       ;
  4 errors generated.

http://buildbot.rhaalovely.net/builders/mozilla-central-freebsd-amd64/builds/759/steps/build/logs/stdio

Comment 1

[Jan Beich]

Attachment: clang detection, v0

Comment 2

[Jan Beich]

Attachment: no -ansi, v0

clang -ansi doesn't allow C99 inline but GNU __inline__ works. Linux got rid of -ansi per bug 772144 comment 13.

NSS itself is a mixed bag of both:
./lib/freebl/sha_fast.h:static __inline__ PRUint32 swap4b(PRUint32 value)
./lib/freebl/sha512.c:static __inline__ PRUint32 swap4b(PRUint32 value)
./lib/freebl/mpi/mpcpucache.c:static inline void dcbzl(char *array)
./lib/freebl/poly1305-donna-x64-sse2-incremental-source.c:static uint128_t INLINE

Comment 3

[Martin Thomson [:mt:]]

Comment on attachment 8723009 [details] [diff] [review]
clang detection, v0

Review of attachment 8723009 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 4

[Jan Beich]

To clarify, this bug is actually about two issues, both triggered by bug 917571. CC=cc (default) builds fine but skips poly1305-donna-x64-sse2-incremental-source.c if 'cc' is Clang while CC=clang (buildbot) is broken by inlines with -ansi.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=068e60c5083f

(In reply to Martin Thomson [:mt:] from comment #3)
> LGTM

Please, use attachment flags. Assuming f+ and passing to a build peer.

Comment 5

[Landry Breuil (:gaston)]

Comment on attachment 8723028 [details] [diff] [review]
no -ansi, v0

I havent seen this issue on my buildbot with clang ~3.4 but the diff seems harmless to me.

Comment 6

[Jan Beich]

Comment on attachment 8723009 [details] [diff] [review]
clang detection, v0

(In reply to Landry Breuil (:gaston) from comment #5)
> I havent seen this issue on my buildbot with clang ~3.4 but the diff seems
> harmless to me.

Based on the following log line the bot probably uses |CC_NAME=clang-3.5| and fails on |ifeq ($(CC_NAME),clang)|, so the affected file isn't built.

  clang-3.5: warning: argument unused during compilation: '-ansi'

Can you check exposing __int128 usage won't break your build?

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8723009 [details] [diff] [review]
clang detection, v0

Review of attachment 8723009 [details] [diff] [review]:
-----------------------------------------------------------------

::: coreconf/Werror.mk
@@ +7,5 @@
>  
> +ifndef CC_IS_CLANG
> +  CC_IS_CLANG := $(and $(findstring clang, $(shell $(CC) --version 2>&1)), 1)
> +  # Export CC_IS_CLANG to save a shell invocation when recursing.
> +  export CC_IS_CLANG

clang passes the CC_IS_GCC test, right? You could add a

ifdef CC_IS_CLANG
CC_IS_GCC = 1
endif

then.

::: lib/freebl/Makefile
@@ +696,5 @@
>  # expression of a .set directive. intel-gcm.s uses .set to give
>  # symbolic names to registers, for example,
>  #     .set  Htbl, %rdi
>  # So we can't use Clang's integrated assembler with intel-gcm.s.
> +ifdef CC_IS_CLANG

That doesn't imply that AS is CC.

Comment 8

[Landry Breuil (:gaston)]

And fwiw, i'm all for removing -ansi, since that breaks nss build since 3.21.1/3.23 as c++-style comments are not allowed with -ansi (which implies -std=c90), so removing -ansi will allow c99 (which is now a blessed requirement for nss)

Comment 9

[Kai Engert [:KaiE:]]

Comment on attachment 8723028 [details] [diff] [review]
no -ansi, v0

https://hg.mozilla.org/projects/nss/rev/295f90dcb39a

Comment 10

[Kai Engert [:KaiE:]]

Comment on attachment 8723028 [details] [diff] [review]
no -ansi, v0

I've accidentally checked this in to the NSS_3_23_BRANCH, too.
Since it isn't a code change, I'm not backing it out.
https://hg.mozilla.org/projects/nss/rev/f34953220e06

Comment 11

[Landry Breuil (:gaston)]

Comment on attachment 8723009 [details] [diff] [review]
clang detection, v0

I havent seen this build failure, but fwiw on OpenBSD when building a port with clang, CC points to a symlink to clang, named cc, so a patch like this would be welcome for clarity. 

c++@ -> /usr/local/bin/clang++
cc@ -> /usr/local/bin/clang
g++@ -> /usr/local/bin/clang++
gcc@ -> /usr/local/bin/clang

On the buildbot, CC=clang without such indirections.

Comment 12

[Jan Beich]

Attachment: clang detection, v1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8cbe4a75b572

(In reply to Mike Hommey [:glandium] from comment #7)
> ::: lib/freebl/Makefile
[...]
> > -ifneq (,$(findstring clang,$(shell $(AS) --version)))
> > +ifdef CC_IS_CLANG
> >  $(OBJDIR)/$(PROG_PREFIX)intel-gcm$(OBJ_SUFFIX): ASFLAGS += -no-integrated-as
> >  endif
> >  endif
> 
> That doesn't imply that AS is CC.

-fno-integrated-as is only supported by Clang, so the following

  ifdef CC_IS_CLANG
  ifeq ($(AS), $(CC))
  $(OBJDIR)/$(PROG_PREFIX)intel-gcm$(OBJ_SUFFIX): ASFLAGS += -no-integrated-as
  endif
  endif # AS=$(CC)
  endif # CC_IS_CLANG

can be optimized into

  ifdef CC_IS_CLANG
  $(OBJDIR)/$(PROG_PREFIX)intel-gcm$(OBJ_SUFFIX): CFLAGS += -no-integrated-as
  endif
  endif # CC_IS_CLANG

NSS doesn't actually support $(AS) != $(CC) case. The most obvious is lack of such a conditional in coreconf/command.mk. The rest are just incompatible flags.

  coreconf/HP-UX.mk:ASFLAGS   += -x assembler-with-cpp
  coreconf/ReliantUNIX.mk:	ASFLAGS     += -x assembler-with-cpp
  coreconf/SunOS5.mk:	ASFLAGS	  += -x assembler-with-cpp
  coreconf/SunOS5.mk:	ASFLAGS   += -Wa,-P
  coreconf/command.mk:ASFLAGS      += $(CFLAGS)
  lib/freebl/Makefile:	    ASFLAGS += -march=opteron -m64 -fPIC
  lib/freebl/Makefile:	ASFLAGS = -Wp,-P -Wp,-traditional -O -mips3
  lib/freebl/Makefile:	ASFLAGS = -x assembler-with-cpp
  lib/freebl/Makefile:    ASFLAGS += -fPIC -Wa,--noexecstack

  $ as -x assembler-with-cpp a.s
  Assembler messages:
  Error: can't open assembler-with-cpp for reading: No such file or directory

  $ as -march=opteron -m64 -fPIC a.s
  as: unrecognized option `-m64'

  $ as -fPIC -Wa,--noexecstack a.s
  as: unrecognized option `-PIC'

  $ as --version
  GNU assembler (GNU Binutils) 2.25.1
  [...]

Comment 13

[Jan Beich]

Attachment: clang detection, v1.1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d4ece955e23

-no-integrated-as seems to be an old spelling for -fno-integrated-as but still used in -cc1 mode. Let's convert as we're touching the line, anyway.

$ clang --help | fgrep integrated
  -fintegrated-as         Enable the integrated assembler
  -fno-integrated-as      Disable the integrated assembler

$ fgrep -r no-integrated mozilla-central/
mozilla-central/old-configure.in:dnl Support for -fno-integrated-as (recent clang)
mozilla-central/old-configure.in:dnl Under clang 3.4+, use -fno-integrated-as to
mozilla-central/old-configure.in:CFLAGS="$CFLAGS -fno-integrated-as -S"
mozilla-central/old-configure.in:AC_MSG_CHECKING([whether C compiler supports -fno-integrated-as])
mozilla-central/old-configure.in:               [ NO_INTEGRATED_AS_CFLAGS="-fno-integrated-as"

Comment 14

[Jan Beich]

Comment on attachment 8727209 [details] [diff] [review]
clang detection, v1.1

ASAN builders use clang < 3.5 which lacks -fno-integrated-as. Tooltool says "clang_version": "r200213" which is from 2014-01-27 clang trunk for 3.5.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a5b8601fd19

Comment 15

[Jan Beich]

Comment on attachment 8727206 [details] [diff] [review]
clang detection, v1

ASAN Try missed in comment 12: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b95d37357d91

Comment 16

[Jan Beich]

Landry, can you check it in NSS repo? No rush to merge into mozilla-central unless you cannot wait for poly1305-donna-x64-sse2-incremental-source.c being built on OpenBSD amd64.

(In reply to Landry Breuil (:gaston) from comment #11)
> CC points to a symlink to clang, named cc, so a patch like this

It doesn't matter. CC_NAME is derived from error message where it's usually a basename of the hardlink. My guess, /usr/local/bin/clang++ is a symlink on OpenBSD that points somewhere else where it currently ends with clang-3.7.

Comment 17

[Jan Beich]

Comment on attachment 8727206 [details] [diff] [review]
clang detection, v1

Can someone else check the patch in?

Comment 18

[Franziskus Kiefer [:franziskus]]

I rebased, landed, and backed out again because this fails on sunos [1].

https://hg.mozilla.org/projects/nss/rev/ec517552a5c2
https://hg.mozilla.org/projects/nss/rev/c7cec7602c08

[1] https://bot.nss-crypto.org:8011/builders/2-sunos-x32-OPT/builds/1082/steps/shell/logs/stdio

Comment 19

[Jan Beich]

Attachment: clang detection, v2

CC_IS_GCC was always defined, surprisingly without breaking MSVC builds. Can you try the new version?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=59d0322ed215

Comment 20

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8736375 [details] [diff] [review]
clang detection, v2

Review of attachment 8736375 [details] [diff] [review]:
-----------------------------------------------------------------

this looks better.
I landed it because I don't have a sun machine to test. The buildbots seem to be ok with this as well.

https://hg.mozilla.org/projects/nss/rev/e4ac9376cd28