X86AsmBackend::WriteNopData uses long nops unconditionally

[Quuxplusone]

Bugzilla Link	PR11212
Status	RESOLVED FIXED
Importance	P normal
Reported by	Roman Divacky (rdivacky@freebsd.org)
Reported on	2011-10-23 05:06:53 -0700
Last modified on	2014-06-28 06:03:52 -0700
Version	trunk
Hardware	PC FreeBSD
CC	cb@cebix.net, dimitry@andric.com, freebsd@nagilum.org, geek4civic@gmail.com, llvm-bugs@lists.llvm.org, llvm@patater.com, patfbsd@davenulle.org, rafael@espindo.la
Fixed by commit(s)
Attachments	llvm-nop.patch (3310 bytes, text/plain) llvm-geode.patch (16858 bytes, text/plain)
Blocks
Blocked by
See also

X86AsmBackend::WriteNopData() uses long NOP encodings unconditionally resulting in illegal instructions being executed on CPUs that dont support these encodings (ie. Amd Geode). We need to check for the target CPU.

According to http://www.tortall.net/projects/yasm/manual/html/arch-x86.html we should even differentiate between amd and intel. Gnu as uses different nops on different cpus too.

[Quuxplusone]

This occurs on an AMD Geode LX processor (not the same as AMD Geode).

CPU: Geode(TM) Integrated Processor by AMD PCS (499.91-MHz 586-class CPU)
  Origin = "AuthenticAMD"  Id = 0x5a2  Family = 5  Model = a  Stepping = 2
  Features=0x88a93d<FPU,DE,PSE,TSC,MSR,CX8,SEP,PGE,CMOV,CLFLUSH,MMX>
  AMD Features=0xc0400000<MMX+,3DNow!+,3DNow!>

