Patchwork Get tests/tcg building, fix unused variable warning due to wrong extended asm operand, fix the 'test' make target.

login
register
mail settings
Submitter Catalin Patulea
Date July 13, 2012, 6:55 a.m.
Message ID <1342162513-2626-1-git-send-email-catalinp@google.com>
Download mbox | patch
Permalink /patch/170799/
State New
Headers show

Comments

Catalin Patulea - July 13, 2012, 6:55 a.m.
Not all tests pass, but at least they can be run using 'make test'.

To build individual tests:
$ cd $BUILD_PATH/tests/tcg
$ SRC_PATH=path/to/qemu make <target>

Signed-off-by: Catalin Patulea <catalinp@google.com>
---
 tests/tcg/Makefile    |   16 ++++++++++------
 tests/tcg/test-i386.c |    3 ++-
 tests/tcg/test_path.c |   13 +++++++------
 3 files changed, 19 insertions(+), 13 deletions(-)
陳韋任 - July 13, 2012, 8:44 a.m.
Tested-by: Wei-Ren Chen

On Fri, Jul 13, 2012 at 02:55:13AM -0400, Catalin Patulea wrote:
> Not all tests pass, but at least they can be run using 'make test'.
> 
> To build individual tests:
> $ cd $BUILD_PATH/tests/tcg
> $ SRC_PATH=path/to/qemu make <target>

  [snip]

Regards,
chenwj
Catalin Patulea - July 16, 2012, 6:17 p.m.
Peter, do you have a second to look at this? I've got a new test in
tests/tcg that checks fprem correctness (or at least compares with
h/w...) that depends on this.

On Fri, Jul 13, 2012 at 4:44 AM, 陳韋任 (Wei-Ren Chen)
<chenwj@iis.sinica.edu.tw> wrote:
>   Tested-by: Wei-Ren Chen
>
> On Fri, Jul 13, 2012 at 02:55:13AM -0400, Catalin Patulea wrote:
>> Not all tests pass, but at least they can be run using 'make test'.
>>
>> To build individual tests:
>> $ cd $BUILD_PATH/tests/tcg
>> $ SRC_PATH=path/to/qemu make <target>
>
>   [snip]
>
> Regards,
> chenwj
>
> --
> Wei-Ren Chen (陳韋任)
> Computer Systems Lab, Institute of Information Science,
> Academia Sinica, Taiwan (R.O.C.)
> Tel:886-2-2788-3799 #1667
> Homepage: http://people.cs.nctu.edu.tw/~chenwj
Peter Maydell - July 16, 2012, 7:08 p.m.
On 13 July 2012 07:55, Catalin Patulea <catalinp@google.com> wrote:
> Not all tests pass, but at least they can be run using 'make test'.
>
> To build individual tests:
> $ cd $BUILD_PATH/tests/tcg
> $ SRC_PATH=path/to/qemu make <target>

I don't know enough about our test makefile structure to
know if this is right (try Anthony) but some comments below.

(It looks like some of this is fixing bustage from Anthony's
commit c09015dd.)

> Signed-off-by: Catalin Patulea <catalinp@google.com>
> ---
>  tests/tcg/Makefile    |   16 ++++++++++------
>  tests/tcg/test-i386.c |    3 ++-
>  tests/tcg/test_path.c |   13 +++++++------
>  3 files changed, 19 insertions(+), 13 deletions(-)
>
> diff --git a/tests/tcg/Makefile b/tests/tcg/Makefile
> index 15e36a2..3256a98 100644
> --- a/tests/tcg/Makefile
> +++ b/tests/tcg/Makefile
> @@ -1,13 +1,13 @@
> --include ../config-host.mak
> +-include ../../config-host.mak
>  -include $(SRC_PATH)/rules.mak
>
> -$(call set-vpath, $(SRC_PATH)/tests)
> +$(call set-vpath, $(SRC_PATH)/tests/tcg)
>
> -QEMU=../i386-linux-user/qemu-i386
> -QEMU_X86_64=../x86_64-linux-user/qemu-x86_64
> +QEMU=../../i386-linux-user/qemu-i386
> +QEMU_X86_64=../../x86_64-linux-user/qemu-x86_64
>  CC_X86_64=$(CC_I386) -m64
>
> -QEMU_INCLUDES += -I..
> +QEMU_INCLUDES += -I../..
>  CFLAGS=-Wall -O2 -g -fno-strict-aliasing
>  #CFLAGS+=-msse2
>  LDFLAGS=
> @@ -36,6 +36,7 @@ TESTS += $(I386_TESTS)
>  endif
>
>  all: $(patsubst %,run-%,$(TESTS))
> +test: all
>
>  # rules to run tests
>
> @@ -74,7 +75,10 @@ run-test_path: test_path
>  # rules to compile tests
>
>  test_path: test_path.o
> +       $(CC_I386) $(LDFLAGS) $(LIBS) -o $@ $^

