835050 - intel-gcm.s fails to compile with clang 3.2

Status: RESOLVED FIXED

(NSS :: Build, defect, P1)

Description

[Mats Palmgren (inactive)]

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 ...

Comment 1

[Mats Palmgren (inactive)]

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

Comment 2

[Mats Palmgren (inactive)]

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

Comment 3

[Wan-Teh Chang]

Attachment: Patch: don't use clang's integrated assembler

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

Comment 4

[Wan-Teh Chang]

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.

Comment 5

[Wan-Teh Chang]

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 6

[Mats Palmgren (inactive)]

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 7

[Kai Engert (:KaiE:)]

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.)

Comment 8

[Wan-Teh Chang]

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

Comment 9

[Jan Beich]

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 10

[Wan-Teh Chang]

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.

Comment 11

[Wan-Teh Chang]

(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.

Comment 12

[Jan Beich]

(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.

Comment 13

[Jan Beich]

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

Comment 14

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

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

Comment 15

[Wan-Teh Chang]

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?

Comment 16

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

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).