Message ID | alpine.DEB.2.20.2309222351090.5892@tpp.orcam.me.uk |
---|---|
State | New |
Headers | show |
Series | RISC-V/testsuite: Fix ILP32 RVV failures from missing <gnu/stubs-ilp32d.h> | expand |
On 9/22/23 17:18, Maciej W. Rozycki wrote: > In non-multilib installations system headers may not be available for > compilation options using a non-default model, causing build errors such > as: > > In file included from .../include/features.h:527, > from .../include/assert.h:35, > from .../gcc/testsuite/gcc.target/riscv/rvv/autovec/vmv-imm-template.h:2, > from .../gcc/testsuite/gcc.target/riscv/rvv/autovec/vmv-imm-fixed-rv32.c:4: > .../include/gnu/stubs.h:11:11: fatal error: gnu/stubs-ilp32d.h: No such file or directory > > Therefore we have to be very cautious when trying to use a non-default > model in the testsuite, preferably avoiding to rely on headers that have > not been supplied by GCC itself, or otherwise verifying in a preparatory > step whether the given model is buildable in a given test environment. > > In this case however we can easily avoid the issue, because <assert.h> > facilities are not used at all by "vmv-imm-template.h", which includes > the header. Remove the inclusion then, turning these issues: > > FAIL: gcc.target/riscv/rvv/autovec/vmv-imm-fixed-rv32.c -O3 -ftree-vectorize (test for excess errors) > UNRESOLVED: gcc.target/riscv/rvv/autovec/vmv-imm-fixed-rv32.c -O3 -ftree-vectorize scan-assembler-times vmv.v.i 32 > UNRESOLVED: gcc.target/riscv/rvv/autovec/vmv-imm-fixed-rv32.c -O3 -ftree-vectorize scan-assembler-times vmv.v.x 8 > FAIL: gcc.target/riscv/rvv/autovec/vmv-imm-rv32.c -O3 -ftree-vectorize (test for excess errors) > UNRESOLVED: gcc.target/riscv/rvv/autovec/vmv-imm-rv32.c -O3 -ftree-vectorize scan-assembler-times vmv.v.i 32 > UNRESOLVED: gcc.target/riscv/rvv/autovec/vmv-imm-rv32.c -O3 -ftree-vectorize scan-assembler-times vmv.v.x 8 > > into successful results: > > PASS: gcc.target/riscv/rvv/autovec/vmv-imm-fixed-rv32.c -O3 -ftree-vectorize (test for excess errors) > PASS: gcc.target/riscv/rvv/autovec/vmv-imm-fixed-rv32.c -O3 -ftree-vectorize scan-assembler-times vmv.v.i 32 > PASS: gcc.target/riscv/rvv/autovec/vmv-imm-fixed-rv32.c -O3 -ftree-vectorize scan-assembler-times vmv.v.x 8 > PASS: gcc.target/riscv/rvv/autovec/vmv-imm-rv32.c -O3 -ftree-vectorize (test for excess errors) > PASS: gcc.target/riscv/rvv/autovec/vmv-imm-rv32.c -O3 -ftree-vectorize scan-assembler-times vmv.v.i 32 > PASS: gcc.target/riscv/rvv/autovec/vmv-imm-rv32.c -O3 -ftree-vectorize scan-assembler-times vmv.v.x 8 > > in a plain LP64 `riscv64-linux-gnu' configuration. > > gcc/testsuite/ > * gcc.target/riscv/rvv/autovec/vmv-imm-template.h: Remove > <assert.h> inclusion. OK jeff
+CC Patrick who's been chasing similar issues. On 9/23/23 00:18, Maciej W. Rozycki wrote: > In non-multilib installations system headers may not be available for > compilation options using a non-default model, causing build errors such > as: > > In file included from .../include/features.h:527, > from .../include/assert.h:35, > from .../gcc/testsuite/gcc.target/riscv/rvv/autovec/vmv-imm-template.h:2, > from .../gcc/testsuite/gcc.target/riscv/rvv/autovec/vmv-imm-fixed-rv32.c:4: > .../include/gnu/stubs.h:11:11: fatal error: gnu/stubs-ilp32d.h: No such file or directory > > Therefore we have to be very cautious when trying to use a non-default > model in the testsuite, preferably avoiding to rely on headers that have > not been supplied by GCC itself, or otherwise verifying in a preparatory > step whether the given model is buildable in a given test environment. > > In this case however we can easily avoid the issue, because <assert.h> > facilities are not used at all by "vmv-imm-template.h", which includes > the header. Remove the inclusion then, turning these issues: > > FAIL: gcc.target/riscv/rvv/autovec/vmv-imm-fixed-rv32.c -O3 -ftree-vectorize (test for excess errors) > UNRESOLVED: gcc.target/riscv/rvv/autovec/vmv-imm-fixed-rv32.c -O3 -ftree-vectorize scan-assembler-times vmv.v.i 32 > UNRESOLVED: gcc.target/riscv/rvv/autovec/vmv-imm-fixed-rv32.c -O3 -ftree-vectorize scan-assembler-times vmv.v.x 8 > FAIL: gcc.target/riscv/rvv/autovec/vmv-imm-rv32.c -O3 -ftree-vectorize (test for excess errors) > UNRESOLVED: gcc.target/riscv/rvv/autovec/vmv-imm-rv32.c -O3 -ftree-vectorize scan-assembler-times vmv.v.i 32 > UNRESOLVED: gcc.target/riscv/rvv/autovec/vmv-imm-rv32.c -O3 -ftree-vectorize scan-assembler-times vmv.v.x 8 > > into successful results: > > PASS: gcc.target/riscv/rvv/autovec/vmv-imm-fixed-rv32.c -O3 -ftree-vectorize (test for excess errors) > PASS: gcc.target/riscv/rvv/autovec/vmv-imm-fixed-rv32.c -O3 -ftree-vectorize scan-assembler-times vmv.v.i 32 > PASS: gcc.target/riscv/rvv/autovec/vmv-imm-fixed-rv32.c -O3 -ftree-vectorize scan-assembler-times vmv.v.x 8 > PASS: gcc.target/riscv/rvv/autovec/vmv-imm-rv32.c -O3 -ftree-vectorize (test for excess errors) > PASS: gcc.target/riscv/rvv/autovec/vmv-imm-rv32.c -O3 -ftree-vectorize scan-assembler-times vmv.v.i 32 > PASS: gcc.target/riscv/rvv/autovec/vmv-imm-rv32.c -O3 -ftree-vectorize scan-assembler-times vmv.v.x 8 > > in a plain LP64 `riscv64-linux-gnu' configuration. > > gcc/testsuite/ > * gcc.target/riscv/rvv/autovec/vmv-imm-template.h: Remove > <assert.h> inclusion. > --- > gcc/testsuite/gcc.target/riscv/rvv/autovec/vmv-imm-template.h | 1 - > 1 file changed, 1 deletion(-) > > gcc-test-riscv-rvv-assert.diff > Index: gcc/gcc/testsuite/gcc.target/riscv/rvv/autovec/vmv-imm-template.h > =================================================================== > --- gcc.orig/gcc/testsuite/gcc.target/riscv/rvv/autovec/vmv-imm-template.h > +++ gcc/gcc/testsuite/gcc.target/riscv/rvv/autovec/vmv-imm-template.h > @@ -1,5 +1,4 @@ > #include <stdint-gcc.h> > -#include <assert.h> > > #define VMV_POS(TYPE,VAL) \ > __attribute__ ((noipa)) This fix is great but is there a more general solution to the problem when we toolchain is built for say just rv64 (and thus only those headers) vs. test building for say rv32 (and failing to build due to lack of headers) or vice-versa. Thx, -Vineet
On Sun, 24 Sep 2023, Vineet Gupta wrote: > This fix is great but is there a more general solution to the problem when we > toolchain is built for say just rv64 (and thus only those headers) vs. test > building for say rv32 (and failing to build due to lack of headers) or > vice-versa. The MIPS port has logic in its target test script for combining test options and excluding ones that are mutually incompatible due to ABI or ISA restrictions. It wasn't written by me and I have only minimally tweaked it (and then many years ago), so I can't remember all the details offhand. See the top comment in gcc/testsuite/gcc.target/mips/mips.exp for further information including usage. I guess it would make sense to pinch that logic for our port, especially given our growing number of machine options. I think it was mentioned at one of the patch review calls (Jeff?). NB the use of this specific <assert.h> header, still in place elsewhere, seems gratuitous to me. We don't need or indeed want to print anything in the test cases (unless verifying something specific to the print facility) and if we want to avoid minor code duplication (i.e. not to have explicit: if (...) __builtin_abort (); replicated across test cases), we can easily implement this via a local header, there's no need to pull in a complex system facility. Also I find the use of this facility questionable in the first place: do we want these test cases to pass even in the case of an issue if run with -DNDEBUG as a target board option (which would cause some tests to be optimised away in their entriety)? Maciej
On Mon, 25 Sep 2023, Maciej W. Rozycki wrote: > NB the use of this specific <assert.h> header, still in place elsewhere, > seems gratuitous to me. We don't need or indeed want to print anything in > the test cases (unless verifying something specific to the print facility) > and if we want to avoid minor code duplication (i.e. not to have explicit: > > if (...) > __builtin_abort (); > > replicated across test cases), we can easily implement this via a local > header, there's no need to pull in a complex system facility. Overall we ought not to require any system headers in compile tests and then link and run tests need a functional target environment anyway. So maybe the use of <assert.h> in run tests isn't as bad after all if not for the -DNDEBUG peculiarity. However I still think the less we depend in verification on external components the better, that's one variable to exclude. Maciej
On Fri, 22 Sep 2023, Jeff Law wrote: > > gcc/testsuite/ > > * gcc.target/riscv/rvv/autovec/vmv-imm-template.h: Remove > > <assert.h> inclusion. > OK Now committed, thanks. Maciej
On 9/25/23 15:17, Maciej W. Rozycki wrote: > On Mon, 25 Sep 2023, Maciej W. Rozycki wrote: > >> NB the use of this specific <assert.h> header, still in place elsewhere, >> seems gratuitous to me. We don't need or indeed want to print anything in >> the test cases (unless verifying something specific to the print facility) >> and if we want to avoid minor code duplication (i.e. not to have explicit: >> >> if (...) >> __builtin_abort (); >> >> replicated across test cases), we can easily implement this via a local >> header, there's no need to pull in a complex system facility. > > Overall we ought not to require any system headers in compile tests and > then link and run tests need a functional target environment anyway. So > maybe the use of <assert.h> in run tests isn't as bad after all if not for > the -DNDEBUG peculiarity. However I still think the less we depend in > verification on external components the better, that's one variable to > exclude. Certainly we don't want extraneous #includes. We can often avoid them with a few judicious prototypes, like for abort (). But we also need to get to the point where we can run tests which have #include directives that reference system headers. Many tests in the various GCC testsuites have those directives and we don't want to be continually trying to eradicate #includes from those tests. The standard way to deal with this is single tree builds which are deprecated or to have an install tree with the suitable multilib headers and libraries. The latter seems like the only viable solution to me. jeff
On Wed, 27 Sep 2023 10:28:55 PDT (-0700), jeffreyalaw@gmail.com wrote: > > > On 9/25/23 15:17, Maciej W. Rozycki wrote: >> On Mon, 25 Sep 2023, Maciej W. Rozycki wrote: >> >>> NB the use of this specific <assert.h> header, still in place elsewhere, >>> seems gratuitous to me. We don't need or indeed want to print anything in >>> the test cases (unless verifying something specific to the print facility) >>> and if we want to avoid minor code duplication (i.e. not to have explicit: >>> >>> if (...) >>> __builtin_abort (); >>> >>> replicated across test cases), we can easily implement this via a local >>> header, there's no need to pull in a complex system facility. >> >> Overall we ought not to require any system headers in compile tests and >> then link and run tests need a functional target environment anyway. So >> maybe the use of <assert.h> in run tests isn't as bad after all if not for >> the -DNDEBUG peculiarity. However I still think the less we depend in >> verification on external components the better, that's one variable to >> exclude. > Certainly we don't want extraneous #includes. We can often avoid them > with a few judicious prototypes, like for abort (). > > But we also need to get to the point where we can run tests which have > #include directives that reference system headers. Many tests in the > various GCC testsuites have those directives and we don't want to be > continually trying to eradicate #includes from those tests. > > The standard way to deal with this is single tree builds which are > deprecated or to have an install tree with the suitable multilib headers > and libraries. The latter seems like the only viable solution to me. IMO this is one of those places where we should just be as normal as possible. So if the other big ports allow system headers then we should, otherwise we should move everyone over to testing in some way we'll catch these before commit. > > jeff
On 9/27/23 11:32, Palmer Dabbelt wrote: > > IMO this is one of those places where we should just be as normal as > possible. So if the other big ports allow system headers then we > should, otherwise we should move everyone over to testing in some way > we'll catch these before commit. Exactly. I think the dance we've been doing with stdint-gcc.h is a bit silly, but I haven't pushed on it at all. No other port does anything similar. When they need stdint.h, they include it. It does mean you have to have the appropriate headers installed for each multilib configuration, but that's the way every other port handles this problem. There's no good reason I'm aware of for RISC-V to be different. jeff
On Wed, 27 Sep 2023, Jeff Law wrote: > > IMO this is one of those places where we should just be as normal as > > possible. So if the other big ports allow system headers then we should, > > otherwise we should move everyone over to testing in some way we'll catch > > these before commit. > Exactly. I think the dance we've been doing with stdint-gcc.h is a bit silly, > but I haven't pushed on it at all. > > No other port does anything similar. When they need stdint.h, they include > it. It does mean you have to have the appropriate headers installed for each > multilib configuration, but that's the way every other port handles this > problem. There's no good reason I'm aware of for RISC-V to be different. I agree that using standard system headers where required is a reasonable expectation, but I maintain that when using a non-default ABI/multilib by an explicit request of a test case, it is the responsibility of our test framework to verify that the chosen ABI/multilib is compatible with the environment. I think it should apply equally to all the tests whether they are run, link, or compilation tests. I think a requirement for a verifier to have headers for all the possible ABIs/multilibs installed is unreasonable. For a hosted target such as Linux/*BSD/whatever it may yet be feasible. For a bare-metal target it may not be possible, and in particular such a target may possibly support one specific ABI only and #error if an incompatible configuration is detected. This must not cause a test to FAIL, because GIGO. Overall I think an expectation ought to be for a given ABI/multilib to be verified by running the whole testsuite for the desired configuration, either by having it as the default for the toolchain under test or via an explicit target board option. I accept the desire to have alternative ABIs/multilibs smoke-tested by a couple of tests explicitly requesting them via a compilation option, to verify that the option works if nothing else. I think the burden of verifying the compatibility of the environment must not be on the individual tests, but the framework itself, and I think the MIPS framework fulfils the requirement, as it verifies the options given in the individual tests without the tests themselves having to request anything, they just list the compilation options they require the usual way via `dg-options'. I think that to verify the compatibility of ABI/multilib options for compilation tests we can pick up a system header we can reasonably expect to change depending on the ABI/multilib chosen, and therefore to pull any ABI/multilib-specific bits, such as <limits.h>. If a given environment turns out incompatible with a given ABI/multilib option, then all the tests requesting said option ought to be automatically demoted to UNSUPPORTED. Most importantly implementing this approach in our test framework is a one-off project, while requiring people to have their environment set up for ABI/multilib configurations they have no interest in would cause them continuous effort. Maciej
On 9/28/23 03:46, Maciej W. Rozycki wrote: > On Wed, 27 Sep 2023, Jeff Law wrote: > >>> IMO this is one of those places where we should just be as normal as >>> possible. So if the other big ports allow system headers then we should, >>> otherwise we should move everyone over to testing in some way we'll catch >>> these before commit. >> Exactly. I think the dance we've been doing with stdint-gcc.h is a bit silly, >> but I haven't pushed on it at all. >> >> No other port does anything similar. When they need stdint.h, they include >> it. It does mean you have to have the appropriate headers installed for each >> multilib configuration, but that's the way every other port handles this >> problem. There's no good reason I'm aware of for RISC-V to be different. > > I agree that using standard system headers where required is a reasonable > expectation, but I maintain that when using a non-default ABI/multilib by > an explicit request of a test case, it is the responsibility of our test > framework to verify that the chosen ABI/multilib is compatible with the > environment. I think it should apply equally to all the tests whether > they are run, link, or compilation tests. No other target enforces this kind of requirement and I would maintain that we shouldn't either. > > I think a requirement for a verifier to have headers for all the possible > ABIs/multilibs installed is unreasonable. For a hosted target such as > Linux/*BSD/whatever it may yet be feasible. For a bare-metal target it > may not be possible, and in particular such a target may possibly support > one specific ABI only and #error if an incompatible configuration is > detected. This must not cause a test to FAIL, because GIGO. If you're testing options that you don't have headers/libraries for, then that's a testsuite bug irrespective of bare metal vs linux. It's been like that for about 30 years at this point. I fail to see why RISC-V should be special in this regard. Jeff
Index: gcc/gcc/testsuite/gcc.target/riscv/rvv/autovec/vmv-imm-template.h =================================================================== --- gcc.orig/gcc/testsuite/gcc.target/riscv/rvv/autovec/vmv-imm-template.h +++ gcc/gcc/testsuite/gcc.target/riscv/rvv/autovec/vmv-imm-template.h @@ -1,5 +1,4 @@ #include <stdint-gcc.h> -#include <assert.h> #define VMV_POS(TYPE,VAL) \ __attribute__ ((noipa)) \