clang -o /OBJDIR/home/mats/moz/inbound3/security/nss/lib/freebl/intel-gcm.o -g -D_POSIX_SOURCE -D_BSD_SOURCE -D_XOPEN_SOURCE -fPIC -DLINUX2_1 -Wall -Werror-implicit-function-declaration -Wno-switch -pipe -DLINUX -Dlinux -DHAVE_STRERROR -DXP_UNIX -DSHLIB_SUFFIX=\"so\" -DSHLIB_PREFIX=\"lib\" -DSHLIB_VERSION=\"3\" -DSOFTOKEN_SHLIB_VERSION=\"3\" -DRIJNDAEL_INCLUDE_TABLES -DDEBUG -UNDEBUG -DDEBUG_mats -D_REENTRANT -DNSS_ENABLE_ECC -DUSE_UTIL_DIRECTLY -DNSS_USE_64 -DNSS_X86_OR_X64 -DNSS_X64 -DNSS_BEVAND_ARCFOUR -DMPI_AMD64 -DMP_ASSEMBLY_MULTIPLY -DNSS_USE_COMBA -DMP_CHAR_STORE_SLOW -DMP_IS_LITTLE_ENDIAN -DUSE_HW_AES -DMP_API_COMPATIBLE -I/OBJDIR/home/mats/moz/inbound3/dist/include/nspr -I/OBJDIR/home/mats/moz/inbound3/dist/include/nspr -I/md1/OBJDIR/home/mats/moz/inbound3/security/build/../../dist/public/nss -I/md1/OBJDIR/home/mats/moz/inbound3/security/build/../../dist/private/nss -Impi -Iecl -march=opteron -m64 -fPIC -Wa,--noexecstack -c intel-gcm.s clang: warning: argument unused during compilation: '-D _POSIX_SOURCE' clang: warning: argument unused during compilation: '-D _BSD_SOURCE' clang: warning: argument unused during compilation: '-D _XOPEN_SOURCE' clang: warning: argument unused during compilation: '-fPIC' clang: warning: argument unused during compilation: '-D LINUX2_1' clang: warning: argument unused during compilation: '-Wall' clang: warning: argument unused during compilation: '-Werror-implicit-function-declaration' clang: warning: argument unused during compilation: '-Wno-switch' clang: warning: argument unused during compilation: '-D LINUX' clang: warning: argument unused during compilation: '-D linux' clang: warning: argument unused during compilation: '-D HAVE_STRERROR' clang: warning: argument unused during compilation: '-D XP_UNIX' clang: warning: argument unused during compilation: '-D SHLIB_SUFFIX="so"' clang: warning: argument unused during compilation: '-D SHLIB_PREFIX="lib"' clang: warning: argument unused during compilation: '-D SHLIB_VERSION="3"' clang: warning: argument unused during compilation: '-D SOFTOKEN_SHLIB_VERSION="3"' clang: warning: argument unused during compilation: '-D RIJNDAEL_INCLUDE_TABLES' clang: warning: argument unused during compilation: '-D DEBUG' clang: warning: argument unused during compilation: '-U NDEBUG' clang: warning: argument unused during compilation: '-D DEBUG_mats' clang: warning: argument unused during compilation: '-D _REENTRANT' clang: warning: argument unused during compilation: '-D NSS_ENABLE_ECC' clang: warning: argument unused during compilation: '-D USE_UTIL_DIRECTLY' clang: warning: argument unused during compilation: '-D NSS_USE_64' clang: warning: argument unused during compilation: '-D NSS_X86_OR_X64' clang: warning: argument unused during compilation: '-D NSS_X64' clang: warning: argument unused during compilation: '-D NSS_BEVAND_ARCFOUR' clang: warning: argument unused during compilation: '-D MPI_AMD64' clang: warning: argument unused during compilation: '-D MP_ASSEMBLY_MULTIPLY' clang: warning: argument unused during compilation: '-D NSS_USE_COMBA' clang: warning: argument unused during compilation: '-D MP_CHAR_STORE_SLOW' clang: warning: argument unused during compilation: '-D MP_IS_LITTLE_ENDIAN' clang: warning: argument unused during compilation: '-D USE_HW_AES' clang: warning: argument unused during compilation: '-D MP_API_COMPATIBLE' clang: warning: argument unused during compilation: '-I /OBJDIR/home/mats/moz/inbound3/dist/include/nspr' clang: warning: argument unused during compilation: '-I /OBJDIR/home/mats/moz/inbound3/dist/include/nspr' clang: warning: argument unused during compilation: '-I /md1/OBJDIR/home/mats/moz/inbound3/security/build/../../dist/public/nss' clang: warning: argument unused during compilation: '-I /md1/OBJDIR/home/mats/moz/inbound3/security/build/../../dist/private/nss' clang: warning: argument unused during compilation: '-I mpi' clang: warning: argument unused during compilation: '-I ecl' clang: warning: argument unused during compilation: '-march=opteron' clang: warning: argument unused during compilation: '-fPIC' intel-gcm.s:24:13: error: unknown token in expression .set Htbl, %rdi ^ intel-gcm.s:25:11: error: unknown token in expression .set Tp, %rsi ^ intel-gcm.s:26:13: error: unknown token in expression .set Mlen, %rdx ^ intel-gcm.s:27:13: error: unknown token in expression .set Alen, %rcx ^ intel-gcm.s:28:11: error: unknown token in expression .set X0, %r8 ^ intel-gcm.s:29:12: error: unknown token in expression .set TAG, %r9 ^ intel-gcm.s:31:8: error: unknown token in expression .set T,%xmm0 ^ ... etc ...

