(In reply to Rainer Orth from comment #0)
> It seems to me that ms-sysv.exp is seriously misguided in trying to do all
> its compilations manually instead of using
> dg-test/dg-runtest/gcc_target_compile
> which whould nicely avoid all those issues.

Well, this was my introduction to DejaGnu and the test harness.  I found that none of these support doing a build when there is more than one object file -- in my case, I'm linking the output of both ms-sysv.c and do_test.S.  However, this test started out with multiple .c files and I was able to reduce it down to one.  I'm going to see if there's a way to cleanly do my assembly inline and reduce it down to a single translation unit which gc-runtest, et. al. will work with.  Otherwise, I'll have to fix the .struct, CFLAGS and multiple warnings.

If I'm wrong about the single object file thing, please point me in the right direction.

> The new gcc.target/x86_64/abi/ms-sysv tests FAIL in various e.g. on
> i386-pc-solaris2.*
> and i686-pc-linux-gnu:
> 
> * In those 32-bit-default configurations, the 32-bit multilib is skipped as
>   unsupported as expected (although the UNSUPPORTED entry in gcc.sum occurs
>   e.g. 45 times for -j48 testing instead of only once),

Sadly, I discovered that by not using the standard test functions that I had to cook up my own parallelism scheme, otherwise all of my tests would run once for each -j<job>!  I think that this issue is fixable though.

Actually, I just realized that it won't help to move do_test.S into ms-sysv.c as inline asm because each test still needs a unique ms-sysv-generated.h header that's generated by the output of gen.cc.  Although I suppose it's possible to generate all of the headers in advance (with unique names) and then set the header name in the options with -DGEN_HEADER_NAME=name.h -- that will have its own set of issues.

> --- Comment #1 from Daniel Santos <daniel.santos at pobox dot com> ---
> (In reply to Rainer Orth from comment #0)
>> It seems to me that ms-sysv.exp is seriously misguided in trying to do all
>> its compilations manually instead of using
>> dg-test/dg-runtest/gcc_target_compile
>> which whould nicely avoid all those issues.
>
> Well, this was my introduction to DejaGnu and the test harness.  I found that
> none of these support doing a build when there is more than one object file --
> in my case, I'm linking the output of both ms-sysv.c and do_test.S.  However,

Huh?  What about dg-additional-sources, which seems to be exactly what
you need and is even documented in sourcebuild.texi ;-)

> this test started out with multiple .c files and I was able to reduce it down
> to one.  I'm going to see if there's a way to cleanly do my assembly inline and

I wouldn't go that way unless absolutely necessary: unless you need
something from gcc inline assembly, this makes the testcase only harder
to read.

> If I'm wrong about the single object file thing, please point me in the right
> direction.

I believe you are, but it's admittedly hard finding your way around in
the testsuite once you leave the trodden paths.

>> The new gcc.target/x86_64/abi/ms-sysv tests FAIL in various e.g. on
>> i386-pc-solaris2.*
>> and i686-pc-linux-gnu:
>> 
>> * In those 32-bit-default configurations, the 32-bit multilib is skipped as
>>   unsupported as expected (although the UNSUPPORTED entry in gcc.sum occurs
>>   e.g. 45 times for -j48 testing instead of only once),
>
> Sadly, I discovered that by not using the standard test functions that I had to
> cook up my own parallelism scheme, otherwise all of my tests would run once for
> each -j<job>!  I think that this issue is fixable though.

I'd hope so.  E.g. you get the usual PASS and FAIL lines in gcc.sum for
free by using the standard infrastructure instead of cooking up your
own.

	Rainer

> --- Comment #2 from Daniel Santos <daniel.santos at pobox dot com> ---
> Actually, I just realized that it won't help to move do_test.S into ms-sysv.c
> as inline asm because each test still needs a unique ms-sysv-generated.h header
> that's generated by the output of gen.cc.  Although I suppose it's possible to
> generate all of the headers in advance (with unique names) and then set the
> header name in the options with -DGEN_HEADER_NAME=name.h -- that will have its
> own set of issues.

Exactly: I'd avoid that if you can.  It will only complicate things.

	Rainer

OK, I think I've got these fixed but I need to rerun my tests now.  Somebody else discovered another flaw that caused the test to break with -j1 (when parallelization wasn't being used).  I hate that I've had to go so far off of the beaten path with this test, but I don't see another way to do it right now unless I pre-generated all of the headers (for each parallelization directory), but they are about 9 MB each so I think that would be a very bad idea -- if you ran -j48, you could get 1.7 GiB of stupid headers.

(In reply to ro@CeBiTec.Uni-Bielefeld.DE from comment #4)
> Exactly: I'd avoid that if you can.  It will only complicate things.

The only advantage is that I can get the offsets of the test_data struct members cleanly using inline gcc, but for now I've just replaced them with hard offsets and put asserts in the main() of the C file to validate that those offsets haven't changed.

Daniel

Created attachment 41386 [details]
proposed fix

Rainer,

Would you be so kind as to test this on Solaris for me please?  I don't have access to a Solaris machine and I've never set it up before, so I wouldn't even know where to start to try to build an OpensSolaris VM.

Also, I don't know if Mike will accept adding the new gcc/testsuite/lib/parallelize.exp, if not I'll just reintegrate it into the ms-sysv.exp file.

(In reply to ro@CeBiTec.Uni-Bielefeld.DE from comment #3)
> > Well, this was my introduction to DejaGnu and the test harness.  I found that
> > none of these support doing a build when there is more than one object file --
> > in my case, I'm linking the output of both ms-sysv.c and do_test.S.  However,
> 
> Huh?  What about dg-additional-sources, which seems to be exactly what
> you need and is even documented in sourcebuild.texi ;-)

I think my limitation is the generated headers.  I was examining how struct-layout-1.exp works (as it is the most similar test to mine) and it generates all of it's sources for each job, but they are pretty small.  I don't suppose you know of a way to generates my sources (headers) in one place and then access them when ms-sysv.exp runs do you?  I'm semi-content with leaving it as it is (presuming all of the flaws can be fixed), but it would be nice to be able to use the standard functions.

On the flip side, each of these tests are fairly slow (especially with rtl checking) and I understand that the way dg splits up tests might not parallelize well with fewer, slower tests.

Daniel

Daniel,

> Would you be so kind as to test this on Solaris for me please?  I don't have
> access to a Solaris machine and I've never set it up before, so I wouldn't even
> know where to start to try to build an OpensSolaris VM.

sure, though there's no need at all (except for the .struct part) to do
the testing on Solaris.  I believe there are ready-made Solaris/x86
VirtualBox images, though.  For the multilib problem, you can easily
configure gcc for i686-pc-linux-gnu with --enable-targets=all on a
Linux/x86_64 box (with a few necessary 32-bit development packages
added), so the default multilib is non-x86_64, while the x86_64 multilib
is only used with -m64.

I gave your patch a quick whirl on i386-pc-solaris2.12 (with /bin/as),
and just that multilib problem is still unfixed.  For the (default)
32-bit multilib, the test is skipped as should be, but for the 64-bit
multilib, compilations still lack the required -m64, so they fail with

spawn /var/gcc/regression/trunk/12-gcc/build/gcc/xgcc -B/var/gcc/regression/trunk/12-gcc/build/gcc/ -I/var/gcc/regression/trunk/12-gcc/build/gcc/testsuite/gcc/ms-sysv -I/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv -fno-diagnostics-show-caret -fdiagnostics-color=never -O2 -Wall -c -o /var/gcc/regression/trunk/12-gcc/build/gcc/testsuite/gcc/ms-sysv/ms-sysv.o /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c^M
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c:62:3: error: #error Test only valid on x86_64^M
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c:102:5: error: unknown type name '__uint128_t'^M
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c:234:34: error: unknown type name '__uint128_t'^M
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c:234:56: error: unknown type name '__uint128_t'^M
WARNING: Could not build /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c.
FAIL: gcc.target/x86_64/abi/ms-sysv CFLAGS="-O2" generator_args="-p0-5"

as before.  To check how things were otherwise, I just manually
assembled/compiled do-test.S and ms-sysv.c with -m64, linked them into
ms-sysv.exe, which ran successfully, so it seems were getting closer.

However, I still don't understand why you are jumping through all these
hoops in ms-sysv.exp doing the compilations etc. manually rather than
just relying on dg-runtest or similar.  This would avoid all this
multilib trouble nicely, and massivly reduce ms-sysv.exp.

One or two nits about PR management, btw.: it is good practice to take
the PR if you're working on it.  And just add the URL to the patch
submissing into the URL field.

Thanks.

	Rainer

Thank you again for the assistance.

(In reply to ro@CeBiTec.Uni-Bielefeld.DE from comment #8)
> Daniel,
>
> > Would you be so kind as to test this on Solaris for me please?  I don't have
> > access to a Solaris machine and I've never set it up before, so I wouldn't even
> > know where to start to try to build an OpensSolaris VM.
>
> sure, though there's no need at all (except for the .struct part) to do
> the testing on Solaris.  I believe there are ready-made Solaris/x86
> VirtualBox images, though.

I've found a few, so going to try them out when I get some time.  Oracle even has something on their downloads.  I haven't used Solaris since the early aughts.

> For the multilib problem, you can easily
> configure gcc for i686-pc-linux-gnu with --enable-targets=all on a
> Linux/x86_64 box (with a few necessary 32-bit development packages
> added), so the default multilib is non-x86_64, while the x86_64 multilib
> is only used with -m64.

Hmm, I seem to be having problems getting this to work.  Would I configure with --target=i686-pc-linux-gnu --enable-targets=all --enable-multilib?

> However, I still don't understand why you are jumping through all these
> hoops in ms-sysv.exp doing the compilations etc. manually rather than
> just relying on dg-runtest or similar.  This would avoid all this
> multilib trouble nicely, and massivly reduce ms-sysv.exp.

Well quite frankly because dg-runtest, et. al. don't offer support for tests that use code generators.  The generated headers using the default options are between 4.4 and 6 MiB in size and there are more things that need to be tested (-fsplit-stack to name one) that isn't tested now.  I would also like to add a feature where defining an environment variable generates more comprehensive tests that I wouldn't want to run for every test (as it could take hours with --enable-checking=all,rtl).

The most behaviorally similar test currently in the tree is gcc/testsuite/gcc.dg/compat/struct-layout-1.exp, which builds a generator (using remote_exec), runs the generator (remote_exec again) to generate sources for all tests and then builds and runs each test using (using compat-execute).  Calls to remote_exec are not automatically parallelized.  I don't fully understand how the gcc/testsuite/lib/compat.exp library works, but I'm guessing that calls to compat-execute are parallelized by dejagnu.

The scheme that struct-layout-1 uses builds the generator and creates sources for all of the tests in job directory (i.e., gcc/testsuite/gcc{,1,2,3,4,5,6,etc.}/gcc.dg-struct-layout-1).  They take up 1.21 MiB per job, so -j48 results in 58 MiB of space usage.  My generator and generated sources are larger, and currently take about 11.65 MiB per job, so -j48 would eat 559 MiB of disk space, even though there are only 6 tests at the moment.  This could be mitigated if there was a way to build and run the generator only once and have the output go to a directory shared across jobs, but I'm not yet aware of any such existing mechanism.

This doesn't mean that my approach is the only solution.  In fact, I built this with Mike Stump's counsel and later discovered that when I ran multiple jobs, each test was run once per job, so -j8 would run all of the tests 8 times, rather than split them apart!  That's when I added the parallelization scheme.

So if you have some better ideas on how to accomplish this then please do present them.  Or maybe I'm misunderstanding something about the way dg-runtest, gcc_target_compile, etc. work in relation to parallelism?  My understanding is that if I use them in succession for a single test run (i.e., build the generator, run the generator, build & run the test) that they could end up being run on different jobs and then fail.

> One or two nits about PR management, btw.: it is good practice to take
> the PR if you're working on it.  And just add the URL to the patch
> submissing into the URL field.
>
> Thanks.
>
> 	Rainer

I very much appreciate hints and guidance about proper PR management, coding standards, etiquette, procedures, norms, etc.! I'm still pretty new to this project but I find it really enjoyable.  However, I don't seem to have the privileges to change those fields.  Do I need to seek advanced privileges from somebody?

Created attachment 41396 [details]
proposed fix v2 part 1

So I've moved the body of do_test_(un)aligned into inline gcc where I can use the template to pass the offsets within test_data.  This splits up the assembly and makes it a bit harder to decipher, but it also cleans up access to test_data struct members.

I've hard-coded the "-m64" into the CFLAGS for now and this should be fine since the test only runs on 64-bit x86 and only when the remote is native.  If I ever figure out where this is usually fed from, I'll swap that out. :)

Anyway, if you can test it again for me and let me know what you think I would appreciate it.  I've got some other code formatting changes I want to send with it, but I separated it out from this patch to simplify reading.  I'll post the second patch anyway though.

Thanks,
Daniel

Created attachment 41397 [details]
proposed fix v2 part 1

Oops, I forgot to re-add my parallelize.exp script to the commit.

Created attachment 41398 [details]
proposed fix v2 part 2

Formatting, comments and other aesthetic changes.

> --- Comment #10 from Daniel Santos <daniel.santos at pobox dot com> ---
[...]
> Anyway, if you can test it again for me and let me know what you think I would
> appreciate it.  I've got some other code formatting changes I want to send with
> it, but I separated it out from this patch to simplify reading.  I'll post the
> second patch anyway though.

Just a quick note with first test results, more later:

* With the patch, a sequential test works on i686-pc-linux-gnu (both
  multilibs).

* On i386-pc-solaris2.12 with /bin/as, do-test.S fails to assemble:

spawn /var/gcc/regression/trunk/12-gcc/build/gcc/xgcc -B/var/gcc/regression/trunk/12-gcc/build/gcc/ -I/var/gcc/regression/trunk/12-gcc/build/gcc/testsuite/gcc/ms-sysv -I/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv -m64 -fno-diagnostics-show-caret -fdiagnostics-color=never -O2 -Wall -c -o /var/gcc/regression/trunk/12-gcc/build/gcc/testsuite/gcc/ms-sysv/do-test.o /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S
Assembler: 
        "/var/tmp//ccTucmac.s", line 3 : Multiply defined label: "regs_to_mem"

.global regs_to_mem; .type regs_to_mem,@function; regs_to_mem:
regs_to_mem:

        "/var/tmp//ccTucmac.s", line 25 : Multiply defined label: "mem_to_regs"

.global mem_to_regs; .type mem_to_regs,@function; mem_to_regs:
mem_to_regs:

        "/var/tmp//ccTucmac.s", line 45 : Symbol "regs_to_mem" already has a size

.size regs_to_mem,.-regs_to_mem

WARNING: Could not assemble /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S

  The first two can be avoided by removing the explicit function labels
  which are already covered by ELFFN_BEGIN.  The last error is due to a
  wrong call to FUNC_END: the second call should be for mem_to_regs, not
  regs_to_mem.

* On i386-pc-solaris2.12 with gas 2.28, this error doesn't happen, but I
  get

WARNING: Could not build /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x8
6_64/abi/ms-sysv/ms-sysv.c.
FAIL: gcc.target/x86_64/abi/ms-sysv CFLAGS+="-O2" generator_args="-p0-5"
PASS: gcc.target/x86_64/abi/ms-sysv CFLAGS+="-O0 -g3" generator_args="-p0-5 --om
it-rbp-clobbers"
WARNING: Could not build /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x8
6_64/abi/ms-sysv/ms-sysv.c.
FAIL: gcc.target/x86_64/abi/ms-sysv CFLAGS+="-mcall-ms2sysv-xlogues -O2" generat
or_args="-p0-5"
WARNING: Link failed.
FAIL: gcc.target/x86_64/abi/ms-sysv CFLAGS+="-mcall-ms2sys

  The first two instances of ms-sysv.c fail to compile:

In file included from /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c:149:0:
/var/gcc/regression/trunk/12-gcc-gas/build/gcc/testsuite/gcc/ms-sysv/ms-sysv-generated.h: In function 'msabi_02_0':
/var/gcc/regression/trunk/12-gcc-gas/build/gcc/testsuite/gcc/ms-sysv/ms-sysv-generated.h:205:1: error: bp cannot be used in asm here

__attribute__ ((noinline, ms_abi)) long msabi_02_0 ()
{
  __asm__ __volatile__ ("" :::"rbp");
  return sysv_0_noinfo ();
}

WARNING: Could not build /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c.

  The last instance of ms-sysv.exe doesn't link:

Undefined                       first referenced
 symbol                             in file
__resms64f_12                       /var/gcc/regression/trunk/12-gcc/build/gcc/testsuite/gcc/ms-sysv/ms-sysv.o
__resms64f_13                       /var/gcc/regression/trunk/12-gcc/build/gcc/testsuite/gcc/ms-sysv/ms-sysv.o
__resms64f_14                       /var/gcc/regression/trunk/12-gcc/build/gcc/testsuite/gcc/ms-sysv/ms-sysv.o
__resms64f_15                       /var/gcc/regression/trunk/12-gcc/build/gcc/testsuite/gcc/ms-sysv/ms-sysv.o
__resms64f_16                       /var/gcc/regression/trunk/12-gcc/build/gcc/testsuite/gcc/ms-sysv/ms-sysv.o
__resms64f_17                       /var/gcc/regression/trunk/12-gcc/build/gcc/testsuite/gcc/ms-sysv/ms-sysv.o
__savms64f_12                       /var/gcc/regression/trunk/12-gcc/build/gcc/testsuite/gcc/ms-sysv/ms-sysv.o
__savms64f_13                       /var/gcc/regression/trunk/12-gcc/build/gcc/testsuite/gcc/ms-sysv/ms-sysv.o
__savms64f_14                       /var/gcc/regression/trunk/12-gcc/build/gcc/testsuite/gcc/ms-sysv/ms-sysv.o
__savms64f_15                       /var/gcc/regression/trunk/12-gcc/build/gcc/testsuite/gcc/ms-sysv/ms-sysv.o
__savms64f_16                       /var/gcc/regression/trunk/12-gcc/build/gcc/testsuite/gcc/ms-sysv/ms-sysv.o
__savms64f_17                       /var/gcc/regression/trunk/12-gcc/build/gcc/testsuite/gcc/ms-sysv/ms-sysv.o
__resms64fx_12                      /var/gcc/regression/trunk/12-gcc/build/gcc/testsuite/gcc/ms-sysv/ms-sysv.o
__resms64fx_13                      /var/gcc/regression/trunk/12-gcc/build/gcc/testsuite/gcc/ms-sysv/ms-sysv.o
__resms64fx_14                      /var/gcc/regression/trunk/12-gcc/build/gcc/testsuite/gcc/ms-sysv/ms-sysv.o
__resms64fx_15                      /var/gcc/regression/trunk/12-gcc/build/gcc/testsuite/gcc/ms-sysv/ms-sysv.o
__resms64fx_16                      /var/gcc/regression/trunk/12-gcc/build/gcc/testsuite/gcc/ms-sysv/ms-sysv.o
__resms64fx_17                      /var/gcc/regression/trunk/12-gcc/build/gcc/testsuite/gcc/ms-sysv/ms-sysv.o
ld: fatal: symbol referencing errors
collect2: error: ld returned 1 exit status

  No idea what's going on here.

	Rainer

> --- Comment #9 from Daniel Santos <daniel.santos at pobox dot com> ---
[...]
>> sure, though there's no need at all (except for the .struct part) to do
>> the testing on Solaris.  I believe there are ready-made Solaris/x86
>> VirtualBox images, though.
>
> I've found a few, so going to try them out when I get some time.  Oracle even
> has something on their downloads.  I haven't used Solaris since the early
> aughts.

As I said, you can try that if you're really motivated, but the problems
at hand are mostly not Solaris-related at all.

>> For the multilib problem, you can easily
>> configure gcc for i686-pc-linux-gnu with --enable-targets=all on a
>> Linux/x86_64 box (with a few necessary 32-bit development packages
>> added), so the default multilib is non-x86_64, while the x86_64 multilib
>> is only used with -m64.
>
> Hmm, I seem to be having problems getting this to work.  Would I configure with
> --target=i686-pc-linux-gnu --enable-targets=all --enable-multilib?

You need to make certain to have the necessary 32-bit libraries and
headers.  Apart from that, configure --target=i686-pc-linux-gnu
--enable-targets=all should be enough, together with CC='gcc -m32'
CXX='g++ -m32'.  I don't pass --enable-multilib, this happens by default.

>> However, I still don't understand why you are jumping through all these
>> hoops in ms-sysv.exp doing the compilations etc. manually rather than
>> just relying on dg-runtest or similar.  This would avoid all this
>> multilib trouble nicely, and massivly reduce ms-sysv.exp.
>
> Well quite frankly because dg-runtest, et. al. don't offer support for tests
> that use code generators.  The generated headers using the default options are

Why would they need to?  You just generate the headers in advance and
than invoke dg-runtest to compile and run the ms-sysv.c test proper.

> between 4.4 and 6 MiB in size and there are more things that need to be tested
> (-fsplit-stack to name one) that isn't tested now.  I would also like to add a
> feature where defining an environment variable generates more comprehensive
> tests that I wouldn't want to run for every test (as it could take hours with
> --enable-checking=all,rtl).