Instructions set can be found here (page #619) but this is hard to read:
http://support.amd.com/us/Embedded_TechDocs/33234H_LX_databook.pdf

I will be happy to test patch!
Thanks, regards.

[Quuxplusone]

I think an acceptable solution would be to emit long nops in 64bit mode and for 32bit mode just emit a series of 0x90. Check the attached patch.

[Quuxplusone]

This would be a regression for 32 bits, no? Do you know which cpus don't have the nops we currently use (and which ones they have)? If not, could you make us produce 0x90 only for the geode and add a FIXME?

[Quuxplusone]

There's quite a bit of logic in GNU as to determine the optimal sequence
of NOPs for the particular CPU it is tuning for.

That is, GNU as usually gets the target CPU passed to it, via e.g.
-march=, -mtune, or even .arch directives in the assembly.

In the worst case, we could just emit a sequence of nops, but I have no
idea what the performance impact of this is.

A slightly better approach would be to emit optimal nop sequences that
work on all i386 CPU's.  This can still be done without passing the
target CPU to the assembly stage.

Obviously, the nicest fix would be to inform the assembly stage of the
CPU we're tuning for by default, and select the optimal nop sequence
based on it...

[Quuxplusone]

Attached llvm-nop.patch (3310 bytes, text/plain): emit a series of 0x90 on x86-32

[Quuxplusone]

Attached llvm-geode.patch (16858 bytes, text/plain): patch

[Quuxplusone]

This patch breaks the build for me:

===> usr.bin/clang (all)
===> usr.bin/clang/clang (all)
c++ -Os -O2 -pipe -march=pentium -mtune=pentium -I/export/src/usr.bin/clang/clan
g/../../../contrib/llvm/include -I/export/src/usr.bin/clang/clang/../../../contr
ib/llvm/tools/clang/include -I/export/src/usr.bin/clang/clang/../../../contrib/l
lvm/tools/clang/tools/driver -I. -I/export/src/usr.bin/clang/clang/../../../cont
rib/llvm/../../lib/clang/include -DLLVM_ON_UNIX -DLLVM_ON_FREEBSD -D__STDC_LIMIT
_MACROS -D__STDC_CONSTANT_MACROS -fno-strict-aliasing -DLLVM_DEFAULT_TARGET_TRIP
LE=\"i386-unknown-freebsd10.0\" -DLLVM_HOSTTRIPLE=\"i386-unknown-freebsd10.0\" -
DDEFAULT_SYSROOT=\"\" -fstack-protector -fno-exceptions -fno-rtti -c /export/src
/usr.bin/clang/clang/../../../contrib/llvm/tools/clang/tools/driver/cc1_main.cpp
c++ -Os -O2 -pipe -march=pentium -mtune=pentium -I/export/src/usr.bin/clang/clan
g/../../../contrib/llvm/include -I/export/src/usr.bin/clang/clang/../../../contr
ib/llvm/tools/clang/include -I/export/src/usr.bin/clang/clang/../../../contrib/l
lvm/tools/clang/tools/driver -I. -I/export/src/usr.bin/clang/clang/../../../cont
rib/llvm/../../lib/clang/include -DLLVM_ON_UNIX -DLLVM_ON_FREEBSD -D__STDC_LIMIT
_MACROS -D__STDC_CONSTANT_MACROS -fno-strict-aliasing -DLLVM_DEFAULT_TARGET_TRIP
LE=\"i386-unknown-freebsd10.0\" -DLLVM_HOSTTRIPLE=\"i386-unknown-freebsd10.0\" -
DDEFAULT_SYSROOT=\"\" -fstack-protector -fno-exceptions -fno-rtti -c /export/src
/usr.bin/clang/clang/../../../contrib/llvm/tools/clang/tools/driver/cc1as_main.c
pp
/export/src/usr.bin/clang/clang/../../../contrib/llvm/tools/clang/tools/driver/c
c1as_main.cpp: In function 'bool ExecuteAssembler(<unnamed>::AssemblerInvocation
&, clang::DiagnosticsEngine&)':
/export/src/usr.bin/clang/clang/../../../contrib/llvm/tools/clang/tools/driver/c
c1as_main.cpp:332: error: no matching function for call to 'llvm::Target::create
MCAsmBackend(std::string&) const'
/export/src/usr.bin/clang/clang/../../../contrib/llvm/include/llvm/Support/TargetRegistry.h:358:
note: candidates are: llvm::MCAsmBackend*
llvm::Target::createMCAsmBackend(llvm::StringRef, llvm::StringRef) const
/export/src/usr.bin/clang/clang/../../../contrib/llvm/tools/clang/tools/driver/cc1as_main.cpp:346:
error: no matching function for call to
'llvm::Target::createMCAsmBackend(std::string&) const'
/export/src/usr.bin/clang/clang/../../../contrib/llvm/include/llvm/Support/TargetRegistry.h:358:
note: candidates are: llvm::MCAsmBackend*
llvm::Target::createMCAsmBackend(llvm::StringRef, llvm::StringRef) const
*** [cc1as_main.o] Error code 1

Stop in /export/src/usr.bin/clang/clang.
*** [all] Error code 1

Stop in /export/src/usr.bin/clang.
*** [all] Error code 1

Stop in /export/src/usr.bin.
*** [usr.bin.all__D] Error code 1

Stop in /export/src.
*** [everything] Error code 1

Stop in /export/src.
*** [buildworld] Error code 1

Stop in /export/src.

[Quuxplusone]

Sorry, you need this patch to clang as well.