# clang --version clang version 3.2 (tags/RELEASE_32/final) Target: x86_64-unknown-linux-gnu Thread model: posix

I'm guessing it's a regression from bug 834741.

Mats: thank you for the bug report. Could you test this patch?

Raphael: could you take a look at this bug? clang's integrated assembler does not support % in the expression part of a .set directive: .set symbol, expression Here is an excerpt of the assembly code in NSS's intel-gcm.s file: ################################################################################ # Generates the final GCM tag # void intel_aes_gcmTAG(uint8_t Htbl[16*16], uint8_t *Tp, uint64_t Mlen, uint64_t Alen, uint8_t* X0, uint8_t* TAG); .type intel_aes_gcmTAG,@function .globl intel_aes_gcmTAG .align 16 intel_aes_gcmTAG: .set Htbl, %rdi .set Tp, %rsi .set Mlen, %rdx .set Alen, %rcx .set X0, %r8 .set TAG, %r9 .set T,%xmm0 .set TMP0,%xmm1 vmovdqu (Tp), T vpshufb .Lbswap_mask(%rip), T, T vpxor TMP0, TMP0, TMP0 shl $3, Mlen shl $3, Alen vpinsrq $0, Mlen, TMP0, TMP0 vpinsrq $1, Alen, TMP0, TMP0 vpxor TMP0, T, T vmovdqu (Htbl), TMP0 call GFMUL vpshufb .Lbswap_mask(%rip), T, T vpxor (X0), T, T vmovdqu T, (TAG) ret .size intel_aes_gcmTAG, .-intel_aes_gcmTAG The .set directive is used to give symbolic names to registers, which is why % appears in the expression part. This seems like a good use case. Do you know if there is a good reason for clang's integrated assembler to disallow % in the expression of a .set directive? Or is it just not yet implemented? Thanks for your advice. I hope clang will support this use case. Note: I made three attempts to work around this, but none of them works. 1. I omitted % in the .set directive and added % when the symbol was used: .set Htbl, rdi vmovdqu (%Htbl), TMP0 clang's integrated assembler says: intel-gcm.s:42:15: error: invalid register name vmovdqu (%Htbl), TMP0 ^ 2. I added % to escape %. .set Htbl, %%rdi 3. I added \ to escape %. .set Htbl, \%rdi clang doesn't allow either of these. So I ended up passing the -no-integrated-as flag to clang.

Raphael: looking at the GNU Assembler manual more closely, I think the way intel-gcm.s uses the .set directive does not match what is documented in http://www.sourceware.org/binutils/docs-2.12/as.info/Set.html#Set It seems that "symbol" has a very specific meaning to the .set directive. It seems to mean a symbol to be resolved by the linker. But apparently GNU Assembler allows .set to be used to define symbolic names as if they were macros. So perhaps it's the GNU Assebler manual that needs to be updated?

Comment on attachment 706793 [details] [diff] [review] Patch: don't use clang's integrated assembler Yes, this works fine. Thanks for the quick response.