I'd strongly suggest only invoking the basic tests during a regular
testsuite run and control additional test modes with an environment
variable as you suggest.

> The most behaviorally similar test currently in the tree is
> gcc/testsuite/gcc.dg/compat/struct-layout-1.exp, which builds a generator
> (using remote_exec), runs the generator (remote_exec again) to generate sources
> for all tests and then builds and runs each test using (using compat-execute). 

Right, but the test executions proper are done with
${tool}_target_compile, which also underly dg-test/dg-runtest.

> Calls to remote_exec are not automatically parallelized.  I don't fully
> understand how the gcc/testsuite/lib/compat.exp library works, but I'm guessing
> that calls to compat-execute are parallelized by dejagnu.
>
> The scheme that struct-layout-1 uses builds the generator and creates sources
> for all of the tests in job directory (i.e.,
> gcc/testsuite/gcc{,1,2,3,4,5,6,etc.}/gcc.dg-struct-layout-1).  They take up
> 1.21 MiB per job, so -j48 results in 58 MiB of space usage.  My generator and
> generated sources are larger, and currently take about 11.65 MiB per job, so
> -j48 would eat 559 MiB of disk space, even though there are only 6 tests at the
> moment.  This could be mitigated if there was a way to build and run the
> generator only once and have the output go to a directory shared across jobs,
> but I'm not yet aware of any such existing mechanism.

I can't help but get the the feeling that your doing very much premature
optimization here.  If I run the test sequentially on an Opteron 8435,
it takes less than 3 hours.  Investing very much work and complexity to
parallelize this (starting with invoking the generator only once instead
of once per parallel execution, which saves ca. 9 seconds each) doesn't
seem a good tradeoff to me.

> So if you have some better ideas on how to accomplish this then please do
> present them.  Or maybe I'm misunderstanding something about the way
> dg-runtest, gcc_target_compile, etc. work in relation to parallelism?  My
> understanding is that if I use them in succession for a single test run (i.e.,
> build the generator, run the generator, build & run the test) that they could
> end up being run on different jobs and then fail.

I'd forget about the parallelism for the moment (unless I'm missing
something) and get the basics working sequentially.  The attached patch
(on top of your last one) switches the whole thing to dg-runtest.  This
works sequentially, apart from the complilation failure on Solaris I
reported last. I haven't tried what happens if you try it in parallel,
but if it causes problems, parallelism can easily be disabled with
gcc_parallel_test_enable 0/1.

> I very much appreciate hints and guidance about proper PR management, coding
> standards, etiquette, procedures, norms, etc.! I'm still pretty new to this
> project but I find it really enjoyable.  However, I don't seem to have the
> privileges to change those fields.  Do I need to seek advanced privileges from
> somebody?

Write privilege in Bugzilla is usually tied to write-after-approval
rights in svn.  I see you don't have the latter yet.  Sorry for barking
up the wrong tree.

	Rainer

Created attachment 41404 [details]
Switch ms-sysv to more regular dg functions

Sorry for my delayed response.  I'm working on adding extended tests triggered by an environment variable (because I needed to better validate somebody else's changes to my -mcall-ms2sysv-xlogues feature and I encountered a flaw in my test program when the header is generated with a -p range of 7 or higher.

