Closed
Bug 1250891
Opened 8 years ago
Closed 8 years ago
poly1305-donna-x64-sse2-incremental-source.c:29:24: error: expected ';' after top level declarator
Categories
(NSS :: Build, defect)
Tracking
(firefox47 affected)
RESOLVED
FIXED
3.24
Tracking | Status | |
---|---|---|
firefox47 | --- | affected |
People
(Reporter: jbeich, Assigned: jbeich)
References
Details
Attachments
(2 files, 3 obsolete files)
2.05 KB,
patch
|
glandium
:
review+
gaston
:
feedback+
martin
:
feedback+
KaiE
:
checked-in+
|
Details | Diff | Splinter Review |
4.40 KB,
patch
|
glandium
:
review+
franziskus
:
feedback+
franziskus
:
checked-in+
|
Details | Diff | Splinter Review |
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
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
Attachment #8723028 -
Flags: feedback?(martin)
Attachment #8723028 -
Flags: feedback?(landry)
Comment 3•8 years ago
|
||
Comment on attachment 8723009 [details] [diff] [review] clang detection, v0 Review of attachment 8723009 [details] [diff] [review]: ----------------------------------------------------------------- LGTM
Updated•8 years ago
|
Attachment #8723028 -
Flags: feedback?(martin) → feedback+
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.
Summary: poly1305-donna-x64-sse2-incremental-source.c:29:24: error: expected ';' after top level declarator (CC=cc) → poly1305-donna-x64-sse2-incremental-source.c:29:24: error: expected ';' after top level declarator
Attachment #8723028 -
Flags: review?(mh+mozilla)
Attachment #8723009 -
Flags: review?(mh+mozilla)
Comment 5•8 years ago
|
||
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.
Attachment #8723028 -
Flags: feedback?(landry) → feedback+
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?
Attachment #8723009 -
Flags: feedback?(landry)
Comment 7•8 years ago
|
||
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.
Attachment #8723009 -
Flags: review?(mh+mozilla) → review-
Updated•8 years ago
|
Attachment #8723028 -
Flags: review?(mh+mozilla) → review+
Comment 8•8 years ago
|
||
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•8 years ago
|
||
Comment on attachment 8723028 [details] [diff] [review] no -ansi, v0 https://hg.mozilla.org/projects/nss/rev/295f90dcb39a
Attachment #8723028 -
Flags: checked-in+
Comment 10•8 years ago
|
||
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
Updated•8 years ago
|
Target Milestone: --- → 3.24
Comment 11•8 years ago
|
||
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.
Attachment #8723009 -
Flags: feedback?(landry) → feedback+
Assignee | ||
Comment 12•8 years ago
|
||
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 [...]
Attachment #8723009 -
Attachment is obsolete: true
Attachment #8727206 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 13•8 years ago
|
||
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"
Attachment #8727206 -
Attachment is obsolete: true
Attachment #8727206 -
Flags: review?(mh+mozilla)
Attachment #8727209 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 14•8 years ago
|
||
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
Attachment #8727209 -
Attachment is obsolete: true
Attachment #8727209 -
Flags: review?(mh+mozilla)
Attachment #8727206 -
Attachment is obsolete: false
Attachment #8727206 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 15•8 years ago
|
||
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
Updated•8 years ago
|
Attachment #8727206 -
Flags: review?(mh+mozilla) → review+
Keywords: checkin-needed
Assignee | ||
Comment 16•8 years ago
|
||
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.
Flags: needinfo?(landry)
Updated•8 years ago
|
Assignee: nobody → jbeich
Status: NEW → ASSIGNED
Assignee | ||
Comment 17•8 years ago
|
||
Comment on attachment 8727206 [details] [diff] [review] clang detection, v1 Can someone else check the patch in?
Flags: needinfo?(landry)
Attachment #8727206 -
Flags: checked-in?
Comment 18•8 years ago
|
||
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
Flags: needinfo?(jbeich)
Assignee | ||
Comment 19•8 years ago
|
||
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
Attachment #8727206 -
Attachment is obsolete: true
Attachment #8727206 -
Flags: checked-in?
Flags: needinfo?(jbeich)
Attachment #8736375 -
Flags: review?(mh+mozilla)
Attachment #8736375 -
Flags: feedback?(franziskuskiefer)
Comment 20•8 years ago
|
||
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
Attachment #8736375 -
Flags: feedback?(franziskuskiefer)
Attachment #8736375 -
Flags: feedback+
Attachment #8736375 -
Flags: checked-in+
Updated•8 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Updated•8 years ago
|
Attachment #8736375 -
Flags: review?(mh+mozilla) → review+
Updated•8 years ago
|
Keywords: checkin-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•