Comment on attachment 706793 [details] [diff] [review] Patch: don't use clang's integrated assembler r=kaie if you want to check this in to NSS. (Once that's done and NSS test machines pass, we can tag a NSS beta3 and give it to mozilla-central.)

Kai: thank you for the review. I checked in the patch (with a comment) in bug 805604, where intel-gcm.s was added.

Comment on attachment 706793 [details] [diff] [review] Patch: don't use clang's integrated assembler Review of attachment 706793 [details] [diff] [review]: ----------------------------------------------------------------- I think Solaris may need same workaround unless clang can never be used there. ::: mozilla/security/nss/lib/freebl/Makefile @@ +188,4 @@ > # comment the next two lines to turn off intel HW accelleration > DEFINES += -DUSE_HW_AES > ASFILES += intel-aes.s intel-gcm.s > + ifneq (,$(findstring clang,$(AS))) While AS=cc may be not common on Linux it's on FreeBSD where USE_HW_AES also makes sense. @@ +188,5 @@ > # comment the next two lines to turn off intel HW accelleration > DEFINES += -DUSE_HW_AES > ASFILES += intel-aes.s intel-gcm.s > + ifneq (,$(findstring clang,$(AS))) > + ASFLAGS += -no-integrated-as I would limit to one file: $(OBJDIR)/intel-gcm.o: ASFLAGS += -no-integrated-as

Comment on attachment 706793 [details] [diff] [review] Patch: don't use clang's integrated assembler Review of attachment 706793 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozilla/security/nss/lib/freebl/Makefile @@ +188,4 @@ > # comment the next two lines to turn off intel HW accelleration > DEFINES += -DUSE_HW_AES > ASFILES += intel-aes.s intel-gcm.s > + ifneq (,$(findstring clang,$(AS))) Jan: I don't understand what you said here. Could you please elaborate? Let me explain what this code does. By default, NSS uses the C compiler $(CC) as the assembler: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/coreconf/command.mk&rev=1.14&mark=11-12 On Linux, NSS does not override this default. This is why I am searching for "clang" (the C compiler name) in $(AS) here. @@ +188,5 @@ > # comment the next two lines to turn off intel HW accelleration > DEFINES += -DUSE_HW_AES > ASFILES += intel-aes.s intel-gcm.s > + ifneq (,$(findstring clang,$(AS))) > + ASFLAGS += -no-integrated-as I didn't know we can do this in GNU make. Thanks for the suggestion! I created a new patch in bug 805604, which also simplifies how we compile intel-gcm-wrap.c with an extra -mssse3 C flag using the same trick. My only concern is whether this is a new feature of GNU make and whether this will require us to use GNU make newer than a certain version to build NSS. If you know the answer, please let us know in my new patch in bug 805604. Thanks.

(In reply to Wan-Teh Chang from comment #10) > > My only concern is whether this is a new feature of GNU make and > whether this will require us to use GNU make newer than a certain > version to build NSS. If you know the answer, please let us know > in my new patch in bug 805604. Thanks. I found that this feature is "target-specific variable values" and it is documented in GNU Make 3.77 manual: ftp://ftp.gnu.org/old-gnu/Manuals/make-3.77/html_node/make_69.html#SEC68 (It is not documented in GNU Make 3.75 manual.) GNU Make 3.77 was released on May 1998. It should be safe to use this feature.

(In reply to Wan-Teh Chang from comment #10) >(In reply to Jan Beich from comment #9) > ::: mozilla/security/nss/lib/freebl/Makefile > @@ +188,4 @@ > > # comment the next two lines to turn off intel HW accelleration > > DEFINES += -DUSE_HW_AES > > ASFILES += intel-aes.s intel-gcm.s > > + ifneq (,$(findstring clang,$(AS))) > > While AS=cc may be not common on Linux it's on FreeBSD where USE_HW_AES also > > makes sense. > Jan: I don't understand what you said here. Could you please elaborate? s/AS=cc/clang as cc/ - http://svnweb.freebsd.org/changeset/base/246123 FreeBSD 10.0 - 'cc' is 'clang' by default, can be overriden WITHOUT_CLANG_IS_CC FreeBSD 9.1 - 'cc' is 'gcc' by default, can be overriden WITH_CLANG_IS_CC I think OS X also has clang as cc since 10.7 "Lion" but otherwise irrelevant to this bug.

Damn, wrong url. http://svnweb.freebsd.org/changeset/base/242624

I reported llvm.org/pr15168 for the missing "feature".

Rafael: thank you for filing a bug report at llvm.org/bugs. Is there a way to give meaningful aliases to registers without using the .set directive?

I don't normally code a lot in assembly, but the two options I can think of (other than fixing the llvm bug) are: * using a .macro. This requires defining the entire command, not just the register name. * using a .S file (which gets preprocessed with cpp).