This won't compile:
ccache gcc -m32  -lrt -pthread -lgthread-2.0 -lrt -lglib-2.0    -o
test_path test_path.o
test_path.o: In function `qemu_iovec_init':
/home/pm215/src/qemu/qemu/cutils.c:142: undefined reference to `g_malloc'
test_path.o: In function `qemu_iovec_add':
/home/pm215/src/qemu/qemu/cutils.c:166: undefined reference to `g_realloc'
test_path.o: In function `qemu_iovec_destroy':
/home/pm215/src/qemu/qemu/cutils.c:210: undefined reference to `g_free'
collect2: ld returned 1 exit status

You need to have the $(LIBS) after the $^.

> +
>  test_path.o: test_path.c
> +       $(CC_I386) $(QEMU_INCLUDES) $(GLIB_CFLAGS) $(CFLAGS) $(LDFLAGS) -c -o $@ $^

Ditto.

>  hello-i386: hello-i386.c
>         $(CC_I386) -nostdlib $(CFLAGS) -static $(LDFLAGS) -o $@ $<
> @@ -86,7 +90,7 @@ testthread: testthread.c
>  # i386/x86_64 emulation test (test various opcodes) */
>  test-i386: test-i386.c test-i386-code16.S test-i386-vm86.S \
>             test-i386.h test-i386-shift.h test-i386-muldiv.h
> -       $(CC_I386) $(CFLAGS) $(LDFLAGS) -o $@ \
> +       $(CC_I386) $(QEMU_INCLUDES) $(CFLAGS) $(LDFLAGS) -o $@ \
>                $(<D)/test-i386.c $(<D)/test-i386-code16.S $(<D)/test-i386-vm86.S -lm
>
>  test-x86_64: test-i386.c \
> diff --git a/tests/tcg/test-i386.c b/tests/tcg/test-i386.c
> index 8e64bba..acbc9c9 100644
> --- a/tests/tcg/test-i386.c
> +++ b/tests/tcg/test-i386.c
> @@ -17,6 +17,7 @@
>   *  along with this program; if not, see <http://www.gnu.org/licenses/>.
>   */
>  #define _GNU_SOURCE
> +#include "compiler.h"  // QEMU_PACKED

checkpatch complains about this //-syntax comment;
might as well just drop it.

>  #include <stdlib.h>
>  #include <stdio.h>
>  #include <string.h>
> @@ -784,7 +785,7 @@ void fpu_clear_exceptions(void)
>          long double fpregs[8];
>      } float_env32;
>
> -    asm volatile ("fnstenv %0\n" : : "m" (float_env32));
> +    asm volatile ("fnstenv %0\n" : "=m" (float_env32));
>      float_env32.fpus &= ~0x7f;
>      asm volatile ("fldenv %0\n" : : "m" (float_env32));
>  }

This doesn't seem to be sufficient for test-i386 to compile
for me:
ccache gcc -m32 -I/home/pm215/src/qemu/qemu/slirp -I.
-I/home/pm215/src/qemu/qemu -I/home/pm215/src/qemu/qemu/fpu -I../..
-Wall -O2 -g -fno-strict-aliasing  -o test-i386 \
              ./test-i386.c ./test-i386-code16.S ./test-i386-vm86.S -lm
./test-i386.c: Assembler messages:
./test-i386.c:1831: Error: expecting lockable instruction after `lock'

Since 'lock nop' isn't valid we probably need to do this with
a .byte directive to emit the sequence we require.
(this is gcc (Ubuntu/Linaro 4.6.1-9ubuntu3) 4.6.1 on x86.)

> diff --git a/tests/tcg/test_path.c b/tests/tcg/test_path.c
> index 7265a94..a064eea 100644
> --- a/tests/tcg/test_path.c
> +++ b/tests/tcg/test_path.c
> @@ -1,11 +1,12 @@
>  /* Test path override code */
> -#include "../config-host.h"
> -#include "../qemu-malloc.c"
> -#include "../cutils.c"
> -#include "../path.c"
> -#include "../trace.c"
> +#define _GNU_SOURCE
> +#include "config-host.h"
> +#include "iov.c"
> +#include "cutils.c"
> +#include "path.c"
> +#include "trace.c"
>  #ifdef CONFIG_TRACE_SIMPLE
> -#include "../simpletrace.c"
> +#include "../trace/simple.c"
>  #endif
>
>  #include <stdarg.h>

The whole way this test works looks pretty foul to me, so yeah, whatever :-)

-- PMM
Catalin Patulea - July 16, 2012, 8 p.m.
On Mon, Jul 16, 2012 at 3:08 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> You need to have the $(LIBS) after the $^.
Done.

>>  test_path.o: test_path.c
>> +       $(CC_I386) $(QEMU_INCLUDES) $(GLIB_CFLAGS) $(CFLAGS) $(LDFLAGS) -c -o $@ $^
>
> Ditto.
I don't think I meant to put $(LDFLAGS) in there. This is just a
compilation step, after all. I've taken it out, please let me know if
you still have any troubles building.

>> +#include "compiler.h"  // QEMU_PACKED
>
> checkpatch complains about this //-syntax comment;
> might as well just drop it.
Done.

>> -    asm volatile ("fnstenv %0\n" : : "m" (float_env32));
>> +    asm volatile ("fnstenv %0\n" : "=m" (float_env32));
>>      float_env32.fpus &= ~0x7f;
>>      asm volatile ("fldenv %0\n" : : "m" (float_env32));
>>  }
>
> This doesn't seem to be sufficient for test-i386 to compile
> for me:
> ccache gcc -m32 -I/home/pm215/src/qemu/qemu/slirp -I.
> -I/home/pm215/src/qemu/qemu -I/home/pm215/src/qemu/qemu/fpu -I../..
> -Wall -O2 -g -fno-strict-aliasing  -o test-i386 \
>               ./test-i386.c ./test-i386-code16.S ./test-i386-vm86.S -lm
> ./test-i386.c: Assembler messages:
> ./test-i386.c:1831: Error: expecting lockable instruction after `lock'

