> -----Original Message-----
> From: Richard Henderson <richard.henderson@linaro.org>
> Sent: Tuesday, July 26, 2022 11:42 AM
> To: Taylor Simpson <tsimpson@quicinc.com>
> Cc: qemu-devel <qemu-devel@nongnu.org>
> Subject: Re: hexagon docker test failure
> 
> On 7/26/22 10:23, Taylor Simpson wrote:
> >
> >> -----Original Message-----
> >> From: Richard Henderson <richard.henderson@linaro.org>
> >> Sent: Tuesday, July 26, 2022 10:41 AM
> >> To: Taylor Simpson <tsimpson@quicinc.com>
> >> Cc: qemu-devel <qemu-devel@nongnu.org>
> >> Subject: hexagon docker test failure
> >>
> >> Hi Taylor,
> >>
> >> One of your recent hexagon testsuite changes is incompatible with the
> >> docker image that we're using:
> >>
> >> tests/tcg/hexagon/multi_result.c:79:16: error: invalid instruction
> >>
> >>     asm volatile("%0,p0 = vminub(%2, %3)\n\t"
> >>
> >>                  ^
> >>
> >> <inline asm>:1:2: note: instantiated into assembly here
> >>
> >>           r3:2,p0 = vminub(r1:0, r3:2)
> >>
> >>           ^
> >>
> >> 1 error generated.
> >>
> >>
> >> Can we try again to update debian-hexagon-cross?  I recall that last
> >> time there was a compiler bug that prevented forward progress.
> >> Perhaps that has been fixed in the interim?
> >>
> >> I'm willing to accept such an update in the next week before rc1, but
> >> if we can't manage that we'll need to disable the failing test(s?).
> >> Thanks in advance,
> >>
> >>
> >> r~
> >
> > Hi Richard,
> >
> > Some of the tests require the -mv67 flag to be passed to the compiler
> because they have instructions that are new in v67.
> >
> > This patch
> > commit cd362defbbd09cbbc08b3bb465141542887b8cef
> > Author: Paolo Bonzini <pbonzini@redhat.com>
> > Date:   Fri May 27 16:35:48 2022 +0100
> >
> >      tests/tcg: merge configure.sh back into main configure script
> >
> > Moved this line from tests/tcg/configure.sh to the main configure
> > script
> > : ${cross_cc_cflags_hexagon="-mv67 -O2 -static"}
> >
> >
> > However, those flags aren't actually passed to the compiler any more - at
> least by default.
> >
> > The gitlab builder is passing
> > https://gitlab.com/qemu-project/qemu/-/jobs/2771528066
> > So, there must be something in $MAKE_CHECK_ARGS
> >
> > I use the following when I run
> > make EXTRA_CFLAGS='-mv67 -O2' check-tcg
> >
> >
> > So, we probably don't need a new docker image.  Do other targets have
> their cross_cc_cflags?  Please advise.
> 
> Oooh, that's easy.  Just modify CFLAGS in
> tests/tcg/hexagon/Makefile.target.
> 
> I've just tested
> 
> --- a/tests/tcg/hexagon/Makefile.target
> 
> +++ b/tests/tcg/hexagon/Makefile.target
> 
> @@ -19,7 +19,7 @@
> 
>   EXTRA_RUNS =
> 
> 
> 
>   CFLAGS += -Wno-incompatible-pointer-types -Wno-undefined-internal
> 
> -CFLAGS += -fno-unroll-loops
> 
> +CFLAGS += -fno-unroll-loops -mv67 -O2
> 
> 
> 
>   HEX_SRC=$(SRC_PATH)/tests/tcg/hexagon
> 
>   VPATH += $(HEX_SRC)
> 
> 
> and it now builds, but I see a runtime error:
> 
> timeout --foreground 90  /home/rth/qemu/bld/qemu-hexagon  misc >
> misc.out
> 
> make[1]: *** [../Makefile.target:158: run-misc] Error 1
> 
> $ cat ./tests/tcg/hexagon-linux-user/misc.out
> 
> ERROR: 0x0007 != 0x001f
> 
> FAIL
> 
> 
> Any ideas there?

That's because misc requires -O2 (don't remember why).  When you put -O2 in CFLAGS, it gets overridden.  Here's the command that gets used - notice the -O0 after the -O2.
/local/mnt/workspace/qemu-series/qemu/tests/docker/docker.py --engine auto cc --cc hexagon-unknown-linux-musl-clang -i qemu/debian-hexagon-cross -s /local/mnt/workspace/qemu-series/qemu --  -Wno-incompatible-pointer-types -Wno-undefined-internal -fno-unroll-loops -mv68 -O2 -Wall -Werror -O0 -g -fno-strict-aliasing  /local/mnt/workspace/qemu-series/qemu/tests/tcg/hexagon/misc.c -o misc  -static


So, instead of putting those in CFLAGS, put them in EXTRA_CFLAGS.

--- a/tests/tcg/hexagon/Makefile.target
+++ b/tests/tcg/hexagon/Makefile.target
@@ -20,6 +20,7 @@ EXTRA_RUNS =
 
 CFLAGS += -Wno-incompatible-pointer-types -Wno-undefined-internal
 CFLAGS += -fno-unroll-loops
+EXTRA_CFLAGS += -mv67 -O2
 
 HEX_SRC=$(SRC_PATH)/tests/tcg/hexagon
 VPATH += $(HEX_SRC)


Do I understand correctly that putting the flags in Makefile.target is the proper way and cross_cc_cflags is obsolete?  If so, I'll send the above as a patch.

Thanks,
Taylor

On 7/26/22 11:00, Taylor Simpson wrote:
> So, instead of putting those in CFLAGS, put them in EXTRA_CFLAGS.
> 
> --- a/tests/tcg/hexagon/Makefile.target
> +++ b/tests/tcg/hexagon/Makefile.target
> @@ -20,6 +20,7 @@ EXTRA_RUNS =
>   
>   CFLAGS += -Wno-incompatible-pointer-types -Wno-undefined-internal
>   CFLAGS += -fno-unroll-loops
> +EXTRA_CFLAGS += -mv67 -O2
>   
>   HEX_SRC=$(SRC_PATH)/tests/tcg/hexagon
>   VPATH += $(HEX_SRC)

Ah, ok.

> Do I understand correctly that putting the flags in Makefile.target is the proper way and cross_cc_cflags is obsolete?

cross_cc_flags is intended to handle using one compiler for multiple targets, e.g. arm vs 
armbe.

Which is not what you're attempting to do; you're trying to test a particular isa. 
Compare tests/tcg/aarch64/Makefile.target:

bti-1 bti-3: CFLAGS += -mbranch-protection=standard

pauth-%: CFLAGS += -march=armv8.3-a

mte-%: CFLAGS += -march=armv8.5-a+memtag


where we set specific isa extensions for specific tests.


r~