Index: tools/driver/cc1as_main.cpp
===================================================================
--- tools/driver/cc1as_main.cpp (revision 163997)
+++ tools/driver/cc1as_main.cpp (working copy)
@@ -329,7 +329,7 @@
     MCAsmBackend *MAB = 0;
     if (Opts.ShowEncoding) {
       CE = TheTarget->createMCCodeEmitter(*MCII, *MRI, *STI, Ctx);
-      MAB = TheTarget->createMCAsmBackend(Opts.Triple);
+      MAB = TheTarget->createMCAsmBackend(Opts.Triple, Opts.CPU);
     }
     Str.reset(TheTarget->createAsmStreamer(Ctx, *Out, /*asmverbose*/true,
                                            /*useLoc*/ true,
@@ -343,7 +343,7 @@
     assert(Opts.OutputType == AssemblerInvocation::FT_Obj &&
            "Invalid file type!");
     MCCodeEmitter *CE = TheTarget->createMCCodeEmitter(*MCII, *MRI, *STI, Ctx);
-    MCAsmBackend *MAB = TheTarget->createMCAsmBackend(Opts.Triple);
+    MCAsmBackend *MAB = TheTarget->createMCAsmBackend(Opts.Triple, Opts.CPU);
     Str.reset(TheTarget->createMCObjectStreamer(Opts.Triple, Ctx, *MAB, *Out,
                                                 CE, Opts.RelaxAll,
                                                 Opts.NoExecStack));

[Quuxplusone]

With this patch applied how do I tell llvm I want code for geode?

nagilum@cakebox ~/Projects/C/src/a16 > clang -O2 -g -o a16.clang-O2 a16.c
nagilum@cakebox ~/Projects/C/src/a16 > gdb a16.clang-O2
GNU gdb 6.1.1 [FreeBSD]
Copyright 2004 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for details.
This GDB was configured as "i386-marcel-freebsd"...
(gdb) b main
Breakpoint 1 at 0x804889c: file a16.c, line 77.
(gdb) run
Starting program: /export/home/nagilum/Projects/C/src/a16/a16.clang-O2

Breakpoint 1, main (argc=-1077947040, argv=0xbfbfd560) at a16.c:77
77              time(&mytime);
Current language:  auto; currently minimal
(gdb) next
78              Variations=ipow(DISTVALS, POSITIONS);
(gdb) next
82              mybrain.variations=NULL;
(gdb) next
83              mybrain.rating=NULL;
(gdb) next

Program received signal SIGILL, Illegal instruction.
0x080488db in main (argc=-1077947040, argv=0xbfbfd560) at a16.c:85
85                      printf("Calculating of first guess enabled!\n");
(gdb) info line 85
Line 85 of "a16.c" starts at address 0x80488c6 <main+54>
   and ends at 0x80488e0 <main+80>.
(gdb) disassemble
Dump of assembler code for function main:
0x08048890 <main+0>:    push   %ebp
0x08048891 <main+1>:    mov    %esp,%ebp
0x08048893 <main+3>:    push   %ebx
0x08048894 <main+4>:    push   %edi
0x08048895 <main+5>:    push   %esi
0x08048896 <main+6>:    sub    $0x7c,%esp
0x08048899 <main+9>:    lea    -0x50(%ebp),%eax
0x0804889c <main+12>:   mov    %eax,(%esp)
0x0804889f <main+15>:   call   0x8048604 <time@plt>
0x080488a4 <main+20>:   movl   $0x186a0,0x804a154
0x080488ae <main+30>:   movl   $0x0,-0x58(%ebp)
0x080488b5 <main+37>:   movl   $0x0,-0x54(%ebp)
0x080488bc <main+44>:   movb   $0x1,-0x59(%ebp)
0x080488c0 <main+48>:   cmpl   $0x2,0x8(%ebp)
0x080488c4 <main+52>:   jl     0x80488d6 <main+70>
0x080488c6 <main+54>:   movl   $0x8049e90,(%esp)
0x080488cd <main+61>:   call   0x8048674 <puts@plt>
0x080488d2 <main+66>:   movb   $0x0,-0x59(%ebp)
0x080488d6 <main+70>:   mov    $0x66666667,%esi
 0x080488db <main+75>:   nopl   0x0(%eax,%eax,1)
0x080488e0 <main+80>:   movl   $0x8049ec0,(%esp)
0x080488e7 <main+87>:   call   0x8048674 <puts@plt>
0x080488ec <main+92>:   movl   $0x8049ee0,(%esp)
0x080488f3 <main+99>:   call   0x8048674 <puts@plt>
0x080488f8 <main+104>:  movl   $0x8049f00,(%esp)
0x080488ff <main+111>:  call   0x8048674 <puts@plt>
0x08048904 <main+116>:  movl   $0x8049d1c,(%esp)
0x0804890b <main+123>:  call   0x80485a4 <printf@plt>
0x08048910 <main+128>:  call   0x8049c60 <GetReply>
0x08048915 <main+133>:  cmp    $0x31,%eax
0x08048918 <main+136>:  jne    0x8048a60 <main+464>
0x0804891e <main+142>:  call   0x80485d4 <random@plt>
0x08048923 <main+147>:  mov    %eax,%edi
0x08048925 <main+149>:  imul   %esi
0x08048927 <main+151>:  mov    %edx,%eax
---Type <return> to continue, or q <return> to quit---q
Quit
(gdb) q
The program is running.  Exit anyway? (y or n) y
nagilum@cakebox ~/Projects/C/src/a16 > which clang
/usr/bin/clang
nagilum@cakebox ~/Projects/C/src/a16 > clang --version
FreeBSD clang version 3.2 (trunk 162107) 20120817
Target: i386-unknown-freebsd10.0
Thread model: posix
nagilum@cakebox ~/Projects/C/src/a16 > clang -mtune=geode -march=geode -O2 -g -
o a16.clang-O3 a16.c
'geode' is not a recognized processor for this target (ignoring processor)
'geode' is not a recognized processor for this target (ignoring processor)
'geode' is not a recognized processor for this target (ignoring processor)
nagilum@cakebox ~/Projects/C/src/a16 > clang -### -mtune=geode -march=geode -O2
-g -o a16.clang-O3 a16.c
FreeBSD clang version 3.2 (trunk 162107) 20120817
Target: i386-unknown-freebsd10.0
Thread model: posix
 "/usr/bin/clang" "-cc1" "-triple" "i386-unknown-freebsd10.0" "-emit-obj" "-disable-free" "-main-file-name" "a16.c" "-mrelocation-model" "static" "-mdisable-fp-elim" "-masm-verbose" "-mconstructor-aliases" "-target-cpu" "geode" "-momit-leaf-frame-pointer" "-g" "-resource-dir" "/usr/bin/../lib/clang/3.2" "-fmodule-cache-path" "/var/tmp/clang-module-cache" "-O2" "-fdebug-compilation-dir" "/home/nagilum/Projects/C/src/a16" "-ferror-limit" "19" "-fmessage-length" "80" "-mstackrealign" "-fobjc-runtime=gnustep" "-fdiagnostics-show-option" "-fcolor-diagnostics" "-o" "/tmp/a16-yca4xa.o" "-x" "c" "a16.c"
 "/usr/bin/ld" "--eh-frame-hdr" "-dynamic-linker" "/libexec/ld-elf.so.1" "--hash-style=both" "--enable-new-dtags" "-m" "elf_i386_fbsd" "-o" "a16.clang-O3" "/usr/lib/crt1.o" "/usr/lib/crti.o" "/usr/lib/crtbegin.o" "-L/usr/lib" "/tmp/a16-yca4xa.o" "-lgcc" "--as-needed" "-lgcc_s" "--no-as-needed" "-lc" "-lgcc" "--as-needed" "-lgcc_s" "--no-as-needed" "/usr/lib/crtend.o" "/usr/lib/crtn.o"

[Quuxplusone]

You add -march=geode to your CFLAGS/CXXFLAGS.

[Quuxplusone]

Confirmed!
Despite the warnings:
'geode' is not a recognized processor for this target (ignoring processor)
'geode' is not a recognized processor for this target (ignoring processor)
'geode' is not a recognized processor for this target (ignoring processor)
this (clang -march=geode -O2 -g -o a16.clang-O2 a16.c) creates working code :)

[Quuxplusone]

Fixed in r164132.

[Quuxplusone]

Why Geode only? This problem affects all pre-"i686" CPUs, and even some "i686" ones (e.g. VIA C3 and Cyrix MII, which support stuff like CMOV but no long NOPs).

[Quuxplusone]

The list of target cpus without long nops was extended in r195679.