Ramana Radhakrishnan <ramana....@googlemail.com> writes: > On Tue, Feb 4, 2014 at 2:12 AM, Michael Hudson-Doyle > <michael.hud...@linaro.org> wrote: >> Ping? I'm attaching a marginally cleaner version of the test. I've had >> a look at integrating this into aapcs64.exp but got defeated in the >> end. If go-torture-execute took a list of sources as c-torture-execute >> does, then I think adding something like this to aapcs64.exp would work: >> >> # Test passing arrays by value >> set src $srcdir/$subdir/test_array.go >> if {[runtest_file_p $runtests $src]} { >> go-torture-execute [list $src $srcdir/$subdir/abitest.S >> $srcdir/$subdir/_test_array_c.c] \ >> $additional_flags >> } >> >> but it doesn't. I also think that some stuff needs to be set up before >> go-torture-execute can be called that I don't really understand and I >> also also don't know how to avoid executing this test if gccgo hasn't >> been built. > > Putting in a shell script like that is a bad idea,
To be clear: I was never proposing that this shell script be committed. It was just an unambiguous way of showing how to run my test. > the dejagnu infrastructure can take care of all the transport and > target bits for this. This will fail if someone were to try testing > this on qemu while the rest of the testsuite might work. > > However what happens if in aarch64.exp > > you do a > > load_lib go-dg.exp > > and essentially run the tests similar to testsuite/go.dg/dg.exp ? I can't see how to run a test case that consists of multiple source files like that. That doesn't mean it's not possible though... > That will take care of any cross-testing issues I hope with this using > dejagnu. > > If that fails I think we should just drop the test as the go testsuite > will catch this. Please. >> >> All that said, is there any chance of getting the original ABI fix >> committed? It would be nice to have it in 4.9. > > > I cannot approve or disapprove this patch but looking at the fix and > the ABI issue under question here, I agree that this should be fixed > for 4.9 and documented in the release notes. Your latest patch should > also take Yufeng's suggestion under consideration about merging the > condition in the if block. Oh right, I forgot that I hadn't sent a patch acting on that comment. Here is one.
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c index 16c51a8..958c667 100644 --- a/gcc/config/aarch64/aarch64.c +++ b/gcc/config/aarch64/aarch64.c @@ -1187,14 +1187,10 @@ aarch64_pass_by_reference (cumulative_args_t pcum ATTRIBUTE_UNUSED, size = (mode == BLKmode && type) ? int_size_in_bytes (type) : (int) GET_MODE_SIZE (mode); - if (type) + /* Aggregates are passed by reference based on their size. */ + if (type && AGGREGATE_TYPE_P (type)) { - /* Arrays always passed by reference. */ - if (TREE_CODE (type) == ARRAY_TYPE) - return true; - /* Other aggregates based on their size. */ - if (AGGREGATE_TYPE_P (type)) - size = int_size_in_bytes (type); + size = int_size_in_bytes (type); } /* Variable sized arguments are always returned by reference. */
Cheers, mwh > > Cheers, > Ramana > >> >> Cheers, >> mwh >> >> Michael Hudson-Doyle <michael.hud...@linaro.org> writes: >> >>> Richard Earnshaw <rearn...@arm.com> writes: >>> >>>> On 17/01/14 23:56, Michael Hudson-Doyle wrote: >>>>> Ian Lance Taylor <i...@golang.org> writes: >>>>> >>>>>> On Fri, Jan 17, 2014 at 11:32 AM, Michael Hudson-Doyle >>>>>> <michael.hud...@canonical.com> wrote: >>>>>>> >>>>>>> On 18 Jan 2014 07:50, "Yufeng Zhang" <yufeng.zh...@arm.com> wrote: >>>>>>>> >>>>>>>> Also can you please try to add some new test(s)? It may not be that >>>>>>>> straightforward to add non-C/C++ tests, but give it a try. >>>>>>> >>>>>>> Can you give some hints? Like at least where in the tree such a test >>>>>>> would >>>>>>> go? I don't know this code at all. >>>>>> >>>>>> There is already a test in libgo, of course. >>>>>> >>>>>> I think it would be pretty hard to write a test that doesn't something >>>>>> like what libgo does. The problem is that GCC is entirely consistent >>>>>> with and without your patch. You could add a Go test that passes an >>>>>> array in gcc/testsuite/go.go-torture/execute/ easily enough, but it >>>>>> would be quite hard to add a test that doesn't pass whether or not >>>>>> your patch is applied. >>>>> >>>>> I think it would have to be a code generation test, i.e. that compiling >>>>> something like >>>>> >>>>> func second(e [2]int64) int64 { >>>>> return e[1] >>>>> } >>>>> >>>>> does not access memory or something along those lines. I'll have a look >>>>> next week. >>>>> >>>>> Cheers, >>>>> mwh >>>>> >>>> >>>> Michael, >>>> >>>> Can you have a look at how the tests in gcc.target/aarch64/aapcs64 work; >>>> it ought to be possible to write a test (though not in C) to catch this. >>> >>> Yeah, I had found those tests. It would be much easier if I could write >>> this in C :-) >>> >>>> The tests work by calling a wrapper function written in assembler -- >>>> that wrapper stores out all the registers used for parameter passing and >>>> then calls back into the test's validator with the register dump >>>> available. Code can then check that values are passed in the places >>>> expected. This gives the compiler the separation between caller and >>>> callee that's needed to test this feature. >>> >>> Right, that much makes sense. I'm not sure I completely understand all >>> the preprocessor trickery involved but the output of it seems clear >>> enough. >>> >>>> My preferred languages for these tests would be (in approximate order) >>>> c, c++, fortran, java, ada, go. That order is based on which languages >>>> are tested most by users. >>> >>> Well of course it cannot be done in C or C++. A commenter on the PR >>> said that while fortran does allow passing arrays by value, it's all >>> handled in the frontend. No idea about Java. Ada has arrays by value >>> but I don't know it even slightly. So it's the last one on your list >>> but here's a diff that adds hack at a test in Go: >>> >>> diff --git a/gcc/testsuite/gcc.target/aarch64/aapcs64/abitest.S >>> b/gcc/testsuite/gcc.target/aarch64/aapcs64/abitest.S >>> index 86ce7be..365cd91 100644 >>> --- a/gcc/testsuite/gcc.target/aarch64/aapcs64/abitest.S >>> +++ b/gcc/testsuite/gcc.target/aarch64/aapcs64/abitest.S >>> @@ -1,9 +1,12 @@ >>> .global dumpregs >>> .global myfunc >>> + .global main.myfunc >>> .type dumpregs,%function >>> .type myfunc,%function >>> + .type main.myfunc,%function >>> dumpregs: >>> myfunc: >>> +main.myfunc: >>> mov x16, sp >>> mov x17, sp >>> sub sp, sp, 352 // 336 for registers and 16 for old sp and lr >>> diff --git a/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array.go >>> b/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array.go >>> new file mode 100644 >>> index 0000000..b5e90e4 >>> --- /dev/null >>> +++ b/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array.go >>> @@ -0,0 +1,9 @@ >>> +package main >>> + >>> +func myfunc(e [2]int64) >>> + >>> +func main() { >>> + myfunc([2]int64{40, 50}) >>> +} >>> + >>> + >>> diff --git a/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array.sh >>> b/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array.sh >>> new file mode 100755 >>> index 0000000..0b3202d >>> --- /dev/null >>> +++ b/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array.sh >>> @@ -0,0 +1,11 @@ >>> +#!/bin/sh >>> +GCC=${GCC:-gcc} >>> +AARCH64HOST=${AARCH64HOST:-localhost} >>> + >>> +set -x >>> + >>> +${GCC}go -g -c test_array.go -o test_array.o >>> +${GCC} -g -c abitest.S -o abitest.o >>> +${GCC} -g -c test_array_c.c -o test_array_c.o >>> +${GCC}go -g abitest.o test_array.o test_array_c.o -o test_array >>> +scp ./test_array ${AARCH64HOST}: && ssh ${AARCH64HOST} ./test_array >>> diff --git a/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array_c.c >>> b/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array_c.c >>> new file mode 100644 >>> index 0000000..981d12d >>> --- /dev/null >>> +++ b/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array_c.c >>> @@ -0,0 +1,19 @@ >>> +int which_kind_of_test = 0; >>> + >>> +#include "abitest-common.h" >>> + >>> +void >>> +testfunc (char *stack) >>> +{ >>> + { >>> + long __x = 40; >>> + if (memcmp (&__x, stack + X0, sizeof (long)) != 0) >>> + abort (); >>> + } >>> + { >>> + long __x = 50; >>> + if (memcmp (&__x, stack + X1, sizeof (long)) != 0) >>> + abort (); >>> + } >>> + return; >>> +} >>> >>> Obviously it's not integrated into the test framework even slightly but >>> on the good side, it fails without my fix and passes with it... Are you >>> able to do the integration part or provide some hints for me? >>> >>> Cheers, >>> mwh >> >> >> diff --git a/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array.go >> b/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array.go >> new file mode 100644 >> index 0000000..c795e88 >> --- /dev/null >> +++ b/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array.go >> @@ -0,0 +1,10 @@ >> +package main >> + >> +//extern myfunc >> +func myfunc(e [2]int64) >> + >> +func main() { >> + myfunc([2]int64{40, 50}) >> +} >> + >> + >> diff --git a/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array.sh >> b/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array.sh >> new file mode 100755 >> index 0000000..1c41192 >> --- /dev/null >> +++ b/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array.sh >> @@ -0,0 +1,8 @@ >> +#!/bin/sh >> +GCCGO=${GCCGO:-gccgo} >> +AARCH64HOST=${AARCH64HOST:-localhost} >> + >> +set -x >> + >> +${GCCGO} -g test_array.go abitest.S test_array_c.o -o test_array >> +scp ./test_array ${AARCH64HOST}: && ssh ${AARCH64HOST} ./test_array >> diff --git a/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array_c.c >> b/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array_c.c >> new file mode 100644 >> index 0000000..41066b2 >> --- /dev/null >> +++ b/gcc/testsuite/gcc.target/aarch64/aapcs64/test_array_c.c >> @@ -0,0 +1,20 @@ >> +int which_kind_of_test = 0; >> + >> +#include "abitest-common.h" >> + >> +void >> +testfunc (char *stack) >> +{ >> + { >> + long __x = 40; >> + if (memcmp (&__x, stack + X0, sizeof (long)) != 0) >> + abort (); >> + } >> + { >> + long __x = 50; >> + if (memcmp (&__x, stack + X1, sizeof (long)) != 0) >> + abort (); >> + } >> + return; >> +} >> + >>