>
> Since 'lock nop' isn't valid we probably need to do this with
> a .byte directive to emit the sequence we require.
I don't understand why gcc would be generating a lock prefix here..

> (this is gcc (Ubuntu/Linaro 4.6.1-9ubuntu3) 4.6.1 on x86.)
I'm on gcc version 4.4.3 (Ubuntu 4.4.3-4ubuntu5.1) and it appears
unreasonably difficult to get 4.6 on my machine. I'll keep trying but
for now I'm shooting blind.

Wei-Ren, what version of gcc are you running? (and had success with?)

>>  #include <stdarg.h>
>
> The whole way this test works looks pretty foul to me, so yeah, whatever :-)
I agree that this is grotesque - I'm just trying to make the minimal
number of changes to unbreak things so that I can get on with my fprem
life ;-)
Peter Maydell - July 16, 2012, 9:11 p.m.
On 16 July 2012 21:00, Catalin Patulea <catalinp@google.com> wrote:
> On Mon, Jul 16, 2012 at 3:08 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> This doesn't seem to be sufficient for test-i386 to compile
>> for me:
>> ccache gcc -m32 -I/home/pm215/src/qemu/qemu/slirp -I.
>> -I/home/pm215/src/qemu/qemu -I/home/pm215/src/qemu/qemu/fpu -I../..
>> -Wall -O2 -g -fno-strict-aliasing  -o test-i386 \
>>               ./test-i386.c ./test-i386-code16.S ./test-i386-vm86.S -lm
>> ./test-i386.c: Assembler messages:
>> ./test-i386.c:1831: Error: expecting lockable instruction after `lock'
>
>>
>> Since 'lock nop' isn't valid we probably need to do this with
>> a .byte directive to emit the sequence we require.
> I don't understand why gcc would be generating a lock prefix here..

Have you looked at the code? We're specifically asking for one:
        asm volatile("lock nop");
(which we expect to be invalid). We just need to handle that the same
way we do for the case a bit later, by using
        asm volatile(".byte 0xsomething, 0xsomething");
to emit the sequence we want rather than asking gas to do something
it thinks is illegal.

Since you have an assembler that doesn't complain about "lock nop"
you should be able to disassemble the binary it creates to find out
what the byte sequence we need is.

>> (this is gcc (Ubuntu/Linaro 4.6.1-9ubuntu3) 4.6.1 on x86.)
> I'm on gcc version 4.4.3 (Ubuntu 4.4.3-4ubuntu5.1) and it appears
> unreasonably difficult to get 4.6 on my machine.

If you have the disk space then setting up a chroot environment of
a newer ubuntu is probably the easiest thing.

-- PMM
Catalin Patulea - July 16, 2012, 9:23 p.m.
On Mon, Jul 16, 2012 at 5:11 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 16 July 2012 21:00, Catalin Patulea <catalinp@google.com> wrote:
>> On Mon, Jul 16, 2012 at 3:08 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> ccache gcc -m32 -I/home/pm215/src/qemu/qemu/slirp -I.
>>> -I/home/pm215/src/qemu/qemu -I/home/pm215/src/qemu/qemu/fpu -I../..
>>> -Wall -O2 -g -fno-strict-aliasing  -o test-i386 \
>>>               ./test-i386.c ./test-i386-code16.S ./test-i386-vm86.S -lm
>>> ./test-i386.c: Assembler messages:
>>> ./test-i386.c:1831: Error: expecting lockable instruction after `lock'
> Since you have an assembler that doesn't complain about "lock nop"
> you should be able to disassemble the binary it creates to find out
> what the byte sequence we need is.
For the readers: PATCHv2 does exactly that, thanks for reviewing it.