(In reply to ro@CeBiTec.Uni-Bielefeld.DE from comment #14)
> You need to make certain to have the necessary 32-bit libraries and
> headers.  Apart from that, configure --target=i686-pc-linux-gnu
> --enable-targets=all should be enough, together with CC='gcc -m32'
> CXX='g++ -m32'.  I don't pass --enable-multilib, this happens by default.

I seem to have encountered some type of bug in something here (autoconf maybe?).  When the build gets to running configure on libgcc, it fails with:

configure: error: cannot compute suffix of object files: cannot compile
See `config.log' for more details.
make[1]: *** [Makefile:13549: configure-target-libgcc] Error 1
make[1]: Leaving directory '/home/daniel/proj/sys/gcc/builds/testsuite-fixes-i686-pc-linux-gnu'
make: *** [Makefile:898: all] Error 2


The failure is actually when it's calling exec with -- and the option isn't recognized:


configure:3469: /home/daniel/proj/sys/gcc/builds/testsuite-fixes-i686-pc-linux-gnu/./gcc/xgcc -B/home/daniel/proj/sys/gcc/builds/testsuite-fixes-i686-pc-linux-gnu/./gc
c/ -B/usr/local/i686-pc-linux-gnu/bin/ -B/usr/local/i686-pc-linux-gnu/lib/ -isystem /usr/local/i686-pc-linux-gnu/include -isystem /usr/local/i686-pc-linux-gnu/sys-incl
ude    -o conftest -g -O2   conftest.c  >&5
/home/daniel/proj/sys/gcc/builds/testsuite-fixes-i686-pc-linux-gnu/./gcc/as: line 106: exec: --: invalid option
exec: usage: exec [-cl] [-a name] [command [arguments ...]] [redirection ...]
configure:3472: $? = 1
configure:3660: checking for suffix of object files
configure:3682: /home/daniel/proj/sys/gcc/builds/testsuite-fixes-i686-pc-linux-gnu/./gcc/xgcc -B/home/daniel/proj/sys/gcc/builds/testsuite-fixes-i686-pc-linux-gnu/./gc
c/ -B/usr/local/i686-pc-linux-gnu/bin/ -B/usr/local/i686-pc-linux-gnu/lib/ -isystem /usr/local/i686-pc-linux-gnu/include -isystem /usr/local/i686-pc-linux-gnu/sys-incl
ude    -c -g -O2  conftest.c >&5
/home/daniel/proj/sys/gcc/builds/testsuite-fixes-i686-pc-linux-gnu/./gcc/as: line 106: exec: --: invalid option
exec: usage: exec [-cl] [-a name] [command [arguments ...]] [redirection ...]
configure:3686: $? = 1


I don't fancy myself an autotools expert, so I'm not yet certain that it's a bug.  I'm using gentoo with 

app-shells/bash-4.3_p48-r1
sys-devel/autoconf-2.64 (among others)
sys-devel/binutils-2.26.1

My spidy-sense tells me to try out binutils 2.27 or some such.

I'll get back on the rest of this soon.

(In reply to Rainer Orth from comment #15)
> Created attachment 41404 [details]
> Switch ms-sysv to more regular dg functions

You may be surprised to learn how many faulty assumptions you may have about how the gcc test harness works and manages parallelization.  I did try your patch and it didn't work.  I didn't go too deeply into trying to analyze the failures, but if you call dg-runtest, you are using gcc's hack-daptation of parallelization.  However, your patch doesn't remove *my* hack-daptation of parallelization, so we end up with two different parallelization schemes that step on each other's toes.

Another problem with the already present parallelization is that it bunches tests into groups of 10 per job which will perform very poorly for these tests.  (https://github.com/gcc-mirror/gcc/blob/master/gcc/testsuite/lib/gcc-defs.exp#L170).  I presume this is to reduce disk I/O and it makes sense from that standpoint (I don't want to know what it would take to get a ramdisk/tmpfs in a platform-neutral fashion.)

However, I'm learning a little more about how the test harness works, and it MAY be possible to call gcc_parallel_test_enable 0 at the start of ms-sysv.exp and be able to use all of the built-in dg-runtest, et. al. functions!  If I can get this to work (and not break something else in the process), then we may be on to a pathway to clean up ms-sysv.exp a little bit -- that is except for a few outstanding (possibly surmountable) issues:

1.) Can the default time-out of 5 minutes be changed?  I need 20 minutes for the slowest processors and a whole HOUR when full tests are enabled.

2.) The test description should include the generator flags and not just the CFLAGS.  Is that possible from dg-runtest, et. al.?  I suppose it's always possible to add them to CFLAGS with -DGEN_FLAGS="-p0-12" as a hack.

I guess you can see why I said that I was "semi-content" to leave it like it is. :)  But I'm also glad to better understand how the test harness parallelization works.  Maybe it's possible to make a small modification to dg-defs.exp to get it to divvy out a single test per job instead of 10.

I intended to respond to your comments from 6 days ago sooner, but better late than never!  Again, sorry for the delay

(In reply to ro@CeBiTec.Uni-Bielefeld.DE from comment #14)
> You need to make certain to have the necessary 32-bit libraries and
> headers.  Apart from that, configure --target=i686-pc-linux-gnu
> --enable-targets=all should be enough, together with CC='gcc -m32'
> CXX='g++ -m32'.  I don't pass --enable-multilib, this happens by default.

I think I'll need to figure this part out soon, as I posted earlier about strange problems when calling exec.  As it turns out, there are a lot of variables in the generated script that aren't being populated, ORIGINAL_AS_FOR_TARGET being one of them.  The "exec: --: invalid option" problem is occurs because ORIGINAL_AS_FOR_TARGET expands to an empty string and the first thing after "exec" is the argument "--32" that is intended for the AS program.

If it turns out that this is because I AM missing some 32-bit library, then we still have a bug somewhere as some configure script should fail (but it's failing to fail :).  I build my x86_64 Gentoo systems with ABI_X86="64 32", so I should have all 32 bit libraries for pretty much everything.

> >> However, I still don't understand why you are jumping through all these
> >> hoops in ms-sysv.exp doing the compilations etc. manually rather than
> >> just relying on dg-runtest or similar.  This would avoid all this
> >> multilib trouble nicely, and massivly reduce ms-sysv.exp.
> >
> > Well quite frankly because dg-runtest, et. al. don't offer support for tests
> > that use code generators.  The generated headers using the default options are
> 
> Why would they need to?  You just generate the headers in advance and
> than invoke dg-runtest to compile and run the ms-sysv.c test proper.

The side-effect of this is that we build the code generator in every -j<job> and run it prior to every call to dg-runtest on each job.  The building takes a few seconds and generating the header is almost instant, but it's still a lot of I/O.  Some of this will likely get mitigated by the I/O scheduler, when a previously written header hasn't been committed to disk yet and a second write removes the previous one from the I/O queue, but it's still a lot of waste IHMO.  And as I mentioned before, if you build with -j48, you'll still end up wasting a lot of disk space, I think around half a gig of space.

The second problem is that using dg-runtest assigns tests to jobs in bunches of 10.  At current, there are 4 tests and they would all be assigned to a single CPU, even though each CPU would have to build and run the code generator.

Now I have been digging deeper into gcc-defs.exp, and I may have a clean solution to this problem and possibly a way to make the entire test harness perform better job distribution, but I think I should fix the real problems here first and do this other work as subsequent patches.  If my guess is correct, with a few changes to gcc-defs.exp, I can use dg-runtest (a la your patch) AND eliminate all of the parallelization code I've written, while still eliminating all unnecessary builds and execution of the code generator and distributing the tests evenly across CPUs.

 
> > between 4.4 and 6 MiB in size and there are more things that need to be tested
> > (-fsplit-stack to name one) that isn't tested now.  I would also like to add a
> > feature where defining an environment variable generates more comprehensive
> > tests that I wouldn't want to run for every test (as it could take hours with
> > --enable-checking=all,rtl).
> 
> I'd strongly suggest only invoking the basic tests during a regular
> testsuite run and control additional test modes with an environment
> variable as you suggest.

Yes, I agree.  Also, I meant "--enable-checking=yes,rtl" above, not "all" (yikes!).  I've been building with --enable-checking=yes,rtl because I choose an ambitious goal for my fist gcc project and I am very paranoid about breaking something.  I want to do everything I can to detect any flaws in my code.  I am adding more tests that triggered by GCC_TEST_RUN_EXPENSIVE for now.

 
> > The most behaviorally similar test currently in the tree is
> > gcc/testsuite/gcc.dg/compat/struct-layout-1.exp, which builds a generator
> > (using remote_exec), runs the generator (remote_exec again) to generate sources
> > for all tests and then builds and runs each test using (using compat-execute). 
> 
> Right, but the test executions proper are done with
> ${tool}_target_compile, which also underly dg-test/dg-runtest.

True, but this test will only use 4-5 CPUs, no matter how many -j<jobs> you throw at it.  I hope to fix this problem with a later patch (as mentioned above).

 
 > I can't help but get the the feeling that your doing very much premature
> optimization here.

lol! Well, I admit that's one of my hang-ups.... 


> If I run the test sequentially on an Opteron 8435,
> it takes less than 3 hours.  Investing very much work and complexity to
> parallelize this (starting with invoking the generator only once instead
> of once per parallel execution, which saves ca. 9 seconds each) doesn't
> seem a good tradeoff to me.

I can't argue that point!  But if I can tune the whole test harness, it might be worthwhile, especially if I can reduce complexity in my own test.


> I'd forget about the parallelism for the moment (unless I'm missing
> something) and get the basics working sequentially.  The attached patch
> (on top of your last one) switches the whole thing to dg-runtest.  This
> works sequentially, apart from the complilation failure on Solaris I
> reported last. I haven't tried what happens if you try it in parallel,
> but if it causes problems, parallelism can easily be disabled with
> gcc_parallel_test_enable 0/1.

I just discovered that!  And parallelism could then be manually accessed via gcc_parallel_test_run_p.

 
> Write privilege in Bugzilla is usually tied to write-after-approval
> rights in svn.  I see you don't have the latter yet.  Sorry for barking
> up the wrong tree.
> 
> 	Rainer

Darn! I thought I might get some more privs out of this. :)

Thank you for all of your assistance, feedback and recommendations!

Daniel

> --- Comment #18 from Daniel Santos <daniel.santos at pobox dot com> ---
> I intended to respond to your comments from 6 days ago sooner, but better late
> than never!  Again, sorry for the delay

No worries at all: I'm way late here myself ;-)

> (In reply to ro@CeBiTec.Uni-Bielefeld.DE from comment #14)
>> You need to make certain to have the necessary 32-bit libraries and
>> headers.  Apart from that, configure --target=i686-pc-linux-gnu
>> --enable-targets=all should be enough, together with CC='gcc -m32'
>> CXX='g++ -m32'.  I don't pass --enable-multilib, this happens by default.
>
> I think I'll need to figure this part out soon, as I posted earlier about
> strange problems when calling exec.  As it turns out, there are a lot of
> variables in the generated script that aren't being populated,
> ORIGINAL_AS_FOR_TARGET being one of them.  The "exec: --: invalid option"
> problem is occurs because ORIGINAL_AS_FOR_TARGET expands to an empty string and
> the first thing after "exec" is the argument "--32" that is intended for the AS
> program.

I now remembered what you're probably missing: as soon as configure and
the gcc build process are run with --target=<some target> which differs
from the (guessed by config.sub) values for --host and --build, they
conclude that a cross-configuration/-build is running and make their set
of assumptions on how to locate cross-ar/as/ld.  In the case at hand
(and any multilibbed target in general), this is wrong: this is *not* a
cross-build since the host can execute both 32 and 64-bit binaries and
many of those assumptions are wrong.

The way to convince configure of that fact is to run it with

--build=i686-pc-linux-gnu --host=i686-pc-linux-gnu --target=i686-pc-linux-gnu

This way, you'll get a native build and all problems should vanish.

While in theory both /usr/bin/as and /usr/bin/ld should be capable of
performing both 32 and 64-bit assemblies/links, I found it best to
provide an i686-pc-linux-gnu gas and gld with

--with-as=<path to gas> --with-gnu-as --with-ld=<path to ld> --with-gnu-ld

to avoid a couple of corner cases.  You should configure binutils in the
same way, perhaps again with CC='gcc -m32' CXX='g++ -m32'.

I'll comment on the rest later once I find some time.

	Rainer

> --- Comment #17 from Daniel Santos <daniel.santos at pobox dot com> ---
> (In reply to Rainer Orth from comment #15)
>> Created attachment 41404 [details]
>> Switch ms-sysv to more regular dg functions
>
> You may be surprised to learn how many faulty assumptions you may have about
> how the gcc test harness works and manages parallelization.  I did try your
> patch and it didn't work.  I didn't go too deeply into trying to analyze the

I didn't even bother to try a parallel run before getting basic
functionality right!  Should have mentioned that explicitly.

> failures, but if you call dg-runtest, you are using gcc's hack-daptation of
> parallelization.  However, your patch doesn't remove *my* hack-daptation of
> parallelization, so we end up with two different parallelization schemes that
> step on each other's toes.
>
> Another problem with the already present parallelization is that it bunches
> tests into groups of 10 per job which will perform very poorly for these tests.
>
> (https://github.com/gcc-mirror/gcc/blob/master/gcc/testsuite/lib/gcc-defs.exp#L170).
>  I presume this is to reduce disk I/O and it makes sense from that standpoint
> (I don't want to know what it would take to get a ramdisk/tmpfs in a
> platform-neutral fashion.)

My basic point still stands: running your ms-sysv tests sequentially
takes just a few minutes even on an old and (by today's standards) slow
CPU, so there's absolutely no point investing lots of effort and
complexity to parallelize what already runs adequately fast sequentially!

> However, I'm learning a little more about how the test harness works, and it
> MAY be possible to call gcc_parallel_test_enable 0 at the start of ms-sysv.exp
> and be able to use all of the built-in dg-runtest, et. al. functions!  If I can
> get this to work (and not break something else in the process), then we may be
> on to a pathway to clean up ms-sysv.exp a little bit -- that is except for a
> few outstanding (possibly surmountable) issues:
>
> 1.) Can the default time-out of 5 minutes be changed?  I need 20 minutes for
> the slowest processors and a whole HOUR when full tests are enabled.

Sure it can: for one there are dg-timeout (and preferably dg-timeout
factor) per testcase.  I still wonder why you'd need that, though: if
all your tests together take no more than a few minutes, why would you
need to increase the timeout at all?  Which processor would this be that
takes 20 minutes or even an hour to run the tests *and complete all
other tests well within the five minute timeout*?

In fact, every test that takes more than about a minute on a resonably
current CPU is frowned upon because under parallel testing/load such
tests tend to run into the timeout.

If your tests regularly exceed the timeout, there's something wrong with
them: you need to split the so individual tests complete within the
minute just mentioned.

If really necessary in a setup, it is possible to set
board_info(unix,gcc,timeout) in a global site.exp file, e.g. to deal
with really slow/ancient systems.  This would be necessary without your
test anyway.

> 2.) The test description should include the generator flags and not just the
> CFLAGS.  Is that possible from dg-runtest, et. al.?  I suppose it's always
> possible to add them to CFLAGS with -DGEN_FLAGS="-p0-12" as a hack.

That's a requirement actually: the summary lines for different runs of a
test must differ so you can tell them apart if one of them fails.  How
this is done in the end is primarily a cosmetic issue, though.

> I guess you can see why I said that I was "semi-content" to leave it like it
> is. :)  But I'm also glad to better understand how the test harness
> parallelization works.  Maybe it's possible to make a small modification to
> dg-defs.exp to get it to divvy out a single test per job instead of 10.

As I said: first get the sequential execution right and then, if really
really necessary (and I want proof for that) look at parallelization.
It may well be that the initial solution is to restrict the number and
size of tests run by everyone.  After all, this is just for a niche
feature of a single architecture/OS and there's no point in everyone
testing on x86 having to pay a massive penalty for that.

	Rainer

(In reply to ro@CeBiTec.Uni-Bielefeld.DE from comment #20)
> > failures, but if you call dg-runtest, you are using gcc's hack-daptation of
> > parallelization.  However, your patch doesn't remove *my* hack-daptation of
> > parallelization, so we end up with two different parallelization schemes that
> > step on each other's toes.
> >
> > Another problem with the already present parallelization is that it bunches
> > tests into groups of 10 per job which will perform very poorly for these tests.
> >
> > (https://github.com/gcc-mirror/gcc/blob/master/gcc/testsuite/lib/gcc-defs.exp#L170).
> >  I presume this is to reduce disk I/O and it makes sense from that standpoint
> > (I don't want to know what it would take to get a ramdisk/tmpfs in a
> > platform-neutral fashion.)
> 
> My basic point still stands: running your ms-sysv tests sequentially
> takes just a few minutes even on an old and (by today's standards) slow
> CPU, so there's absolutely no point investing lots of effort and
> complexity to parallelize what already runs adequately fast sequentially!

There is plenty of point!  It may be fast without --enable-checking=rtl, but it's very slow with it.  A very large portion of my development lifecycle was spent waiting for tests to run.  Using --enable-checking=rtl caught SO many errors that didn't (or might not have) cause an ICE without it.

Now at the time, all I had was a phenom and when I had more than 64 tests run in a single function it was very slow (I presume due to thrashing the data cache) which is the reason the generator splits the tests out into multiple functions that run 64 tests each.  I have a nice quad i7 now, so it's going faster.  But one thing I hadn't gotten back to yet was adding more extensive tests using features and optimizations that effect the stack (-fsplit-stack, -pg, etc.).

 
> > However, I'm learning a little more about how the test harness works, and it
> > MAY be possible to call gcc_parallel_test_enable 0 at the start of ms-sysv.exp
> > and be able to use all of the built-in dg-runtest, et. al. functions!  If I can
> > get this to work (and not break something else in the process), then we may be
> > on to a pathway to clean up ms-sysv.exp a little bit -- that is except for a
> > few outstanding (possibly surmountable) issues:
> >
> > 1.) Can the default time-out of 5 minutes be changed?  I need 20 minutes for
> > the slowest processors and a whole HOUR when full tests are enabled.
> 
> Sure it can: for one there are dg-timeout (and preferably dg-timeout
> factor) per testcase.  I still wonder why you'd need that, though: if
> all your tests together take no more than a few minutes, why would you
> need to increase the timeout at all?  Which processor would this be that
> takes 20 minutes or even an hour to run the tests *and complete all
> other tests well within the five minute timeout*?

Thanks for that!  I *may* have a better solution (described below).  But the 5 minute timeout even happens on my new i7 when --enable-checking=rtl is on.
 
> In fact, every test that takes more than about a minute on a resonably
> current CPU is frowned upon because under parallel testing/load such
> tests tend to run into the timeout.

The time is taken during compilation and also during linking when -flto is used (again, with rtl checking).

> If your tests regularly exceed the timeout, there's something wrong with
> them: you need to split the so individual tests complete within the
> minute just mentioned.

Hah! That is actually the direction I had decided to go and have been testing it the last few days. :)  However, they can still run longer than 1 minute (with rtl checking).  I can split them apart even further with a few changes to the code generator.

> If really necessary in a setup, it is possible to set
> board_info(unix,gcc,timeout) in a global site.exp file, e.g. to deal
> with really slow/ancient systems.  This would be necessary without your
> test anyway.
> 
> > 2.) The test description should include the generator flags and not just the
> > CFLAGS.  Is that possible from dg-runtest, et. al.?  I suppose it's always
> > possible to add them to CFLAGS with -DGEN_FLAGS="-p0-12" as a hack.
> 
> That's a requirement actually: the summary lines for different runs of a
> test must differ so you can tell them apart if one of them fails.  How
> this is done in the end is primarily a cosmetic issue, though.

I seem to have worked this out, although I have to do a regsub to replace spaces in the generator_args with an escaped space and it prints a little ugly, but at least it works.

    set escaped_generator_args [regsub -all " " $generator_args "\\ "]
    set cflags "$cflags\"-DGEN_ARGS=$escaped_generator_args\""

> > I guess you can see why I said that I was "semi-content" to leave it like it
> > is. :)  But I'm also glad to better understand how the test harness
> > parallelization works.  Maybe it's possible to make a small modification to
> > dg-defs.exp to get it to divvy out a single test per job instead of 10.
> 
> As I said: first get the sequential execution right and then, if really
> really necessary (and I want proof for that) look at parallelization.
> It may well be that the initial solution is to restrict the number and
> size of tests run by everyone.  After all, this is just for a niche
> feature of a single architecture/OS and there's no point in everyone
> testing on x86 having to pay a massive penalty for that.
> 
> 	Rainer

This is a niche feature and only for x86_64, but it targets every *nix OS where Wine can run including Mac, GNU/Linux, Solaris and I think BSD too.  (btw, Wine actually runs on ARM now!)  We don't need this on Windows.  But to my knowledge, Wine would be the only project to benefit from it (so "niche" is accurate).

Spitting out the tests out solves a few problems -- the timeouts and the header file size is much smaller.  It even ends up distributing the tests a tiny bit better (it uses 2 CPUs :)

Separate from that, I have a change set that provides a mechanism to tune parallelization.  It works by a .exp file disclosing in advance how many tests they plan to run and calculating the tests-at-a-time from that value.  One non-pretty aspect of it is how I grab the number of jobs being run:

    global gcc_runtest_parallelize_njobs

    if { [info exists env(MAKEFLAGS)] } {
	set njobs [regsub "^.*?(?: -)?j(\\d+).*?$" $env(MAKEFLAGS) "\\1" ]
	if [regexp "^\\d+$" $njobs match] then {
	    set gcc_runtest_parallelize_njobs $njobs
	}
    }

Anyway, I'm re-working this into a logical progression of patches:

1.) First fixing broken things (there is an additional test error with "-p7"
    in the generator arguments),
2.) Cleaning up some bad code formatting, inconsistencies and other cosmetics,
3.) Adding an option to the generator to control how tests are split
    across functions.
4.) Adding an extensive set of tests using torture flags and that are
    triggered by GCC_TEST_RUN_EXPENSIVE (if you think I should use a
    different environment variable, please let me know),
5.) And then my proposed parallelization tweaks.

This way we can at least get the broken things fixed immediately and punt parallelization in the (likely) event that my initial proposal isn't yet ready for prime time.

Thanks,
Daniel

Created attachment 41486 [details]
proposed fix v3 part 1

Rainer,

I thought I would post this here before posting to the list since I still don't have a useable i686 build to test with.  Either way, I *think* all of the Solaris problems should be fixed.  This patch set addresses a number of other issues as well and ends with a proposed approach to tune parallelization.

Created attachment 41487 [details]
proposed fix v3 part 2

Created attachment 41488 [details]
proposed fix v3 part 3

Created attachment 41489 [details]
proposed fix v3 part 4

Created attachment 41490 [details]
proposed fix v3 part 5

I'm currently running a few jobs to try to measure the difference in load average and running time of tests.  I can think of a whole lot of better solutions to this issue, but I suppose I will need to throw a few numbers out there if I'm going to assert it's a problem.  With or without this last patch, a lot of other problems should be solved.

> --- Comment #22 from Daniel Santos <daniel.santos at pobox dot com> ---
[...]
> I thought I would post this here before posting to the list since I still don't
> have a useable i686 build to test with.  Either way, I *think* all of the
> Solaris problems should be fixed.  This patch set addresses a number of other
> issues as well and ends with a proposed approach to tune parallelization.

Unfortunately, it doesn't:

* You missed one issue I had reported before:

FAIL: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c  -O2 "-DGEN_ARGS=-p0\ -t64" (test for excess errors)
Excess errors:
/var/gcc/regression/trunk/12-gcc/build/gcc/testsuite/gcc/ms-sysv/ms-sysv-generated.h:30:1: error: bp cannot be used in asm here
Assembler: 
	"/var/tmp//ccVdks6a.s", line 43 : Symbol "regs_to_mem" already has a size

  Fixed like this:

diff --git a/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S b/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S
--- a/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S
+++ b/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S
@@ -90,7 +90,7 @@ FUNC_BEGIN(mem_to_regs)
 	mov	0xd0(%rax),%r14
 	mov	0xd8(%rax),%r15
 	retq
-FUNC_END(regs_to_mem)
+FUNC_END(mem_to_regs)
 
 # NOTE: Not MT safe
 FUNC_BEGIN(do_test_unaligned)

* Also as I'd reported before, with the fix above, I still get a couple
  of FAILures:

FAIL: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c  -O2 "-DGEN_ARGS=-p0\ -t64" (test for excess errors)
WARNING: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c  -O2 "-DGEN_ARGS=-p0\ -t64" compilation failed to produce executable
FAIL: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c -mcall-ms2sysv-xlogues -O2 "-DGEN_ARGS=-p0\ -t64" (test for excess errors)
WARNING: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c -mcall-ms2sysv-xlogues -O2 "-DGEN_ARGS=-p0\ -t64" compilation failed to produce executable
PASS: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c  -O0 -g3 "-DGEN_ARGS=-p0\ -t64\ --omit-rbp-clobbers" (test for excess errors)
PASS: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c  -O0 -g3 "-DGEN_ARGS=-p0\ -t64\ --omit-rbp-clobbers" execution test
FAIL: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c -mcall-ms2sysv-xlogues -O0 -g3 "-DGEN_ARGS=-p0\ -t64\ --omit-rbp-clobbers" (test for excess errors)
WARNING: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c -mcall-ms2sysv-xlogues -O0 -g3 "-DGEN_ARGS=-p0\ -t64\ --omit-rbp-clobbers" compilation failed to produce executable
FAIL: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c  -O2 "-DGEN_ARGS=-p1\ -t64" (test for excess errors)
WARNING: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c  -O2 "-DGEN_ARGS=-p1\ -t64" compilation failed to produce executable
FAIL: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c -mcall-ms2sysv-xlogues -O2 "-DGEN_ARGS=-p1\ -t64" (test for excess errors)
WARNING: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c -mcall-ms2sysv-xlogues -O2 "-DGEN_ARGS=-p1\ -t64" compilation failed to produce executable
PASS: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c  -O0 -g3 "-DGEN_ARGS=-p1\ -t64\ --omit-rbp-clobbers" (test for excess errors)
PASS: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c  -O0 -g3 "-DGEN_ARGS=-p1\ -t64\ --omit-rbp-clobbers" execution test
FAIL: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c -mcall-ms2sysv-xlogues -O0 -g3 "-DGEN_ARGS=-p1\ -t64\ --omit-rbp-clobbers" (test for excess errors)
WARNING: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c -mcall-ms2sysv-xlogues -O0 -g3 "-DGEN_ARGS=-p1\ -t64\ --omit-rbp-clobbers" compilation failed to produce executable
FAIL: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c  -O2 "-DGEN_ARGS=-p5\ -t64" (test for excess errors)
WARNING: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c  -O2 "-DGEN_ARGS=-p5\ -t64" compilation failed to produce executable
FAIL: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c -mcall-ms2sysv-xlogues -O2 "-DGEN_ARGS=-p5\ -t64" (test for excess errors)
WARNING: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c -mcall-ms2sysv-xlogues -O2 "-DGEN_ARGS=-p5\ -t64" compilation failed to produce executable
PASS: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c  -O0 -g3 "-DGEN_ARGS=-p5\ -t64\ --omit-rbp-clobbers" (test for excess errors)
PASS: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c  -O0 -g3 "-DGEN_ARGS=-p5\ -t64\ --omit-rbp-clobbers" execution test
FAIL: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c -mcall-ms2sysv-xlogues -O0 -g3 "-DGEN_ARGS=-p5\ -t64\ --omit-rbp-clobbers" (test for excess errors)
WARNING: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c -mcall-ms2sysv-xlogues -O0 -g3 "-DGEN_ARGS=-p5\ -t64\ --omit-rbp-clobbers" compilation failed to produce executable
FAIL: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c  -O2 "-DGEN_ARGS=-p8\ -t64" (test for excess errors)
WARNING: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c  -O2 "-DGEN_ARGS=-p8\ -t64" compilation failed to produce executable
FAIL: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c -mcall-ms2sysv-xlogues -O2 "-DGEN_ARGS=-p8\ -t64" (test for excess errors)
WARNING: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c -mcall-ms2sysv-xlogues -O2 "-DGEN_ARGS=-p8\ -t64" compilation failed to produce executable
PASS: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c  -O0 -g3 "-DGEN_ARGS=-p8\ -t64\ --omit-rbp-clobbers" (test for excess errors)
PASS: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c  -O0 -g3 "-DGEN_ARGS=-p8\ -t64\ --omit-rbp-clobbers" execution test
FAIL: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c -mcall-ms2sysv-xlogues -O0 -g3 "-DGEN_ARGS=-p8\ -t64\ --omit-rbp-clobbers" (test for excess errors)
WARNING: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c -mcall-ms2sysv-xlogues -O0 -g3 "-DGEN_ARGS=-p8\ -t64\ --omit-rbp-clobbers" compilation failed to produce executable

		=== gcc Summary for unix/-m64 ===

# of expected passes		8
# of unexpected failures	12

FAIL: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c  -O2 "-DGEN_ARGS=-p0\ -t64" (test for excess errors)
Excess errors:
/var/gcc/regression/trunk/12-gcc/build/gcc/testsuite/gcc/ms-sysv/ms-sysv-generated.h:30:1: error: bp cannot be used in asm here

  Full compiler output is

In file included from /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c:158:0:
/var/gcc/regression/trunk/12-gcc/build/gcc/testsuite/gcc/ms-sysv/ms-sysv-generated.h: In function 'msabi_02_0':
/var/gcc/regression/trunk/12-gcc/build/gcc/testsuite/gcc/ms-sysv/ms-sysv-generated.h:30:1: error: bp cannot be used in asm here

  At least some of the tests PASS now :-)

Besides, can you *pretty please* concentrate on the issue at hand in
this PR, i.e. the failing tests on i?86 -m64?  Most/all of your
cleanups, nice as they may be on their own, have nothing to do with the
PR and just distract from the main issue.

Thanks.
        Rainer

> --- Comment #27 from ro at CeBiTec dot Uni-Bielefeld.DE <ro at CeBiTec dot
> Uni-Bielefeld.DE> ---
[...]
> Besides, can you *pretty please* concentrate on the issue at hand in
> this PR, i.e. the failing tests on i?86 -m64?  Most/all of your
> cleanups, nice as they may be on their own, have nothing to do with the
> PR and just distract from the main issue.

I've also included your patches in yesterday night's full bootstraps and
found an additional issue: with -j96 (sparc-sun-solaris2.12) resp. -j24
(i386-pc-solaris2.12), I get many (74 resp. 100) instances of

WARNING: dg_set_test_count should be called prior to running any test.

in mail-report.log.

As I've said before, the parallelization of ms-sysv.exp runs may be a
bonus, but is certainly separate from this PR and thus should be split
out: a sequential run of ms-sysv.exp (-m64/-m32) on x86_64-pc-linux-gnu
takes just 3m36 on a 2.67 GHz Xeon X7542 with default checking options;
nothing to worry about for the common case.

	Rainer

(In reply to ro@CeBiTec.Uni-Bielefeld.DE from comment #28)
> As I've said before, the parallelization of ms-sysv.exp runs may be a
> bonus, but is certainly separate from this PR and thus should be split
> out:

Yes, you are right of course.  I was trying to kill too many birds with one stone and I somehow omitted a bit of your patch for the function size thing, sorry about that.  Some of this gets complicated though, if you want me to use dg-runtest then a few other changes must be made as well, but obviously not as many as I had included.  I'll get this sorted out.

Please also note that I did seek guidance when putting this exp file together (back in December)  I was following Mike Stump's direction, but you were probably on vacation or something. :)  https://gcc.gnu.org/ml/gcc/2016-12/msg00145.html

I've also been motivated to expand the tests by a change somebody else made to my original patch that I wasn't confident the original tests would fully check (been worried about it, but it all looks good).  I'll get a cleaned up patch for you soon.


(In reply to ro@CeBiTec.Uni-Bielefeld.DE from comment #27)

> * Also as I'd reported before, with the fix above, I still get a couple
>   of FAILures:
> 
>
> FAIL: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c  -O2 "-DGEN_ARGS=-p0\ -t64"
> (test for excess errors)
> Excess errors:
> /var/gcc/regression/trunk/12-gcc/build/gcc/testsuite/gcc/ms-sysv/ms-sysv-
> generated.h:30:1: error: bp cannot be used in asm here
> 
>   Full compiler output is
> 
> In file included from
> /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-
> sysv.c:158:0:
> /var/gcc/regression/trunk/12-gcc/build/gcc/testsuite/gcc/ms-sysv/ms-sysv-
> generated.h: In function 'msabi_02_0':
> /var/gcc/regression/trunk/12-gcc/build/gcc/testsuite/gcc/ms-sysv/ms-sysv-
> generated.h:30:1: error: bp cannot be used in asm here
> 
>   At least some of the tests PASS now :-)

Well this is a problem and is unexpected.  Can you please post the relevant portion of the log file?  What I really need to see is the command line to build ms-sysv.c.  I'm going to *guess* that the problem is that TEST_ALWAYS_FLAGS contains something that enables hard frame pointers and that I need this little change:

     # Detect when hard frame pointers are enabled (or required) so we know not
     # to generate bp clobbers.
-    if [regexp "^(.+ +| *)-(O0|fno-omit-frame-pointer|p|pg)( +.*)?$" \
-              $cflags match] then {
+    if [regexp "^( *|.* )-(O0|fno-omit-frame-pointer|p|pg)( *| +.*)$" \
+              "$TEST_ALWAYS_FLAGS $cflags" match] then {
        set generator_args "$generator_args --omit-rbp-clobbers"
     }

We could also just pass --omit-rbp-clobbers to the generator in all cases, although it would weaken the tests.

Thanks,
Daniel

> --- Comment #29 from Daniel Santos <daniel.santos at pobox dot com> ---
> (In reply to ro@CeBiTec.Uni-Bielefeld.DE from comment #28)
>> As I've said before, the parallelization of ms-sysv.exp runs may be a
>> bonus, but is certainly separate from this PR and thus should be split
>> out:
>
> Yes, you are right of course.  I was trying to kill too many birds with one
> stone and I somehow omitted a bit of your patch for the function size thing,
> sorry about that.  Some of this gets complicated though, if you want me to use
> dg-runtest then a few other changes must be made as well, but obviously not as
> many as I had included.  I'll get this sorted out.

Fine.  Imagine your formatting/coding style changes: when they are
intermixed with functional changes, every reviewer has to check
thoroughly which part is which.  If you separate them (which is easy for
you to do) and submit them separately, the formatting stuff will be
obvious or nearly so, and you save everyone time by having to review
only the beef of the functional changes, in the end also giving you
faster review.

> Please also note that I did seek guidance when putting this exp file together
> (back in December)  I was following Mike Stump's direction, but you were
> probably on vacation or something. :) 
> https://gcc.gnu.org/ml/gcc/2016-12/msg00145.html

Might be.  However, I've been appointed testsuite maintainer at a time
when nobody else was around.  Later, Mike stepped up again who's way
more experienced here than I am.  In the case at hand, I happen to be
both victim of your patches' fallout and testsuite maintainer ;-)

> I've also been motivated to expand the tests by a change somebody else made to
> my original patch that I wasn't confident the original tests would fully check
> (been worried about it, but it all looks good).  I'll get a cleaned up patch
> for you soon.

Excellent, thanks for your patience.  I know the first times through the
system can be hard and tedious...

> (In reply to ro@CeBiTec.Uni-Bielefeld.DE from comment #27)
>
>> * Also as I'd reported before, with the fix above, I still get a couple
>>   of FAILures:
>> 
>>
>> FAIL: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c  -O2 "-DGEN_ARGS=-p0\ -t64"
>> (test for excess errors)
>> Excess errors:
>> /var/gcc/regression/trunk/12-gcc/build/gcc/testsuite/gcc/ms-sysv/ms-sysv-
>> generated.h:30:1: error: bp cannot be used in asm here
>> 
>>   Full compiler output is
>> 
>> In file included from
>> /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-
>> sysv.c:158:0:
>> /var/gcc/regression/trunk/12-gcc/build/gcc/testsuite/gcc/ms-sysv/ms-sysv-
>> generated.h: In function 'msabi_02_0':
>> /var/gcc/regression/trunk/12-gcc/build/gcc/testsuite/gcc/ms-sysv/ms-sysv-
>> generated.h:30:1: error: bp cannot be used in asm here
>> 
>>   At least some of the tests PASS now :-)
>
> Well this is a problem and is unexpected.  Can you please post the relevant
> portion of the log file?  What I really need to see is the command line to
> build ms-sysv.c.  I'm going to *guess* that the problem is that
> TEST_ALWAYS_FLAGS contains something that enables hard frame pointers and that
> I need this little change:
>
>      # Detect when hard frame pointers are enabled (or required) so we know not
>      # to generate bp clobbers.
> -    if [regexp "^(.+ +| *)-(O0|fno-omit-frame-pointer|p|pg)( +.*)?$" \
> -              $cflags match] then {
> +    if [regexp "^( *|.* )-(O0|fno-omit-frame-pointer|p|pg)( *| +.*)$" \
> +              "$TEST_ALWAYS_FLAGS $cflags" match] then {
>         set generator_args "$generator_args --omit-rbp-clobbers"
>      }

Here's the complete output from and amd64-pc-solaris2.12 build with your
patch included:

spawn /var/gcc/regression/trunk/12-gcc-64/build/gcc/xgcc -B/var/gcc/regression/trunk/12-gcc-64/build/gcc/ /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c -fno-diagnostics-show-caret -fdiagnostics-color=never -O2 -DGEN_ARGS=-p1 -t64 -I/var/gcc/regression/trunk/12-gcc-64/build/gcc/testsuite/gcc6/ms-sysv -I/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv -Wall -Wall /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S -lm -o ./ms-sysv.exe^M
In file included from /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c:158:0:^M
/var/gcc/regression/trunk/12-gcc-64/build/gcc/testsuite/gcc6/ms-sysv/ms-sysv-generated.h: In function 'msabi_02_1':^M
/var/gcc/regression/trunk/12-gcc-64/build/gcc/testsuite/gcc6/ms-sysv/ms-sysv-generated.h:42:1: error: bp cannot be used in asm here^M
compiler exited with status 1

I suspect the problem is not explicitly passing -fno-omit-frame-pointer,
though.  gcc/config/i386/sol2.h has

#define USE_IX86_FRAME_POINTER 1
#define USE_X86_64_FRAME_POINTER 1

which does this implicitly...

	Rainer

Created attachment 41532 [details]
proposed fix v4

OK, so here goes version 4!  Hopefully we're getting closer.

(In reply to ro@CeBiTec.Uni-Bielefeld.DE from comment #30)
> Fine.  Imagine your formatting/coding style changes: when they are
> intermixed with functional changes, every reviewer has to check
> thoroughly which part is which.  If you separate them (which is easy for
> you to do) and submit them separately, the formatting stuff will be
> obvious or nearly so, and you save everyone time by having to review
> only the beef of the functional changes, in the end also giving you
> faster review.

Yes.  I had at least split out all of the formatting stuff into the first patch, but I had attached them from git format-patch and selected the patch checkbox in Bugzilla, so maybe it wasn't as obvious as it didn't show the subject or text in the mbox file (it would have probably been better to copy & paste the subject line of each patch).  I've separated out only what's relevant to this bug report: what breaks on Solaris, the ugly hard-coded offsets and replacing the custom parallelization code.  Note that I have included the changes to split the test jobs into smaller pieces so that potential RTL-checking timeout issues are resolved.

> > Please also note that I did seek guidance when putting this exp file together
> > (back in December)  I was following Mike Stump's direction, but you were
> > probably on vacation or something. :) 
> > https://gcc.gnu.org/ml/gcc/2016-12/msg00145.html
> 
> Might be.  However, I've been appointed testsuite maintainer at a time
> when nobody else was around.  Later, Mike stepped up again who's way
> more experienced here than I am.  In the case at hand, I happen to be
> both victim of your patches' fallout and testsuite maintainer ;-)

lol!!  Well I think you're a bit more knowledgeable to some parts of the test harness as his original direction left all of my tests running one for each -j<job>!  Once I figured that out, I tried to pester Mike and the mailing list for a solution, but ended up just hacking out that custom parallelization code.  But it's all good because I'm learning, and that keeps me happy. :)

> > I've also been motivated to expand the tests by a change somebody else made to
> > my original patch that I wasn't confident the original tests would fully check
> > (been worried about it, but it all looks good).  I'll get a cleaned up patch
> > for you soon.
> 
> Excellent, thanks for your patience.  I know the first times through the
> system can be hard and tedious...

HAH! Thanks for YOUR patience! :)  This patch doesn't include those extended tests, but at least I have run them and feel more confident.  I'll submit those changes separately.


> Here's the complete output from and amd64-pc-solaris2.12 build with your
> patch included:
> 
> spawn /var/gcc/regression/trunk/12-gcc-64/build/gcc/xgcc
> -B/var/gcc/regression/trunk/12-gcc-64/build/gcc/
> /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-
> sysv.c -fno-diagnostics-show-caret -fdiagnostics-color=never -O2
> -DGEN_ARGS=-p1 -t64
> -I/var/gcc/regression/trunk/12-gcc-64/build/gcc/testsuite/gcc6/ms-sysv
> -I/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv
> -Wall -Wall
> /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-
> test.S -lm -o ./ms-sysv.exe^M
> In file included from
> /vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-
> sysv.c:158:0:^M
> /var/gcc/regression/trunk/12-gcc-64/build/gcc/testsuite/gcc6/ms-sysv/ms-sysv-
> generated.h: In function 'msabi_02_1':^M
> /var/gcc/regression/trunk/12-gcc-64/build/gcc/testsuite/gcc6/ms-sysv/ms-sysv-
> generated.h:42:1: error: bp cannot be used in asm here^M
> compiler exited with status 1
> 
> I suspect the problem is not explicitly passing -fno-omit-frame-pointer,
> though.  gcc/config/i386/sol2.h has
> 
> #define USE_IX86_FRAME_POINTER 1
> #define USE_X86_64_FRAME_POINTER 1
> 
> which does this implicitly...
> 
> 	Rainer

Awesome!  This is what I've done in this patch

     # Detect when hard frame pointers are enabled (or required) so we know not
     # to generate bp clobbers.
-    if [regexp "^(.+ +| *)-(O0|fno-omit-frame-pointer|p|pg)( +.*)?$" \
-              $cflags match] then {
+    if { [regexp "(^| )-(O0|fno-omit-frame-pointer|p|pg)( |$)" \
+                "$TEST_ALWAYS_FLAGS $cflags" match]
+        || [istarget *-*-solaris*] } then {
        set generator_args "$generator_args --omit-rbp-clobbers"
     }

I went ahead with adding TEST_ALWAYS_FLAGS just in case, but hopefully the istarget will fix this.  I haven't gotten to setup a Solaris VM yet, so I'll have to ask you to please test this again for me and thank you!!

Also, if this works, should I still post it to the mailing list prior to committing it?  The first time I did that, I didn't realize that you were the other testsuite maintainer -- d'oh! :o

2017-06-10  Daniel Santos  <daniel.santos@pobox.com>

	* gcc.target/x86_64/abi/ms-sysv/do-test.S (test_data_save,
	test_data_input, test_data_output, test_data_fn,
	test_data_retaddr): Remove.
	(regs_to_mem, mem_to_regs): Make global.
	(do_test_aligned, do_test_unaligned): Gut and call do_test_body.
	* gcc.target/x86_64/abi/ms-sysv/ms-sysv.c: Add dg-* directives.
	(do_test_body0): New function.
	(do_test_body): New function added via inline assembly.
	* gcc.target/x86_64/abi/ms-sysv/ms-sysv.exp (runtest_ms_sysv): Replace
	custom parallelization code with dg-runtest, et. al.

Thanks!
Daniel

PS: I still need to create a local SVN repo and practice committing with git-svn, because it's been a long time since I've done a push to SVN via git.  If I screwed that up, they might revoke my commit privs!

Created attachment 41533 [details]
41532: proposed fix v5

Slight change, I forgot this little bit:

@@ -66,8 +66,6 @@ if { (![istarget x86_64-*-*] && ![istarget i?86-*-*])
     return
 }

-global GCC_RUNTEST_PARALLELIZE_DIR
-
 proc runtest_ms_sysv { cflags generator_args } {
     global GCC_UNDER_TEST HOSTCXX HOSTCXXFLAGS tmpdir srcdir subdir \
           TEST_ALWAYS_FLAGS runtests

> --- Comment #32 from Daniel Santos <daniel.santos at pobox dot com> ---
> Created attachment 41533 [details]
>   --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=41533&action=edit
> 41532: proposed fix v5

I've given this one a whirl (with Solaris/x86 /bin/as only so far), and
we're getting closer: right now, the only ms-sysv tests failing are

FAIL: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c -mcall-ms2sysv-xlogues -O0 -g3 "-DGEN_ARGS=-p0\ --omit-rbp-clobbers" (test for excess errors)
FAIL: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c -mcall-ms2sysv-xlogues -O0 -g3 "-DGEN_ARGS=-p1\ --omit-rbp-clobbers" (test for excess errors)
FAIL: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c -mcall-ms2sysv-xlogues -O0 -g3 "-DGEN_ARGS=-p5\ --omit-rbp-clobbers" (test for excess errors)
FAIL: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c -mcall-ms2sysv-xlogues -O2 "-DGEN_ARGS=-p0\ --omit-rbp-clobbers" (test for excess errors)
FAIL: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c -mcall-ms2sysv-xlogues -O2 "-DGEN_ARGS=-p1\ --omit-rbp-clobbers" (test for excess errors)
FAIL: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c -mcall-ms2sysv-xlogues -O2 "-DGEN_ARGS=-p5\ --omit-rbp-clobbers" (test for excess errors)

In all cases, I get link errors:

Excess errors:
Undefined                       first referenced
 symbol                             in file
__resms64f_12                       /var/tmp//ccRVJueb.o
__resms64f_13                       /var/tmp//ccRVJueb.o
__resms64f_14                       /var/tmp//ccRVJueb.o
__resms64f_15                       /var/tmp//ccRVJueb.o
__resms64f_16                       /var/tmp//ccRVJueb.o
__resms64f_17                       /var/tmp//ccRVJueb.o
__savms64f_12                       /var/tmp//ccRVJueb.o
__savms64f_13                       /var/tmp//ccRVJueb.o
__savms64f_14                       /var/tmp//ccRVJueb.o
__savms64f_15                       /var/tmp//ccRVJueb.o
__savms64f_16                       /var/tmp//ccRVJueb.o
__savms64f_17                       /var/tmp//ccRVJueb.o
__resms64fx_12                      /var/tmp//ccRVJueb.o
__resms64fx_13                      /var/tmp//ccRVJueb.o
__resms64fx_14                      /var/tmp//ccRVJueb.o
__resms64fx_15                      /var/tmp//ccRVJueb.o
__resms64fx_16                      /var/tmp//ccRVJueb.o
__resms64fx_17                      /var/tmp//ccRVJueb.o
ld: fatal: symbol referencing errors

	Rainer

One more data point: I tried to run the ms-sysv.exp tests on
x86_64-apple-darwin and failed initially:

FAIL: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c  -O2 "-DGEN_ARGS=-p0" (test for excess errors)
Excess errors:
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c:168:Unknown pseudo-op: .global
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c:168:Rest of line ignored. 1st junk character valued 100 (d).
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S:51:Unknown pseudo-op: .global
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S:51:Rest of line ignored. 1st junk character valued 114 (r).
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S:73:Unknown pseudo-op: .global
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S:73:Rest of line ignored. 1st junk character valued 109 (m).
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S:96:Unknown pseudo-op: .global
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S:96:Rest of line ignored. 1st junk character valued 100 (d).
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S:97:Unknown pseudo-op: .cfi_startproc
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S:107:Unknown pseudo-op: .global
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S:107:Rest of line ignored. 1st junk character valued 100 (d).
/vol/gcc/src/hg/trunk/local/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S:116:Unknown pseudo-op: .cfi_endproc

There are two issues here: while gas understands .global, the Darwin as
does not.  However, globl is common to both (as can e.g. be seen in
libffi/src/x86/sysv.S), so using it makes the test more portable.

The .cfi_* pseudo-ops are another matter: the same file has this comment

/* Sadly, OSX cctools-as doesn't understand .cfi directives at all.  */

so if frame info is really necessary, it would have to be hand-coded as
in those files.  It seems that it's not, though: just commenting
.cfi_startproc and .cfi_endproc still lets the tests still PASS on
x86_64-pc-linux-gnu.  Older Solaris/x86 assemblers have the same issue,
btw.

I cannot right now test the patched tests on Darwin, but will have to do
so later tonight at home.

	Rainer


diff --git a/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S b/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S
--- a/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S
+++ b/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S
@@ -34,7 +34,7 @@ see the files COPYING3 and COPYING.RUNTI
 # endif
 
 # define FUNC(fn)		\
-	.global fn;		\
+	.globl fn;		\
 	ELFFN_BEGIN(fn);	\
 fn:
 
@@ -94,7 +94,7 @@ FUNC_END(mem_to_regs)
 
 # NOTE: Not MT safe
 FUNC(do_test_unaligned)
-	.cfi_startproc
+	#.cfi_startproc
 	# The below alignment checks are to verify correctness of the test
 	# its self.
 
@@ -113,7 +113,7 @@ FUNC(do_test_aligned)
 L0:
 	popf
 	jmp	do_test_body
-        .cfi_endproc
+        #.cfi_endproc
 FUNC_END(do_test_aligned)
 FUNC_END(do_test_unaligned)
 
diff --git a/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c b/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c
--- a/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c
+++ b/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c
@@ -156,7 +156,7 @@ static const char *argv0;
 #endif
 
 #define FUNC_BEGIN(fn)			\
-	"	.global " fn "\n"	\
+	"	.globl " fn "\n"	\
 	ELFFN_BEGIN(fn)			\
 	fn ":\n"
 #define FUNC_END(fn) ELFFN_END(fn)

> In all cases, I get link errors:
>
> Excess errors:
> Undefined                       first referenced
>  symbol                             in file
> __resms64f_12                       /var/tmp//ccRVJueb.o
> __resms64f_13                       /var/tmp//ccRVJueb.o
> __resms64f_14                       /var/tmp//ccRVJueb.o
> __resms64f_15                       /var/tmp//ccRVJueb.o
> __resms64f_16                       /var/tmp//ccRVJueb.o
> __resms64f_17                       /var/tmp//ccRVJueb.o
> __savms64f_12                       /var/tmp//ccRVJueb.o
> __savms64f_13                       /var/tmp//ccRVJueb.o
> __savms64f_14                       /var/tmp//ccRVJueb.o
> __savms64f_15                       /var/tmp//ccRVJueb.o
> __savms64f_16                       /var/tmp//ccRVJueb.o
> __savms64f_17                       /var/tmp//ccRVJueb.o
> __resms64fx_12                      /var/tmp//ccRVJueb.o
> __resms64fx_13                      /var/tmp//ccRVJueb.o
> __resms64fx_14                      /var/tmp//ccRVJueb.o
> __resms64fx_15                      /var/tmp//ccRVJueb.o
> __resms64fx_16                      /var/tmp//ccRVJueb.o
> __resms64fx_17                      /var/tmp//ccRVJueb.o
> ld: fatal: symbol referencing errors
I've found what's going on here: those functions are from
libgcc/config/i386/{res, sav}ms64*.S.  However,
t-msabi which controls their build is only included for Linux/x86_64
targets, not for Solaris (or Darwin).

If I do this with /bin/as, I get all sorts of assembler errors:

Assembler:
        "/var/tmp//ccPz3Pub.s", line 1 : Illegal mnemonic
        Near line: ".macro SSE_SAVE off=0"
        "/var/tmp//ccPz3Pub.s", line 1 : Syntax error
        Near line: ".macro SSE_SAVE off=0"
        "/var/tmp//ccPz3Pub.s", line 1 : Syntax error
        Near line: ".macro SSE_SAVE off=0"
        "/var/tmp//ccPz3Pub.s", line 2 : Syntax error
        Near line: " movaps %xmm15,(\off - 0x90)(%rax)"
        "/var/tmp//ccPz3Pub.s", line 3 : Syntax error
        Near line: " movaps %xmm14,(\off - 0x80)(%rax)"
        "/var/tmp//ccPz3Pub.s", line 4 : Syntax error
        Near line: " movaps %xmm13,(\off - 0x70)(%rax)"
        "/var/tmp//ccPz3Pub.s", line 5 : Syntax error
        Near line: " movaps %xmm12,(\off - 0x60)(%rax)"
        "/var/tmp//ccPz3Pub.s", line 6 : Syntax error
        Near line: " movaps %xmm11,(\off - 0x50)(%rax)"
        "/var/tmp//ccPz3Pub.s", line 7 : Syntax error
        "/var/tmp//ccPz3Pub.s", line 8 : Syntax error
        Near line: " movaps %xmm9, (\off - 0x30)(%rax)"
        "/var/tmp//ccPz3Pub.s", line 9 : Syntax error
        Near line: " movaps %xmm8, (\off - 0x20)(%rax)"
        "/var/tmp//ccPz3Pub.s", line 10 : Syntax error
        Near line: " movaps %xmm7, (\off - 0x10)(%rax)"
        "/var/tmp//ccPz3Pub.s", line 11 : Syntax error
        Near line: " movaps %xmm6, \off(%rax)"
        "/var/tmp//ccPz3Pub.s", line 12 : Illegal mnemonic
        Near line: ".endm"
        "/var/tmp//ccPz3Pub.s", line 12 : Syntax error
        Near line: ".endm"
        "/var/tmp//ccPz3Pub.s", line 13 : Illegal mnemonic
        Near line: ".macro SSE_RESTORE off=0"
        "/var/tmp//ccPz3Pub.s", line 13 : Syntax error
        Near line: ".macro SSE_RESTORE off=0"
        "/var/tmp//ccPz3Pub.s", line 13 : Syntax error
        Near line: ".macro SSE_RESTORE off=0"
        "/var/tmp//ccPz3Pub.s", line 14 : Syntax error
        Near line: " movaps (\off - 0x90)(%rsi), %xmm15"
        "/var/tmp//ccPz3Pub.s", line 15 : Syntax error
        Near line: " movaps (\off - 0x80)(%rsi), %xmm14"
        "/var/tmp//ccPz3Pub.s", line 16 : Syntax error
        Near line: " movaps (\off - 0x70)(%rsi), %xmm13"
        "/var/tmp//ccPz3Pub.s", line 17 : Syntax error
        Near line: " movaps (\off - 0x60)(%rsi), %xmm12"
        "/var/tmp//ccPz3Pub.s", line 18 : Syntax error
        Near line: " movaps (\off - 0x50)(%rsi), %xmm11"
        "/var/tmp//ccPz3Pub.s", line 19 : Syntax error
        Near line: " movaps (\off - 0x40)(%rsi), %xmm10"
        "/var/tmp//ccPz3Pub.s", line 20 : Syntax error
        Near line: " movaps (\off - 0x30)(%rsi), %xmm9"
        "/var/tmp//ccPz3Pub.s", line 21 : Syntax error
        Near line: " movaps (\off - 0x20)(%rsi), %xmm8"
        "/var/tmp//ccPz3Pub.s", line 22 : Syntax error
        Near line: " movaps (\off - 0x10)(%rsi), %xmm7"
        "/var/tmp//ccPz3Pub.s", line 23 : Syntax error
        Near line: " movaps \off(%rsi), %xmm6"
        "/var/tmp//ccPz3Pub.s", line 24 : Illegal mnemonic
        Near line: ".endm"
        "/var/tmp//ccPz3Pub.s", line 24 : Syntax error
        Near line: ".endm"
        "/var/tmp//ccPz3Pub.s", line 41 : Illegal mnemonic
        Near line: " SSE_SAVE off=0x60"
Too many errors - Goodbye
make[2]: *** [/vol/gcc/src/hg/trunk/local/libgcc/shared-object.mk:36:
savms64_s.o] Error 1
make[2]: Leaving directory
'/var/gcc/regression/trunk/12-gcc/build/i386-pc-solaris2.12/amd64/libgcc'

The files are obviously full of gas extensions, which breaks both
Solaris and Darwin.

In an i386-pc-solaris2.12 build configured to use gas, however, all
ms-sysv.exp tests now finally PASS :-)

  Rainer

Thank you for all of your work on this.  The .cfi directives shouldn't be *too* critical -- I've barely scratched the surface of learning DWARF and, iirc, the last time I stepped through these stubs in gdb it still wasn't always able to determine the call frame correctly (although I could be thinking of stepping through the assembly code in the test program).  I suppose this can be an issue for somebody debugging Wine code at some future date, but I have no qualms with removing it for now, and possibly redoing it later in more portable way (and that actually provides the debugger with everything it needs).

Also, you *had* mentioned this linking problem in the past and I apologize for loosing track of it.  I have not actually done a thorough study of ABIs used in other *nix operating systems, but my guess would be that all 64-bit platforms that GCC supports use the SystemV ABI except for Windows (Cygwin & MinGW)?  This is a question outside of my expertise, so please let me know if the below solution amenable.

I should also note that while this optimization isn't meant for Windows and would likely almost never appear in code built for windows (unless somebody is trying to link to objects/libs built on for *nix), support on Windows is explicitly disabled due to the SEH unwind emit code not supporting REG_CFA_EXPRESSION, which it requires.  So we don't need the stubs on Windows anyway.

diff --git a/libgcc/config.host b/libgcc/config.host
index 7711abf2704..f0f0d6c0916 100644
--- a/libgcc/config.host
+++ b/libgcc/config.host
@@ -1355,6 +1355,14 @@ esac
 case ${host} in
 i[34567]86-*-* | x86_64-*-*)
        case ${host} in
+       *-*-cygwin* | *-*-mingw*)
+               ;;
+       *)
+               tmake_file="${tmake_file} i386/t-msabi"
+               ;;
+       esac
+
+       case ${host} in
        *-musl*)
                tmake_file="${tmake_file} i386/t-cpuinfo-static"
                ;;
@@ -1365,11 +1373,12 @@ i[34567]86-*-* | x86_64-*-*)
        ;;
 esac

+
 case ${host} in
 i[34567]86-*-linux* | x86_64-*-linux* | \
   i[34567]86-*-kfreebsd*-gnu | x86_64-*-kfreebsd*-gnu | \
   i[34567]86-*-gnu*)
-       tmake_file="${tmake_file} t-tls i386/t-linux i386/t-msabi t-slibgcc-libgcc"
+       tmake_file="${tmake_file} t-tls i386/t-linux t-slibgcc-libgcc"
        if test "$libgcc_cv_cfi" = "yes"; then
                tmake_file="${tmake_file} t-stack i386/t-stack-i386"
        fi


As for the stubs, I don't think there's a real need to stay tied to gas extensions -- truth be told, this was my first actual non-inline, x86 assembly code I have written (last time I did assembly prior was on a Motorola 6502/6510), so I'm sorry to have forced you to become my unwitting tutor!  :)  This is assembly with cpp, so the gas .macro could be replaced with a cpp macro, but is that acceptable considering that it would result in multiple instructions on the same line delimited by semicolons instead of "\n\t"?  So should I just copy & paste the instructions and be done with it?

Thanks,
Daniel

(In reply to Daniel Santos from comment #36)
> tutor!  :)  This is assembly with cpp, so the gas .macro could be replaced
> with a cpp macro, but is that acceptable considering that it would result in
> multiple instructions on the same line delimited by semicolons instead of
> "\n\t"?  So should I just copy & paste the instructions and be done with it?

I forgot to note that even though the gas .macro takes an argument for base offset, in all uses that offset is 0x60.

Created attachment 41543 [details]
proposed fix v5 addendum (only partially tested)

I've only run check on RUNTESTFLAGS="ms-sysv.exp" so far and I have a full regression test running right now, but I *think* this is correct.  I'm presuming that using .hidden is a no-no as well, at least from what I can tell it's elf-specific, but I'm not sure what else to do with it other than #ifdef __ELF__.  (I googled 'hidden elf' and got a lot of interesting fiction...)  So I'm sorry to just ask you to see if it blows up on Solaris & Darwin w/o gas.

I'm also unsure about my changes to libtgcc/config.host as I just don't have a broad understanding of all of the *nix platforms out there.

Feedback greatly appreciated!

Thanks,
Daniel

Created attachment 41544 [details]
proposed fix v5 addendum

remove fix stray carriage return...

Comment on attachment 41544 [details]
proposed fix v5 addendum

OK, my tests have completed successfully.

> --- Comment #36 from Daniel Santos <daniel.santos at pobox dot com> ---
> Thank you for all of your work on this.  The .cfi directives shouldn't be *too*
> critical -- I've barely scratched the surface of learning DWARF and, iirc, the
> last time I stepped through these stubs in gdb it still wasn't always able to
> determine the call frame correctly (although I could be thinking of stepping
> through the assembly code in the test program).  I suppose this can be an issue
> for somebody debugging Wine code at some future date, but I have no qualms with
> removing it for now, and possibly redoing it later in more portable way (and
> that actually provides the debugger with everything it needs).

I'd just leave them in as comments for now as a reminder, similar to
what's done in libffi.

> Also, you *had* mentioned this linking problem in the past and I apologize for
> loosing track of it.  I have not actually done a thorough study of ABIs used in
> other *nix operating systems, but my guess would be that all 64-bit platforms
> that GCC supports use the SystemV ABI except for Windows (Cygwin & MinGW)? 
> This is a question outside of my expertise, so please let me know if the below
> solution amenable.

I guess that's true for most x86_64 Unix systems, but I'm unsure about
Darwin here, as well as non-Unix x86_64 targets.  One difference I
noticed during my Darwin testing last night was a __USER_LABEL_PREFIX__
of _, which your code doesn't yet account for.  I managed to put
something together, based on your original v5 patch, and the result did
compile on Darwin.  However, all ms-sysv.exp execution tests FAIL there;
had no time to investigate yet.  Might indicate a bug in my changes or
an ABI difference.

> I should also note that while this optimization isn't meant for Windows and
> would likely almost never appear in code built for windows (unless somebody is
> trying to link to objects/libs built on for *nix), support on Windows is
> explicitly disabled due to the SEH unwind emit code not supporting
> REG_CFA_EXPRESSION, which it requires.  So we don't need the stubs on Windows
> anyway.
>
> diff --git a/libgcc/config.host b/libgcc/config.host
> index 7711abf2704..f0f0d6c0916 100644
> --- a/libgcc/config.host
> +++ b/libgcc/config.host
> @@ -1355,6 +1355,14 @@ esac
>  case ${host} in
>  i[34567]86-*-* | x86_64-*-*)
>         case ${host} in
> +       *-*-cygwin* | *-*-mingw*)
> +               ;;
> +       *)
> +               tmake_file="${tmake_file} i386/t-msabi"
> +               ;;
> +       esac
> +
> +       case ${host} in
>         *-musl*)
>                 tmake_file="${tmake_file} i386/t-cpuinfo-static"
>                 ;;
> @@ -1365,11 +1373,12 @@ i[34567]86-*-* | x86_64-*-*)
>         ;;
>  esac
>
> +
>  case ${host} in
>  i[34567]86-*-linux* | x86_64-*-linux* | \
>    i[34567]86-*-kfreebsd*-gnu | x86_64-*-kfreebsd*-gnu | \
>    i[34567]86-*-gnu*)
> -       tmake_file="${tmake_file} t-tls i386/t-linux i386/t-msabi
> t-slibgcc-libgcc"
> +       tmake_file="${tmake_file} t-tls i386/t-linux t-slibgcc-libgcc"
>         if test "$libgcc_cv_cfi" = "yes"; then
>                 tmake_file="${tmake_file} t-stack i386/t-stack-i386"
>         fi

Seems the right thing to do.  In my patch, I'd added t-msabi to
Darwin/x86 and Solaris/x86 only, but unless the testsuite is changed to
exercise the code on every x86 target, your change is certainly consistent.

> As for the stubs, I don't think there's a real need to stay tied to gas
> extensions -- truth be told, this was my first actual non-inline, x86 assembly
> code I have written (last time I did assembly prior was on a Motorola
> 6502/6510), so I'm sorry to have forced you to become my unwitting tutor!  :) 
> This is assembly with cpp, so the gas .macro could be replaced with a cpp
> macro, but is that acceptable considering that it would result in multiple
> instructions on the same line delimited by semicolons instead of "\n\t"?  So
> should I just copy & paste the instructions and be done with it?

My patch of last night went for the cpp macro route: as you say, we
already use cpp features and doing so here makes the code easier to read
and less error-prone.

I'm attaching both my testsuite changes to deal with
__USER_LABEL_PREFIX__ (shamelessly stolen from libffi resp. a testcase
that has a similar need), which is enough to allow the code to compile
and link on Darwin, and the libgcc parts.

I haven't yet tried merging those changes with you v5.1 patch, though.

	Rainer

> --- Comment #37 from Daniel Santos <daniel.santos at pobox dot com> ---
> (In reply to Daniel Santos from comment #36)
>> tutor!  :)  This is assembly with cpp, so the gas .macro could be replaced
>> with a cpp macro, but is that acceptable considering that it would result in
>> multiple instructions on the same line delimited by semicolons instead of
>> "\n\t"?  So should I just copy & paste the instructions and be done with it?
>
> I forgot to note that even though the gas .macro takes an argument for base
> offset, in all uses that offset is 0x60.

Right, which makes it even easier to handle with a simple #define.

	Rainer

> --- Comment #38 from Daniel Santos <daniel.santos at pobox dot com> ---
[...]
> I've only run check on RUNTESTFLAGS="ms-sysv.exp" so far and I have a full
> regression test running right now, but I *think* this is correct.  I'm
> presuming that using .hidden is a no-no as well, at least from what I can tell
> it's elf-specific, but I'm not sure what else to do with it other than #ifdef
> __ELF__.  (I googled 'hidden elf' and got a lot of interesting fiction...)  So
> I'm sorry to just ask you to see if it blows up on Solaris & Darwin w/o gas.

.hidden itself is indeed ELF specific (and was initially invented by Sun
IIRC), but libgcc already has provisions to handle similar concepts on
other targets (cf. asm_hidden_op in configure.ac and the .vis files).  I
haven't yet studied how this applies to Darwin (which supports
.private_extern instead).

> I'm also unsure about my changes to libtgcc/config.host as I just don't have a
> broad understanding of all of the *nix platforms out there.

As I said: I believe they are correct in principle, non-Unix targets
being ELF and using gas, so you're probably safe.  I guess the x86
maintainers will tell (or enraged target maintainers when they see
bootstrap failures ;-)

	Rainer

Created attachment 41545 [details]
testsuite changes for Darwin etc. on top of v5 patch

Created attachment 41546 [details]
libgcc changes for Darwin etc. on top of v5 patch

Created attachment 41547 [details]
libgcc changes for Darwin etc. on top of v5 patch

Created attachment 41587 [details]
proposed fix v6 1/2 (testsuite)

I'm sorry for the delay again.  I've been having some health problems infringing upon my hacking time.

I wanted to study the use of __USER_LABEL_PREFIX__ to make sure I understand the implications.  I'm not completely clear on rather or not this is automatically applied when a back end uses gen_rtx_SYMBOL_REF (or some such), but guess is that it is.  It is also plausible to omit Darwin support for now, as I've learned that 64-bit Wine isn't yet working for Darwin either.  If there are further problems, then that might be the smartest way to go since I don't have access to such a machine witch which I can test, experiment, debug, etc.  But if this does the trick, then all the better.

I changed the C() macro to ASMNAME() just because I prefer helpful names and I decided to yank out all of the FUNC_BEGIN/FUNC_END macros from ms-sysv.c and just use #if directives directly in the string definition.  There's no sense in maintaining a separate set of asm support macros dealing in strings when there's only one use site.

I also noticed a possible "gotcha" with the #if __x86_64__ and __SSE2__ -- not that I would expect necessarily expect it to happen.

In hopes of making your review easier, below is a delta between this new (v6) patch set and your last posted patches.





diff --git a/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S b/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S
index 40e119b6cc3..4a4f2e42c61 100644
--- a/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S
+++ b/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S
@@ -23,40 +23,42 @@ a copy of the GCC Runtime Library Exception along with this program;
 see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 <http://www.gnu.org/licenses/>.  */
 
-#ifdef __x86_64__
-
-# ifdef __ELF__
-#  define ELFFN_BEGIN(fn)       .type fn,@function
-#  define ELFFN_END(fn)         .size fn,.-fn
-# else
-#  define ELFFN_BEGIN(fn)
-#  define ELFFN_END(fn)
-# endif
-
-#define C2(X, Y)  X ## Y
-#define C1(X, Y)  C2(X, Y)
+#if defined(__x86_64__) && defined(__SSE2__)
+
+/* These macros currently support GNU/Linux, Solaris and Darwin.  */
+
+#ifdef __ELF__
+# define FN_TYPE(fn) .type fn,@function
+# define FN_SIZE(fn) .size fn,.-fn
+#else
+# define FN_TYPE(fn)
+# define FN_SIZE(fn)
+#endif
+
 #ifdef __USER_LABEL_PREFIX__
-# define C(X)     C1(__USER_LABEL_PREFIX__, X)
+# define ASMNAME2(prefix, name)	prefix ## name
+# define ASMNAME1(prefix, name)	ASMNAME2(prefix, name)
+# define ASMNAME(name)		ASMNAME1(__USER_LABEL_PREFIX__, name)
 #else
-# define C(X)     X
+# define ASMNAME(name)		name
 #endif
 
-# define FUNC(fn)		\
-	.globl C(fn);		\
-	ELFFN_BEGIN(C(fn));	\
-C(fn):
+#define FUNC_BEGIN(fn)		\
+	.globl ASMNAME(fn);	\
+	FN_TYPE (ASMNAME(fn));	\
+ASMNAME(fn):
 
-#define FUNC_END(fn) ELFFN_END(fn)
+#define FUNC_END(fn) FN_SIZE(ASMNAME(fn))
 
-# ifdef __AVX__
-#  define MOVAPS vmovaps
-# else
-#  define MOVAPS movaps
-# endif
+#ifdef __AVX__
+# define MOVAPS vmovaps
+#else
+# define MOVAPS movaps
+#endif
 
 	.text
 
-FUNC(regs_to_mem)
+FUNC_BEGIN(regs_to_mem)
 	MOVAPS	%xmm6, (%rax)
 	MOVAPS	%xmm7, 0x10(%rax)
 	MOVAPS	%xmm8, 0x20(%rax)
@@ -78,7 +80,7 @@ FUNC(regs_to_mem)
 	retq
 FUNC_END(regs_to_mem)
 
-FUNC(mem_to_regs)
+FUNC_BEGIN(mem_to_regs)
 	MOVAPS	(%rax), %xmm6
 	MOVAPS	0x10(%rax),%xmm7
 	MOVAPS	0x20(%rax),%xmm8
@@ -101,8 +103,7 @@ FUNC(mem_to_regs)
 FUNC_END(mem_to_regs)
 
 # NOTE: Not MT safe
-FUNC(do_test_unaligned)
-	#.cfi_startproc
+FUNC_BEGIN(do_test_unaligned)
 	# The below alignment checks are to verify correctness of the test
 	# its self.
 
@@ -112,7 +113,7 @@ FUNC(do_test_unaligned)
 	jne	L0
 	int	$3		# Stack not unaligned
 
-FUNC(do_test_aligned)
+FUNC_BEGIN(do_test_aligned)
 	# Verify that incoming stack is aligned
 	pushf
 	test	$0xf, %rsp
@@ -120,8 +121,7 @@ FUNC(do_test_aligned)
 	int	$3		# Stack not aligned
 L0:
 	popf
-	jmp	C(do_test_body)
-        #.cfi_endproc
+	jmp	ASMNAME(do_test_body)
 FUNC_END(do_test_aligned)
 FUNC_END(do_test_unaligned)
 
diff --git a/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c b/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c
index 2880f6fc9a2..81c9c1ffdac 100644
--- a/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c
+++ b/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c
@@ -62,8 +62,8 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 #include <errno.h>
 #include <ctype.h>
 
-#ifndef __x86_64__
-# error Test only valid on x86_64
+#if !defined(__x86_64__) || !defined(__SSE2__)
+# error Test only valid on x86_64 with -msse2
 #endif
 
 enum reg_data_sets
@@ -147,37 +147,33 @@ static __attribute__((ms_abi)) long
 
 static int arbitrarily_fail;
 static const char *argv0;
-#ifdef __ELF__
-# define ELFFN_BEGIN(fn)	"\t.type " fn ",@function\n"
-# define ELFFN_END(fn)		"\t.size " fn ",.-" fn "\n"
+
+#ifdef __USER_LABEL_PREFIX__
+# define ASMNAME3(name) 	#name
+# define ASMNAME2(prefix, name) ASMNAME3(prefix ## name)
+# define ASMNAME1(prefix, name) ASMNAME2(prefix, name)
+# define ASMNAME(name)		ASMNAME1(__USER_LABEL_PREFIX__, name)
 #else
-# define ELFFN_BEGIN(fn)
-# define ELFFN_END(fn)
+# define ASMNAME(name)		#name
 #endif
 
-#define C(X)  C2 (__USER_LABEL_PREFIX__, X)
-#define C2(X, Y) STRING (X) Y
-#define STRING(X)    #X
-
-#define FUNC_BEGIN(fn)			\
-	"	.globl " C(fn) "\n"	\
-	ELFFN_BEGIN(C(fn))		\
-	C(fn) ":\n"
-#define FUNC_END(fn) ELFFN_END(C(fn))
-
 void __attribute__((used))
 do_test_body0 (void)
 {
   __asm__ ("\n"
-  	FUNC_BEGIN("do_test_body")
+	"	.globl " ASMNAME(do_test_body) "\n"
+#ifdef __ELF__
+	"	.type " ASMNAME(do_test_body) ",@function\n"
+#endif
+	ASMNAME(do_test_body) ":\n"
 	"\n"
 	"	# Save registers.\n"
 	"	lea	%0, %%rax\n"
-	"	call	" C("regs_to_mem") "\n"
+	"	call	" ASMNAME(regs_to_mem) "\n"
 	"\n"
 	"	# Load registers with random data.\n"
 	"	lea	%1, %%rax\n"
-	"	call	" C("mem_to_regs") "\n"
+	"	call	" ASMNAME(mem_to_regs) "\n"
 	"\n"
 	"	# Save original return address.\n"
 	"	pop	%%rax\n"
@@ -194,14 +190,16 @@ do_test_body0 (void)
 	"	# resulting register values.\n"
 	"	push	%%rax\n"
 	"	lea	%2, %%rax\n"
-	"	call	" C("regs_to_mem") "\n"
+	"	call	" ASMNAME(regs_to_mem) "\n"
 	"\n"
 	"	# Restore registers.\n"
 	"	lea	%0, %%rax\n"
-	"	call	" C("mem_to_regs") "\n"
+	"	call	" ASMNAME(mem_to_regs) "\n"
 	"	pop	%%rax\n"
 	"	retq\n"
-	FUNC_END(do_test_body)
+#ifdef __ELF__
+	"	.size " ASMNAME(do_test_body) ",.-" ASMNAME(do_test_body) "\n"
+#endif
 	::
 	"m"(test_data.regdata[REG_SET_SAVE]),
 	"m"(test_data.regdata[REG_SET_INPUT]),
diff --git a/libgcc/config/i386/i386-asm.h b/libgcc/config/i386/i386-asm.h
index 285f75f76ac..1387fd24b4f 100644
--- a/libgcc/config/i386/i386-asm.h
+++ b/libgcc/config/i386/i386-asm.h
@@ -26,39 +26,45 @@ see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
 #ifndef I386_ASM_H
 #define I386_ASM_H
 
+#include "auto-host.h"
+
+/* These macros currently support GNU/Linux, Solaris and Darwin.  */
+
 #ifdef __ELF__
-# define ELFFN(fn) .type fn,@function
+# define FN_TYPE(fn) .type fn,@function
+# define FN_SIZE(fn) .size fn,.-fn
+# ifdef HAVE_GAS_HIDDEN
+#  define FN_HIDDEN(fn) .hidden fn
+# endif
 #else
-# define ELFFN(fn)
+# define FN_TYPE(fn)
+# define FN_SIZE(fn)
+#endif
+
+#ifndef FN_HIDDEN
+# define FN_HIDDEN(fn)
 #endif
 
-#define C2(X, Y)  X ## Y
-#define C1(X, Y)  C2(X, Y)
 #ifdef __USER_LABEL_PREFIX__
-# define C(X)     C1(__USER_LABEL_PREFIX__, X)
+# define ASMNAME2(prefix, name)	prefix ## name
+# define ASMNAME1(prefix, name)	ASMNAME2(prefix, name)
+# define ASMNAME(name)		ASMNAME1(__USER_LABEL_PREFIX__, name)
 #else
-# define C(X)     X
+# define ASMNAME(name)		name
 #endif
 
-#define FUNC_START(fn)\
-	.globl C(fn);	\
-	ELFFN (C(fn));	\
-fn:
+#define FUNC_BEGIN(fn)		\
+	.globl ASMNAME(fn);	\
+	FN_TYPE (ASMNAME(fn));	\
+ASMNAME(fn):
 
-#ifdef __ELF__
-# define HIDDEN_FUNC(fn)\
-	FUNC_START (fn)	\
-	.hidden C(fn);
-#else
-# define HIDDEN_FUNC(fn)\
-	FUNC_START (fn)
-#endif
+#define HIDDEN_FUNC(fn)		\
+	.globl ASMNAME(fn);	\
+	FN_TYPE(ASMNAME(fn));	\
+	FN_HIDDEN(ASMNAME(fn));	\
+ASMNAME(fn):
 
-#ifdef __ELF__
-# define FUNC_END(fn) .size fn,.-fn
-#else
-# define FUNC_END(fn)
-#endif
+#define FUNC_END(fn) FN_SIZE(ASMNAME(fn))
 
 #ifdef __SSE2__
 # ifdef __AVX__

Created attachment 41588 [details]
proposed fix v6 2/2 (libgcc)

The only thing this changes from your patches is some macro names and testing HAVE_GAS_HIDDEN.  From what I can tell ".hidden" is an ELF thing.  (I guess I really need to learn the ins and outs of ELF and DWARF much better.)  There isn't currently a HAVE_GAS_SIZE in gcc/configure.ac, but it looks like .size syntax can vary across assemblers as well, so hopefully just the #ifdef __ELF__ is a sufficient test for that.

> --- Comment #47 from Daniel Santos <daniel.santos at pobox dot com> ---
[...]
> I'm sorry for the delay again.  I've been having some health problems
> infringing upon my hacking time.

No worries at all: don't even think about this stuff before you're well again!

> I wanted to study the use of __USER_LABEL_PREFIX__ to make sure I understand
> the implications.  I'm not completely clear on rather or not this is
> automatically applied when a back end uses gen_rtx_SYMBOL_REF (or some such),
> but guess is that it is.  It is also plausible to omit Darwin support for now,

That's my understanding as well.

> as I've learned that 64-bit Wine isn't yet working for Darwin either.  If there
> are further problems, then that might be the smartest way to go since I don't
> have access to such a machine witch which I can test, experiment, debug, etc. 
> But if this does the trick, then all the better.

We're getting considerably closer with this latest patch.  I'll describe
the current issues below; maybe some of them are issues even beyond Darwin.

> I changed the C() macro to ASMNAME() just because I prefer helpful names and I
> decided to yank out all of the FUNC_BEGIN/FUNC_END macros from ms-sysv.c and
> just use #if directives directly in the string definition.  There's no sense in
> maintaining a separate set of asm support macros dealing in strings when
> there's only one use site.

Fine with me.  The new names are certainly more expressive, while I'd
just ripped the definitions out of libffi to see if I could something
working that way at all.

> I also noticed a possible "gotcha" with the #if __x86_64__ and __SSE2__ -- not
> that I would expect necessarily expect it to happen.

I'm not sure this is even possible...

> In hopes of making your review easier, below is a delta between this new (v6)
> patch set and your last posted patches.

The new patch works fine for me on both x86_64-pc-linux-gnu (as
expected) and i386-pc-solaris2.12.

On x86_64-apple-darwin11.4.2, there are a couple of isues, some of which
I'd already resolved before you posted the revised patch.

* Initially all tests SEGVed like this (e.g. with -p0 and compiled with -O2):

Program received signal SIGSEGV, Segmentation fault.
0x000000010004664d in regs_to_mem ()
1: x/i $pc
=> 0x10004664d <regs_to_mem>:   movaps %xmm6,(%rax)
(gdb) where
#0  0x000000010004664d in regs_to_mem ()
#1  0x00000001000465df in do_test_body ()
#2  0x000000010002f227 in do_tests_0000 ()
#3  0x00000001000468e3 in main ()

  Here, %rax is 0x0.

  This happens because some setup happens between do_test_body0 and
  do_test_body, and do_test_aligned jumps directly to do_test_body:

        .globl _do_test_body0
        .no_dead_strip _do_test_body0
_do_test_body0:
        movq    _test_data@GOTPCREL(%rip), %rax

        .globl _do_test_body
_do_test_body:

        # Save registers.
        lea     (%rax), %rax
        call    _regs_to_mem

  By that jump, you bypass the setup of %rax and make the test FAIL.  I
  managed to avoid this by changing the jmp to do_test_body0 instead.
  This gets me past this failure, and works on Linux/x86_64, too.
  However, this makes the tests FAIL on Solaris/x86, supposedly due to
  the -fomit-frame-pointer/-fno-omit-frame-pointer difference (though I
  haven't looked more closely).

* With the do_test_body0 jump, I hit the next issue on Darwin with -O0:
  the test SEGVs here:

Program received signal SIGSEGV, Segmentation fault.
0x0000000100031c4e in do_test_body0 ()
    at /vol/gcc/src/hg/trunk/solaris/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c:163
163       __asm__ ("\n"
1: x/i $pc
=> 0x100031c4e <do_test_body0+21>:      mov    %rax,0x2a8(%rdi)
(gdb) where
(gdb) p/x $rdi
$1 = 0x5dc3340b214ef45c

  which is no wonder since %rdi got clobbered just before.  Here's the
  assembler output:

        .globl _do_test_body0
        .no_dead_strip _do_test_body0
_do_test_body0:
        pushq   %rbp
        movq    %rsp, %rbp
        movq    _test_data@GOTPCREL(%rip), %rax
        movq    _test_data@GOTPCREL(%rip), %rdx
        movq    _test_data@GOTPCREL(%rip), %rcx
        movq    _test_data@GOTPCREL(%rip), %rsi
        movq    _test_data@GOTPCREL(%rip), %rdi

        .globl _do_test_body
_do_test_body:

        # Save registers.
        lea     (%rax), %rax
        call    _regs_to_mem

        # Load registers with random data.
        lea     224(%rdx), %rax
        call    _mem_to_regs

        # Save original return address.
        pop     %rax
        movq    %rax, 680(%rdi)

        # Call the test function
        call    *672(%rsi)

        # Restore the original return address.
        movq    680(%rdi), %rcx
        push    %rcx

  By clobbering %rdi (and %rsi), you cause the SEGV above.  To work
  around this, I've wrapped the save/restore of those regs in !__APPLE__
  which gets me a bit further.

* Now I hit another SEGV here:

Program received signal SIGSEGV, Segmentation fault.
0x0000000100001922 in msabi_00_0 ()
    at /var/gcc/regression/trunk/10.7-gcc/build/gcc/testsuite/gcc/ms-sysv/ms-sysv-generated.h:16
16      {
1: x/i $pc
=> 0x100001922 <msabi_00_0+13>: movaps %xmm6,(%rsp)
(gdb) where
#0  0x0000000100001922 in msabi_00_0 ()
    at /var/gcc/regression/trunk/10.7-gcc/build/gcc/testsuite/gcc/ms-sysv/ms-sysv-generated.h:16
#1  0x0000000100031c77 in do_test_body0 ()
    at /vol/gcc/src/hg/trunk/solaris/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c:163
Cannot access memory at address 0x6fdc16ba5f5f46bc

  At this point I gave up for now...

Here's the snippet I've been using to get that far:

diff --git a/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S b/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S
--- a/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S
+++ b/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S
@@ -69,8 +69,10 @@ FUNC_BEGIN(regs_to_mem)
 	MOVAPS	%xmm13, 0x70(%rax)
 	MOVAPS	%xmm14, 0x80(%rax)
 	MOVAPS	%xmm15, 0x90(%rax)
+#ifndef __APPLE__
 	mov	%rsi, 0xa0(%rax)
 	mov	%rdi, 0xa8(%rax)
+#endif
 	mov	%rbx, 0xb0(%rax)
 	mov	%rbp, 0xb8(%rax)
 	mov	%r12, 0xc0(%rax)
@@ -91,8 +93,10 @@ FUNC_BEGIN(mem_to_regs)
 	MOVAPS	0x70(%rax),%xmm13
 	MOVAPS	0x80(%rax),%xmm14
 	MOVAPS	0x90(%rax),%xmm15
+#ifndef __APPLE__
 	mov	0xa0(%rax),%rsi
 	mov	0xa8(%rax),%rdi
+#endif
 	mov	0xb0(%rax),%rbx
 	mov	0xb8(%rax),%rbp
 	mov	0xc0(%rax),%r12
@@ -121,7 +125,11 @@ FUNC_BEGIN(do_test_aligned)
 	int	$3		# Stack not aligned
 L0:
 	popf
+#ifdef __APPLE__
+	jmp	ASMNAME(do_test_body0)
+#else
 	jmp	ASMNAME(do_test_body)
+#endif
 FUNC_END(do_test_aligned)
 FUNC_END(do_test_unaligned)
 
	Rainer

Created attachment 41605 [details]
darwin fixup (on top of v6)

(In reply to ro@CeBiTec.Uni-Bielefeld.DE from comment #49)
> 
> No worries at all: don't even think about this stuff before you're well
> again!

Thank you, but this is chronic and comes and goes.  The trend line seems to be heading upward for the moment, however.

>[...]
> > In hopes of making your review easier, below is a delta between this new (v6)
> > patch set and your last posted patches.
> 
> The new patch works fine for me on both x86_64-pc-linux-gnu (as
> expected) and i386-pc-solaris2.12.
> 
> On x86_64-apple-darwin11.4.2, there are a couple of isues, some of which
> I'd already resolved before you posted the revised patch.
> 
> * Initially all tests SEGVed like this (e.g. with -p0 and compiled with -O2):
> 
> Program received signal SIGSEGV, Segmentation fault.
> 0x000000010004664d in regs_to_mem ()
> 1: x/i $pc
> => 0x10004664d <regs_to_mem>:   movaps %xmm6,(%rax)
> (gdb) where
> #0  0x000000010004664d in regs_to_mem ()
> #1  0x00000001000465df in do_test_body ()
> #2  0x000000010002f227 in do_tests_0000 ()
> #3  0x00000001000468e3 in main ()
> 
>   Here, %rax is 0x0.
> 
>   This happens because some setup happens between do_test_body0 and
>   do_test_body, and do_test_aligned jumps directly to do_test_body:
> 
>         .globl _do_test_body0
>         .no_dead_strip _do_test_body0
> _do_test_body0:
>         movq    _test_data@GOTPCREL(%rip), %rax
> 
>         .globl _do_test_body
> _do_test_body:
> 
>         # Save registers.
>         lea     (%rax), %rax
>         call    _regs_to_mem
> 
>   By that jump, you bypass the setup of %rax and make the test FAIL.  I
>   managed to avoid this by changing the jmp to do_test_body0 instead.
>   This gets me past this failure, and works on Linux/x86_64, too.
>   However, this makes the tests FAIL on Solaris/x86, supposedly due to
>   the -fomit-frame-pointer/-fno-omit-frame-pointer difference (though I
>   haven't looked more closely).

Thanks again for your help on this.  All of this asm is a big ABI hack and presumes I'm working with 64-bit SystemV ABI, but apple's ABI appears to differ somewhat (I've may have found a good description of that here: https://developer.apple.com/library/content/documentation/DeveloperTools/Conceptual/MachOTopics/1-Articles/x86_64_code.html).

My hope in declaring my own global symbol in the assembly and then explicitly RET-ing was to bypass any ABI-specific setup and tear-down, most notably the hard frame pointer.  In the case of Darwin, this doesn't appear to work since the asm template instantiation of my "global + offset" seems to work quite differently and wants to store the base address in a register that is already being used for something else (actually, every register except XMM0-5 and XMM16+ are volatile at this point).  I had expected it to generate something akin to:

        lea test_data + 224(%rip), %rax

It would be nice if the "naked" function attribute were available for the i386 back-end, then I wouldn't have to screw around with trying to hack-away the ABI. (maybe a worthwhile future venture)

The attached patch (on top of v6) *might* solve the problem on Darwin, but I don't understand exactly how GOTPCREL works, other than it's using a global offset table for linking.  Hopefully, the linker can translate this directly into a constant rip-rel offset.  What I'm doing here is that instead of feeding addresses to the asm template, I'm giving in the offsets and schlepping together an address operand from that, e.g.:

        lea %p0 + test_data@GOTPCREL(%%rip), %%rax

Now if this fix *does* work, then I might need to investigate if this is a performance problem for Darwin -- why use an extra instruction to copy the address to a register before modifying it?  If it doesn't work then it's probably because it really *needs* two instructions.  I'm curious what the disassembly of the linked program looks like.

> 
> * With the do_test_body0 jump, I hit the next issue on Darwin with -O0:
>   the test SEGVs here:
> 
> Program received signal SIGSEGV, Segmentation fault.
> 0x0000000100031c4e in do_test_body0 ()
>     at
> /vol/gcc/src/hg/trunk/solaris/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-
> sysv.c:163
> 163       __asm__ ("\n"
> 1: x/i $pc
> => 0x100031c4e <do_test_body0+21>:      mov    %rax,0x2a8(%rdi)
> (gdb) where
> (gdb) p/x $rdi
> $1 = 0x5dc3340b214ef45c

Yeah, this won't work because the reality is that all GP registers are volatile at this point, but gcc will generate code that clobbers them based upon the alleged ABI of the function -- which is a lie. :)  If the attached patch doesn't work, then I think it's best to just move the assembly back into do-test.S and hard-code the offsets in a shared header file, so we can use them as a macro from do-test.S and also check them with asserts in ms-sysv.c.

I suppose another option would be to just declare everything as clobbered in the asm clobber list, but that somehow just makes me feel dirty. :)

Thanks,
Daniel

(In reply to Daniel Santos from comment #50)
> 
> It would be nice if the "naked" function attribute were available for the
> i386 back-end, then I wouldn't have to screw around with trying to hack-away
> the ABI. (maybe a worthwhile future venture)
> 

This is bug 25967

> The attached patch (on top of v6) *might* solve the problem on Darwin, but I
> don't understand exactly how GOTPCREL works, other than it's using a global
> offset table for linking.  Hopefully, the linker can translate this directly
> into a constant rip-rel offset.  What I'm doing here is that instead of feeding
> addresses to the asm template, I'm giving in the offsets and schlepping
> together an address operand from that, e.g.:
>
>         lea %p0 + test_data@GOTPCREL(%%rip), %%rax
>
> Now if this fix *does* work, then I might need to investigate if this is a
> performance problem for Darwin -- why use an extra instruction to copy the
> address to a register before modifying it?  If it doesn't work then it's
> probably because it really *needs* two instructions.  I'm curious what the
> disassembly of the linked program looks like.

Unfortunately, the patch doesn't work, apart from the

+# define PCREL "@GETPCREL"

-> @GOTPCREL typo ;-)

At -O0 -g3, it SEGVs at

Program received signal SIGSEGV, Segmentation fault.
0x0000000000000000 in ?? ()
1: x/i $pc
=> 0x0: <error: Cannot access memory at address 0x0>
(gdb) where
#0  0x0000000000000000 in ?? ()
#1  0x0000000100031c58 in do_test_body0 ()
    at /vol/gcc/src/hg/trunk/solaris/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c:178
Backtrace stopped: previous frame inner to this frame (corrupt stack?)

where %rip is 0x0.  This happens because most of the addresses are off
by 0x680 bytes.  Here's the disassembly:

(gdb) x/12i 0x0000000100031c58-42
   0x100031c2e <do_test_body0>: push   %rbp
   0x100031c2f <do_test_body0+1>:       mov    %rsp,%rbp
   0x100031c32 <do_test_body0>: lea    0x1b407(%rip),%rax        # 0x10004d040
   0x100031c39 <do_test_body0+7>:       callq  0x10003247c <regs_to_mem>
   0x100031c3e <do_test_body0+12>:
    lea    0x1b4db(%rip),%rax        # 0x10004d120 <do_tests_0004_noinfo>
   0x100031c45 <do_test_body0+19>:      callq  0x1000324ea <mem_to_regs>
   0x100031c4a <do_test_body0+24>:      pop    %rax
   0x100031c4b <do_test_body0+25>:
    mov    %rax,0x1b696(%rip)        # 0x10004d2e8 <buffer.5456+104>
   0x100031c52 <do_test_body0+32>:
    callq  *0x1b688(%rip)        # 0x10004d2e0 <buffer.5456+96>
   0x100031c58 <do_test_body0+38>:
    mov    0x1bd09(%rip),%rcx        # 0x10004d968 <test_data+680>

Here are the addresses that are supposed to be used:

%p0

(gdb) p/x &test_data.regdata[0]
$11 = 0x10004d6c0

%p1

(gdb) p/x &test_data.regdata[1]
$12 = 0x10004d7a0

%p4

(gdb) p/x &test_data.retaddr
$13 = 0x10004d968

%p3

(gdb) p/x &test_data.fn
$14 = 0x10004d960

Only the second use of %p4 is right.

	Rainer

(In reply to ro@CeBiTec.Uni-Bielefeld.DE from comment #52)
> Unfortunately, the patch doesn't work, apart from the
> 
> +# define PCREL "@GETPCREL"
> 
> -> @GOTPCREL typo ;-)

Ah hah! That would explain why I couldn't use that addressing on gnu/linux, I was looking for the Global Effset Table! :)

> At -O0 -g3, it SEGVs at
> 
> Program received signal SIGSEGV, Segmentation fault.
> 0x0000000000000000 in ?? ()
> 1: x/i $pc
> => 0x0: <error: Cannot access memory at address 0x0>
> (gdb) where
> #0  0x0000000000000000 in ?? ()
> #1  0x0000000100031c58 in do_test_body0 ()
>     at
> /vol/gcc/src/hg/trunk/solaris/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-
> sysv.c:178
> Backtrace stopped: previous frame inner to this frame (corrupt stack?)
> 
> where %rip is 0x0.  This happens because most of the addresses are off
> by 0x680 bytes.  Here's the disassembly:
> 
> (gdb) x/12i 0x0000000100031c58-42
>    0x100031c2e <do_test_body0>: push   %rbp
>    0x100031c2f <do_test_body0+1>:       mov    %rsp,%rbp
>    0x100031c32 <do_test_body0>: lea    0x1b407(%rip),%rax        #
> 0x10004d040
>    0x100031c39 <do_test_body0+7>:       callq  0x10003247c <regs_to_mem>
>    0x100031c3e <do_test_body0+12>:
>     lea    0x1b4db(%rip),%rax        # 0x10004d120 <do_tests_0004_noinfo>
>    0x100031c45 <do_test_body0+19>:      callq  0x1000324ea <mem_to_regs>
>    0x100031c4a <do_test_body0+24>:      pop    %rax
>    0x100031c4b <do_test_body0+25>:
>     mov    %rax,0x1b696(%rip)        # 0x10004d2e8 <buffer.5456+104>
>    0x100031c52 <do_test_body0+32>:
>     callq  *0x1b688(%rip)        # 0x10004d2e0 <buffer.5456+96>
>    0x100031c58 <do_test_body0+38>:
>     mov    0x1bd09(%rip),%rcx        # 0x10004d968 <test_data+680>
> 
> Here are the addresses that are supposed to be used:
> 
> %p0
> 
> (gdb) p/x &test_data.regdata[0]
> $11 = 0x10004d6c0
> 
> %p1
> 
> (gdb) p/x &test_data.regdata[1]
> $12 = 0x10004d7a0
> 
> %p4
> 
> (gdb) p/x &test_data.retaddr
> $13 = 0x10004d968
> 
> %p3
> 
> (gdb) p/x &test_data.fn
> $14 = 0x10004d960
> 
> Only the second use of %p4 is right.
> 
> 	Rainer

Great! When I correct the GOTPCREL typo, I can build this on gnu/linux and I get a variation of the same problem.  So apparently GOTPCREL allows you to specify the address of the object, but not an address plus offset -- which is why gcc emits that on Darwin in the first place.  All is becoming clear.

Also, I lied about needing all registers in do_test_(un)aligned; I forgot that this is called as an ms_abi function.  I can clobber rax, r10 and r11 prior to calling the test function and rcx, rdx, and r8-11 after the test function has returned.  So I have plenty of registers to accommodate this.

Created attachment 41627 [details]
darwin fixup (on top of v6) -- second attempt

So I've learned that some_symbol@GOTPCREL(%%rip) resolves to the the address of the GOT *entry* for that symbol, which has to be dereferenced to get the address of the object its self.  I was able to test this on my machine by changing #ifdef __MACH__ to #ifndef and this patch is working using the GOT.

I've re-written do_test_body and added a macro LOAD_TEST_DATA_ADDR(dest) in hopes to make both the sources fairly readable and the resulting assembly also readable.  To simplify the routine, I changed mem_to_regs/regs_to_mem to use r10 instead of rax so that I don't have to save and restore it.

Of course this is sub-optimal code, but the execution of the test program is by no means the bottleneck -- I'm trying to keep it as simple and maintainable as possible!

The macro has only two uses, so if you prefer, I can remove it and just replace it with inline #if blocks, e.g.,

#ifdef __MACH__
	"	mov	" ASMNAME(test_data) "@GOTPCREL(%%rip), %%rax\n"
#else
	"	lea	" ASMNAME(test_data) "(%%rip), %%rax\n"
#endif

Thanks!
Daniel

> --- Comment #54 from Daniel Santos <daniel.santos at pobox dot com> ---
> Created attachment 41627 [details]
>   --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=41627&action=edit
> darwin fixup (on top of v6) -- second attempt
[...]
> The macro has only two uses, so if you prefer, I can remove it and just replace
> it with inline #if blocks, e.g.,
>
> #ifdef __MACH__
>         "       mov     " ASMNAME(test_data) "@GOTPCREL(%%rip), %%rax\n"
> #else
>         "       lea     " ASMNAME(test_data) "(%%rip), %%rax\n"
> #endif

I'm fine either way, with a slight preference for the macro version (the
less code duplication, the better ;-)

I've tested this patch last night on both x86_64-apple-darwin11.4.2 and
i386-pc-solaris2.12 and it worked just fine on both!

Thanks a lot.

	Rainer

(In reply to ro@CeBiTec.Uni-Bielefeld.DE from comment #55)
> > --- Comment #54 from Daniel Santos <daniel.santos at pobox dot com> ---
> > Created attachment 41627 [details]
> >   --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=41627&action=edit
> > darwin fixup (on top of v6) -- second attempt
> [...]
> > The macro has only two uses, so if you prefer, I can remove it and just replace
> > it with inline #if blocks, e.g.,
> >
> > #ifdef __MACH__
> >         "       mov     " ASMNAME(test_data) "@GOTPCREL(%%rip), %%rax\n"
> > #else
> >         "       lea     " ASMNAME(test_data) "(%%rip), %%rax\n"
> > #endif
> 
> I'm fine either way, with a slight preference for the macro version (the
> less code duplication, the better ;-)

That's fine with me. :)  

 
> I've tested this patch last night on both x86_64-apple-darwin11.4.2 and
> i386-pc-solaris2.12 and it worked just fine on both!
> 
> Thanks a lot.
> 
> 	Rainer

Wonderful! I presume that we still need libgcc buy-off?  I'll put together a ChangeLog and post it to gcc-patches tomorrow.

Thanks!
Daniel

> --- Comment #56 from Daniel Santos <daniel.santos at pobox dot com> ---
[...]
> Wonderful! I presume that we still need libgcc buy-off?  I'll put together a
> ChangeLog and post it to gcc-patches tomorrow.

Right.  Best copy both Uros and Iain Sandoe for x86 and Darwin review in
case they have ideas or suggestions.

Thanks again.

	Rainer

Author: dansan
Date: Mon Jul 24 21:59:57 2017
New Revision: 250488

URL: https://gcc.gnu.org/viewcvs?rev=250488&root=gcc&view=rev
Log:
PR testsuite/80759 Fix -mcall-ms2sysv-xlogues on Darwin and Solaris

2017-07-24  Daniel Santos  <daniel.santos@pobox.com>

	PR testsuite/80759
	* config.host: include i386/t-msabi for darwin and solaris.
	* config/i386/i386-asm.h
	(ELFFN): Rename to FN_TYPE.
	(FN_SIZE): New macro.
	(FN_HIDDEN): Likewise.
	(ASMNAME): Likewise.
	(FUNC_START): Rename to FUNC_BEGIN, use ASMNAME, replace .global with
	.globl.
	(HIDDEN_FUNC): Use ASMNAME and .globl instead of .global.
	(SSE_SAVE): Convert to cpp macro, hard-code offset (always 0x60).
	* config/i386/resms64.S: Use SSE_SAVE as cpp macro instead of gas
	.macro.
	* config/i386/resms64f.S: Likewise.
	* config/i386/resms64fx.S: Likewise.
	* config/i386/resms64x.S: Likewise.
	* config/i386/savms64.S: Likewise.
	* config/i386/savms64f.S: Likewise.

Modified:
    trunk/libgcc/ChangeLog
    trunk/libgcc/config.host
    trunk/libgcc/config/i386/i386-asm.h
    trunk/libgcc/config/i386/resms64.S
    trunk/libgcc/config/i386/resms64f.S
    trunk/libgcc/config/i386/resms64fx.S
    trunk/libgcc/config/i386/resms64x.S
    trunk/libgcc/config/i386/savms64.S
    trunk/libgcc/config/i386/savms64f.S

Author: dansan
Date: Mon Jul 24 22:00:35 2017
New Revision: 250489

URL: https://gcc.gnu.org/viewcvs?rev=250489&root=gcc&view=rev
Log:
PR testsuite/80759 Fix broken tests in ms-sysv.exp

2017-07-24  Daniel Santos  <daniel.santos@pobox.com>

	PR testsuite/80759
	* gcc.target/x86_64/abi/ms-sysv/do-test.S
	(ELFFN_BEGIN): Rename to FN_TYPE.
	(ELFFN_END): Rename to FN_SIZE.
	(ASMNAME): New macro.
	(FUNC): Rename to FUNC_BEGIN, use ASMNAME and use .globl instead of
	.global.
	(FUNC_END): Use ASMNAME.
	(test_data_save): Remove.
	(test_data_input): Likewise.
	(test_data_output: Likewise.
	(test_data_fn): Likewise.
	(test_data_retaddr): Likewise.
	(regs_to_mem): Make globals, use r10 instead of rax.
	(mem_to_regs): Likewise.
	(do_test_unaligned): Remove .cfi directives, remove pushf/popf, move
	body to ms-sysv.c.
	(do_test_aligned): Likewise.
	* gcc.target/x86_64/abi/ms-sysv/ms-sysv.c:
	Add dg-* directives.
	(PASTE_STR): New macro.
	(ASMNAME): Likewise.
	(LOAD_TEST_DATA_ADDR): Likewise.
	(TEST_DATA_OFFSET): Likewise.
	(do_test_body0): New C function.
	(do_test_body): New inline assembly routine.
	* gcc.target/x86_64/abi/ms-sysv/ms-sysv.exp
	(runtest_ms_sysv): Modify.

Modified:
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S
    trunk/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c
    trunk/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.exp

At revision r250610 I still see

WARNING: Could not generate /opt/gcc/build_w/gcc/testsuite/gcc/ms-sysv/ms-sysv-generated.h
WARNING: Could not generate /opt/gcc/build_w/gcc/testsuite/gcc/ms-sysv/ms-sysv-generated.h
WARNING: Could not generate /opt/gcc/build_w/gcc/testsuite/gcc/ms-sysv/ms-sysv-generated.h
WARNING: Could not generate /opt/gcc/build_w/gcc/testsuite/gcc/ms-sysv/ms-sysv-generated.h
WARNING: Could not generate /opt/gcc/build_w/gcc/testsuite/gcc5/ms-sysv/ms-sysv-generated.h
WARNING: Could not generate /opt/gcc/build_w/gcc/testsuite/gcc5/ms-sysv/ms-sysv-generated.h
WARNING: Could not generate /opt/gcc/build_w/gcc/testsuite/gcc5/ms-sysv/ms-sysv-generated.h
WARNING: Could not generate /opt/gcc/build_w/gcc/testsuite/gcc5/ms-sysv/ms-sysv-generated.h
FAIL: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c  -O0 -g3 "-DGEN_ARGS=-p0\\ --omit-rbp-clobbers" (test for excess errors)
UNRESOLVED: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c  -O0 -g3 "-DGEN_ARGS=-p0\\ --omit-rbp-clobbers" compilation failed to produce executable
FAIL: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c  -O2 "-DGEN_ARGS=-p0" (test for excess errors)
UNRESOLVED: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c  -O2 "-DGEN_ARGS=-p0" compilation failed to produce executable
FAIL: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c  -O2 "-DGEN_ARGS=-p1" (test for excess errors)
UNRESOLVED: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c  -O2 "-DGEN_ARGS=-p1" compilation failed to produce executable
FAIL: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c  -O2 "-DGEN_ARGS=-p5" (test for excess errors)
UNRESOLVED: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c  -O2 "-DGEN_ARGS=-p5" compilation failed to produce executable
FAIL: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c -mcall-ms2sysv-xlogues -O0 -g3 "-DGEN_ARGS=-p0\\ --omit-rbp-clobbers" (test for excess errors)
UNRESOLVED: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c -mcall-ms2sysv-xlogues -O0 -g3 "-DGEN_ARGS=-p0\\ --omit-rbp-clobbers" compilation failed to produce executable
FAIL: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c -mcall-ms2sysv-xlogues -O2 "-DGEN_ARGS=-p0" (test for excess errors)
UNRESOLVED: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c -mcall-ms2sysv-xlogues -O2 "-DGEN_ARGS=-p0" compilation failed to produce executable
FAIL: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c -mcall-ms2sysv-xlogues -O2 "-DGEN_ARGS=-p1" (test for excess errors)
UNRESOLVED: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c -mcall-ms2sysv-xlogues -O2 "-DGEN_ARGS=-p1" compilation failed to produce executable
FAIL: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c -mcall-ms2sysv-xlogues -O2 "-DGEN_ARGS=-p5" (test for excess errors)
UNRESOLVED: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c -mcall-ms2sysv-xlogues -O2 "-DGEN_ARGS=-p5" compilation failed to produce executable

with -m64 (see https://gcc.gnu.org/ml/gcc-testresults/2017-07/msg02582.html).

Looking at the log file, I see

spawn -ignore SIGHUP /opt/gcc/build_w/gcc/xgcc -B/opt/gcc/build_w/gcc/ /opt/gcc/work/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c -m64 -fno-diagnostics-show-caret -fdiagnostics-color=never -O2 -DGEN_ARGS=-p1 -I/opt/gcc/build_w/gcc/testsuite/gcc1/ms-sysv -I/opt/gcc/work/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv -Wall -Wall /opt/gcc/work/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/do-test.S -lm -o ./ms-sysv.exe^M
/opt/gcc/work/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c:136:37: warning: 'do_tests' used but never defined^M
/opt/gcc/work/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c:345:39: warning: 'check_results' defined but not used [-Wunused-function]^M
/opt/gcc/work/gcc/testsuite/gcc.target/x86_64/abi/ms-sysv/ms-sysv.c:227:1: warning: 'init_test' defined but not used [-Wunused-function]^M
Undefined symbols for architecture x86_64:^M
  "_do_tests", referenced from:^M
      _main in cciP9N43.o^M
ld: symbol(s) not found for architecture x86_64^M
collect2: error: ld returned 1 exit status^M
compiler exited with status 1
FAIL: gcc.target/x86_64/abi/ms-sysv/ms-sysv.c  -O2 "-DGEN_ARGS=-p1" (test for excess errors)

but the test compiles if I compile it manually.

(In reply to Dominique d'Humieres from comment #60)
> At revision r250610 I still see
> 
> WARNING: Could not generate
> /opt/gcc/build_w/gcc/testsuite/gcc/ms-sysv/ms-sysv-generated.h

Thank you for the report.  Perhaps I should have had the test error here instead of issue a warning.  Can you please post the part of the log file where the generator fails?

The various building ms-sysv.c are expected if the generator failed.  The contents of ms-sysv-generated.h are part of each test run.  The -DGEN_ARGS= is actually ignored by the program -- it's just there to make each test description unique while reflecting the arguments passed to the generator (to create the ms-sysv-generated.h used to build that test).

Thanks!
Daniel

Created attachment 41937 [details]
Log file for ms-sysv.exp

Log file generated with

make -k check-gcc RUNTESTFLAGS="ms-sysv.exp"

Created attachment 41943 [details]
test patch for uncaught exception in generator

(In reply to Dominique d'Humieres from comment #62)
> Created attachment 41937 [details]
> Log file for ms-sysv.exp
> 
> Log file generated with
> 
> make -k check-gcc RUNTESTFLAGS="ms-sysv.exp"

Thanks Dominique.  I'm still not seeing all of what I want to see because when you ran this test run the code generator gcc/testsuite/gcc/ms-sysv-generate.exe already existed and so was not rebuilt.  I'm curious to know the full command line when it was built.  Could you apply this patch and run it again?  I should probably make sure that we rebuild the generator on each new run anyway.

As far as the generator, it's not catching an exception that it should.  I had been catching it by value because I always knew the type that was being thrown, but maybe there is some type of weirdness in your particular stdlibc++ implementation, so this patch also changes the catch to by-reference.

Thanks,
Daniel

Created attachment 41963 [details]
Log file with the patch in comment 63

GCC 8.1 has been released.

GCC 8.2 has been released.

GCC 8.3 has been released.

GCC 8.4.0 has been released, adjusting target milestone.