>>> (this is gcc (Ubuntu/Linaro 4.6.1-9ubuntu3) 4.6.1 on x86.)
>> I'm on gcc version 4.4.3 (Ubuntu 4.4.3-4ubuntu5.1) and it appears
>> unreasonably difficult to get 4.6 on my machine.
>
> If you have the disk space then setting up a chroot environment of
> a newer ubuntu is probably the easiest thing.
That is a really great idea, will do.

Patch

diff --git a/tests/tcg/Makefile b/tests/tcg/Makefile
index 15e36a2..3256a98 100644
--- a/tests/tcg/Makefile
+++ b/tests/tcg/Makefile
@@ -1,13 +1,13 @@ 
--include ../config-host.mak
+-include ../../config-host.mak
 -include $(SRC_PATH)/rules.mak
 
-$(call set-vpath, $(SRC_PATH)/tests)
+$(call set-vpath, $(SRC_PATH)/tests/tcg)
 
-QEMU=../i386-linux-user/qemu-i386
-QEMU_X86_64=../x86_64-linux-user/qemu-x86_64
+QEMU=../../i386-linux-user/qemu-i386
+QEMU_X86_64=../../x86_64-linux-user/qemu-x86_64
 CC_X86_64=$(CC_I386) -m64
 
-QEMU_INCLUDES += -I..
+QEMU_INCLUDES += -I../..
 CFLAGS=-Wall -O2 -g -fno-strict-aliasing
 #CFLAGS+=-msse2
 LDFLAGS=
@@ -36,6 +36,7 @@  TESTS += $(I386_TESTS)
 endif
 
 all: $(patsubst %,run-%,$(TESTS))
+test: all
 
 # rules to run tests
 
@@ -74,7 +75,10 @@  run-test_path: test_path
 # rules to compile tests
 
 test_path: test_path.o
+	$(CC_I386) $(LDFLAGS) $(LIBS) -o $@ $^
+
 test_path.o: test_path.c
+	$(CC_I386) $(QEMU_INCLUDES) $(GLIB_CFLAGS) $(CFLAGS) $(LDFLAGS) -c -o $@ $^
 
 hello-i386: hello-i386.c
 	$(CC_I386) -nostdlib $(CFLAGS) -static $(LDFLAGS) -o $@ $<
@@ -86,7 +90,7 @@  testthread: testthread.c
 # i386/x86_64 emulation test (test various opcodes) */
 test-i386: test-i386.c test-i386-code16.S test-i386-vm86.S \
            test-i386.h test-i386-shift.h test-i386-muldiv.h
-	$(CC_I386) $(CFLAGS) $(LDFLAGS) -o $@ \
+	$(CC_I386) $(QEMU_INCLUDES) $(CFLAGS) $(LDFLAGS) -o $@ \
               $(<D)/test-i386.c $(<D)/test-i386-code16.S $(<D)/test-i386-vm86.S -lm
 
 test-x86_64: test-i386.c \
diff --git a/tests/tcg/test-i386.c b/tests/tcg/test-i386.c
index 8e64bba..acbc9c9 100644
--- a/tests/tcg/test-i386.c
+++ b/tests/tcg/test-i386.c
@@ -17,6 +17,7 @@ 
  *  along with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 #define _GNU_SOURCE
+#include "compiler.h"  // QEMU_PACKED
 #include <stdlib.h>
 #include <stdio.h>
 #include <string.h>
@@ -784,7 +785,7 @@  void fpu_clear_exceptions(void)
         long double fpregs[8];
     } float_env32;
 
-    asm volatile ("fnstenv %0\n" : : "m" (float_env32));
+    asm volatile ("fnstenv %0\n" : "=m" (float_env32));
     float_env32.fpus &= ~0x7f;
     asm volatile ("fldenv %0\n" : : "m" (float_env32));
 }
diff --git a/tests/tcg/test_path.c b/tests/tcg/test_path.c
index 7265a94..a064eea 100644
--- a/tests/tcg/test_path.c
+++ b/tests/tcg/test_path.c
@@ -1,11 +1,12 @@ 
 /* Test path override code */
-#include "../config-host.h"
-#include "../qemu-malloc.c"
-#include "../cutils.c"
-#include "../path.c"
-#include "../trace.c"
+#define _GNU_SOURCE
+#include "config-host.h"
+#include "iov.c"
+#include "cutils.c"
+#include "path.c"
+#include "trace.c"
 #ifdef CONFIG_TRACE_SIMPLE
-#include "../simpletrace.c"
+#include "../trace/simple.c"
 #endif
 
 #include <stdarg.h>