| Message ID | alpine.DEB.2.20.17.1611101343100.10580@tp.orcam.me.uk |
|---|---|
| State | Accepted |
| Headers | show |
Maciej Rozycki <Maciej.Rozycki@imgtec.com> writes: > This however requires the correct annotation of branch targets as code, > because the ISA mode is not relevant for data symbols and is therefore > not recorded for them. I wonder if it would have been possible to add the ISA mode to data symbols and hide it in readelf/objdump? I don't know what older binutils would have done with such labels but it would have made the new checks compatible with pre-existing GCC code generation. Regardless the changes in this patch would still be important to correctly identify labels as text. > Let it be produced then, making it appear in output generated right > after `$L2' definitions above and thus fixing the assembly. Use the > `mach2' pass, after all the MIPS16 constant pools have been fixed, to > scan the insn stream backwards, identifying any labels still present at > the end of a function or immediately preceding a MIPS16 constant pool, > using dummy `consttable' insns previously inserted to identify the > beginning of each such constant pool. Insert the `insn_pseudo' insn > immediately after these labels, which emits the `.insn' pseudo-op. > > References: > > [1] "MIPS Architecture for Programmers, Volume II-B: The microMIPS32 > Instruction Set", MIPS Technologies, Inc., Document Number: MD00582, > Revision 5.04, January 15, 2014, Section 7.1 "Assembly-Level > Compatibility", p. 533 > > [2] "MIPS Architecture for Programmers, Volume II-B: The microMIPS64 > Instruction Set", MIPS Technologies, Inc., Document Number: MD00594, > Revision 5.04, January 15, 2014, Section 8.1 "Assembly-Level > Compatibility", p. 623 > > gcc/ > * config/mips/mips.c (mips16_emit_constants): Emit `consttable' > insn at the beginning of the constant pool. > (mips_insert_insn_pseudos): New function. > (mips_machine_reorg2): Call it. > * config/mips/mips.md (unspec): Add UNSPEC_CONSTTABLE and > UNSPEC_INSN_PSEUDO enum values. > (insn_pseudo, consttable): New insns. > > gcc/testsuite/ > * gcc.target/mips/insn-casesi.c: New test case. > * gcc.target/mips/insn-pseudo-1.c: New test case. > * gcc.target/mips/insn-pseudo-2.c: New test case. > * gcc.target/mips/insn-pseudo-3.c: New test case. > * gcc.target/mips/insn-pseudo-4.c: New test case. > * gcc.target/mips/insn-tablejump.c: New test case. > --- > I have successfully regression-tested it with the `mips-mti-linux-gnu' > target, with a big-endian o32 regular MIPS multilib and a little-endian > o32 MIPS16 multilib, with no regressions, except as noted below. I did > some big-endian n64 regular MIPS and little-endian o32 microMIPS > testing, including with the new cases, and things looked good, except as > noted below. I also generated assembly manually (for the assembly-match > cases) and examined output visually, including all the four above > multilibs, and also -fPIC and -mno-abicalls variants, which I have no > immediate way of testing automatically. As noted below and my opinion in general... Dealing with the intricacies of getting the MIPS part of the GCC testsuite running cleanly for all variations of the architecture is a prohibitively expensive process to apply to each patch. Now that we are in stage 3 then various testsuite issues will get dealt with. > With n64 (`-mabi=n64') testing none of the test cases under > gcc.target/mips/ were run and the test harness broke as follows: > > ERROR: (DejaGnu) proc "cc1: error: '-mfpxx' can only be used with the > o32 ABI" does not exist. > The error code is NONE > The info on the error is: > invalid command name "cc1:" > while executing > "::tcl_unknown cc1: error: '-mfpxx' can only be used with the o32 ABI" > ("uplevel" body line 1) > invoked from within > "uplevel 1 ::tcl_unknown $args" > > I take it as a bug in the harness, which ought to be looked into > separately, and not a problem with this change. I haven't seen this failure before which may be down to a different way of invoking the testsuite I guess (I have run n64 testing fairly recently). > With MIPS16 (`-mips16') and microMIPS (`-mmicromips') testing the test > cases failed to compile as follows, respectively: > > spawn -ignore SIGHUP .../gcc/gcc/xgcc -B.../gcc/gcc/ > .../gcc/testsuite/gcc.target/mips/insn-tablejump.c -fno-diagnostics- > show-caret -fdiagnostics-color=never --sysroot=.../sysroot -O0 - > DNOMIPS16=__attribute__((nomips16)) - > DNOMICROMIPS=__attribute__((nomicromips)) - > DNOCOMPRESSION=__attribute__((nocompression)) -mmicromips -mno-mips16 - > DMICROMIPS=__attribute__((micromips)) -EL -mips16 -Wl,-dynamic- > linker,.../sysroot/mipsel-r2-mips16-hard/lib/ld.so.1 -Wl,- > rpath,.../sysroot/mipsel-r2-mips16-hard/lib -Wl,- > rpath,.../sysroot/mipsel-r2-mips16-hard/usr/lib -lm -o insn- > tablejump.exe > cc1: error: unsupported combination: -mips16 -mmicromips compiler exited > with status 1 output is: > cc1: error: unsupported combination: -mips16 -mmicromips > > FAIL: gcc.target/mips/insn-tablejump.c -O0 (test for excess errors) > Excess errors: > cc1: error: unsupported combination: -mips16 -mmicromips > > and: > > spawn -ignore SIGHUP .../gcc/gcc/xgcc -B.../gcc/gcc/ > .../gcc/testsuite/gcc.target/mips/insn-casesi.c -fno-diagnostics-show- > caret -fdiagnostics-color=never --sysroot=.../sysroot -O0 - > DNOMIPS16=__attribute__((nomips16)) - > DNOMICROMIPS=__attribute__((nomicromips)) - > DNOCOMPRESSION=__attribute__((nocompression)) -mno-micromips -mips16 - > mcode-readable=yes -DMIPS16=__attribute__((mips16)) -EL -mmicromips-Wl,- > dynamic-linker,.../sysroot/micromipsel-r2-hard/lib/ld.so.1 -Wl,- > rpath,.../sysroot/micromipsel-r2-hard/lib -Wl,- > rpath,.../sysroot/micromipsel-r2-hard/usr/lib -lm -o insn-casesi.exe > cc1: error: unsupported combination: -mips16 -mmicromips compiler exited > with status 1 output is: > cc1: error: unsupported combination: -mips16 -mmicromips > > FAIL: gcc.target/mips/insn-casesi.c -O0 (test for excess errors) > Excess errors: > cc1: error: unsupported combination: -mips16 -mmicromips > > Again, I take it as a bug in the harness, which places the required > overrides specified and implied by `dg-options' before board compilation > flags rather than afterwards. As such it's a separate problem which has > nothing to do with this change and needs to be fixed independently. I see nothing wrong with the way the tests are described so it does look like a harness issue. There are tests that fall into each of the two categories of failure but as above I don't recall seeing these kind of failures before. > OK to apply? OK, thanks. Matthew
On Mon, 14 Nov 2016, Matthew Fortune wrote: > > This however requires the correct annotation of branch targets as code, > > because the ISA mode is not relevant for data symbols and is therefore > > not recorded for them. > > I wonder if it would have been possible to add the ISA mode to data > symbols and hide it in readelf/objdump? I don't know what older binutils > would have done with such labels but it would have made the new checks > compatible with pre-existing GCC code generation. Regardless the changes > in this patch would still be important to correctly identify labels as > text. Well, I suppose we could add ISA mode annotation (i.e. STO_MIPS16 and STO_MICROMIPS flags, as applicable) to data symbols and then ignore it for anything but branch/jump validation. I have mixed feelings about such an arrangement though, and I don't find jumping to data symbols particularly clean in the first place, so I think we should avoid it anyway, at least in generated code. Also I reckon errors from ISA mode checks in binutils have already identified a few Linux kernel bugs as support for microMIPS compilation was added there, so I think these checks have proved themselves useful and attempts should not be made to defeat them. > > --- > > I have successfully regression-tested it with the `mips-mti-linux-gnu' > > target, with a big-endian o32 regular MIPS multilib and a little-endian > > o32 MIPS16 multilib, with no regressions, except as noted below. I did > > some big-endian n64 regular MIPS and little-endian o32 microMIPS > > testing, including with the new cases, and things looked good, except as > > noted below. I also generated assembly manually (for the assembly-match > > cases) and examined output visually, including all the four above > > multilibs, and also -fPIC and -mno-abicalls variants, which I have no > > immediate way of testing automatically. > > As noted below and my opinion in general... Dealing with the intricacies > of getting the MIPS part of the GCC testsuite running cleanly for all > variations of the architecture is a prohibitively expensive process to > apply to each patch. Now that we are in stage 3 then various testsuite > issues will get dealt with. Having test bots running the interesting matrix of targets and multilibs might help a bit, although most likely only over changes already committed unless we have a way to submit candidate patches for testing. I believe the Linux kernel project has actually got this covered as I do see failure reports about people's mailing list patch submissions sent right away regularly, although for build errors only which are surely easier to catch. Perhaps we could learn from their experience though sometime. > > With n64 (`-mabi=n64') testing none of the test cases under > > gcc.target/mips/ were run and the test harness broke as follows: > > > > ERROR: (DejaGnu) proc "cc1: error: '-mfpxx' can only be used with the > > o32 ABI" does not exist. > > The error code is NONE > > The info on the error is: > > invalid command name "cc1:" > > while executing > > "::tcl_unknown cc1: error: '-mfpxx' can only be used with the o32 ABI" > > ("uplevel" body line 1) > > invoked from within > > "uplevel 1 ::tcl_unknown $args" > > > > I take it as a bug in the harness, which ought to be looked into > > separately, and not a problem with this change. > > I haven't seen this failure before which may be down to a different way > of invoking the testsuite I guess (I have run n64 testing fairly recently). I have figured out that the precedence of compilation flags is set in `default_target_compile' in /usr/share/dejagnu/target.exp, so it's not something we can easily control locally unless (in the usual TCL way) we override that procedure, which is quite a substantial piece of code actually. Based on this observation however I have determined that moving multilib and related flags such as `-mabi=n64' and possibly also `-Wl,-rpath,...' from `$board_info(...,cflags)' over to `$board_info(...,multilib_flags)' helps a bit and reduces the number of test suite issues, although still causes occasional troubles with link tests where wrong multilib matching happens in the absence of an exact match, such as with `-mabi=n64 -mmicromips' choosing the (o32) `-mmicromips' multilib which is not link compatible rather than the (non-microMIPS) `-mabi=n64' which usually is (with the minor issue of short delay slots in cross-mode jump relaxation, which might be solvable by the harness with the `-minterlink-compressed' option where required). But that's yet another problem, which needs to be addressed elsewhere if we choose to. I am a bit nervous about having board-dependent compilation flags split across multiple variables, but if that's what DejaGNU forces upon us, then let it be (at least until/unless someone finds incentive to make any changes here). This particular ERROR however, that is trying to interpret a compilation error message as a TCL procedure name, is I think worth looking into, as we should be getting the failure paths right, even the more exotic ones. We've most likely missed a quotation or escapes somewhere, causing the error message to be interpreted rather than being treated literally. > > With MIPS16 (`-mips16') and microMIPS (`-mmicromips') testing the test > > cases failed to compile as follows, respectively: > > > > spawn -ignore SIGHUP .../gcc/gcc/xgcc -B.../gcc/gcc/ > > .../gcc/testsuite/gcc.target/mips/insn-tablejump.c -fno-diagnostics- > > show-caret -fdiagnostics-color=never --sysroot=.../sysroot -O0 - > > DNOMIPS16=__attribute__((nomips16)) - > > DNOMICROMIPS=__attribute__((nomicromips)) - > > DNOCOMPRESSION=__attribute__((nocompression)) -mmicromips -mno-mips16 - > > DMICROMIPS=__attribute__((micromips)) -EL -mips16 -Wl,-dynamic- > > linker,.../sysroot/mipsel-r2-mips16-hard/lib/ld.so.1 -Wl,- > > rpath,.../sysroot/mipsel-r2-mips16-hard/lib -Wl,- > > rpath,.../sysroot/mipsel-r2-mips16-hard/usr/lib -lm -o insn- > > tablejump.exe > > cc1: error: unsupported combination: -mips16 -mmicromips compiler exited > > with status 1 output is: > > cc1: error: unsupported combination: -mips16 -mmicromips > > > > FAIL: gcc.target/mips/insn-tablejump.c -O0 (test for excess errors) > > Excess errors: > > cc1: error: unsupported combination: -mips16 -mmicromips > > > > and: > > > > spawn -ignore SIGHUP .../gcc/gcc/xgcc -B.../gcc/gcc/ > > .../gcc/testsuite/gcc.target/mips/insn-casesi.c -fno-diagnostics-show- > > caret -fdiagnostics-color=never --sysroot=.../sysroot -O0 - > > DNOMIPS16=__attribute__((nomips16)) - > > DNOMICROMIPS=__attribute__((nomicromips)) - > > DNOCOMPRESSION=__attribute__((nocompression)) -mno-micromips -mips16 - > > mcode-readable=yes -DMIPS16=__attribute__((mips16)) -EL -mmicromips-Wl,- > > dynamic-linker,.../sysroot/micromipsel-r2-hard/lib/ld.so.1 -Wl,- > > rpath,.../sysroot/micromipsel-r2-hard/lib -Wl,- > > rpath,.../sysroot/micromipsel-r2-hard/usr/lib -lm -o insn-casesi.exe > > cc1: error: unsupported combination: -mips16 -mmicromips compiler exited > > with status 1 output is: > > cc1: error: unsupported combination: -mips16 -mmicromips > > > > FAIL: gcc.target/mips/insn-casesi.c -O0 (test for excess errors) > > Excess errors: > > cc1: error: unsupported combination: -mips16 -mmicromips > > > > Again, I take it as a bug in the harness, which places the required > > overrides specified and implied by `dg-options' before board compilation > > flags rather than afterwards. As such it's a separate problem which has > > nothing to do with this change and needs to be fixed independently. > > I see nothing wrong with the way the tests are described so it does look > like a harness issue. There are tests that fall into each of the two > categories of failure but as above I don't recall seeing these kind of > failures before. These as well have been eliminated with the use of `multilib_flags' although as I noted above I still see link failures from a wrong inexact multilib selection sometimes. > > OK to apply? > > OK, thanks. Patch applied now, along with the other two in this series. Thanks for your review. Maciej
Maciej Rozycki <Maciej.Rozycki@imgtec.com> writes: > On Mon, 14 Nov 2016, Matthew Fortune wrote: > > > > This however requires the correct annotation of branch targets as > > > code, because the ISA mode is not relevant for data symbols and is > > > therefore not recorded for them. > > > > I wonder if it would have been possible to add the ISA mode to data > > symbols and hide it in readelf/objdump? I don't know what older > > binutils would have done with such labels but it would have made the > > new checks compatible with pre-existing GCC code generation. > > Regardless the changes in this patch would still be important to > > correctly identify labels as text. > > Well, I suppose we could add ISA mode annotation (i.e. STO_MIPS16 and > STO_MICROMIPS flags, as applicable) to data symbols and then ignore it > for anything but branch/jump validation. I have mixed feelings about > such an arrangement though, and I don't find jumping to data symbols > particularly clean in the first place, so I think we should avoid it > anyway, at least in generated code. Also I reckon errors from ISA mode > checks in binutils have already identified a few Linux kernel bugs as > support for microMIPS compilation was added there, so I think these > checks have proved themselves useful and attempts should not be made to > defeat them. Sure. I just thought I'd mention it as an idea. > > > --- > > > I have successfully regression-tested it with the `mips-mti-linux- > gnu' > > > target, with a big-endian o32 regular MIPS multilib and a little- > endian > > > o32 MIPS16 multilib, with no regressions, except as noted below. I > did > > > some big-endian n64 regular MIPS and little-endian o32 microMIPS > > > testing, including with the new cases, and things looked good, > except as > > > noted below. I also generated assembly manually (for the assembly- > match > > > cases) and examined output visually, including all the four above > > > multilibs, and also -fPIC and -mno-abicalls variants, which I have > no > > > immediate way of testing automatically. > > > > As noted below and my opinion in general... Dealing with the > intricacies > > of getting the MIPS part of the GCC testsuite running cleanly for all > > variations of the architecture is a prohibitively expensive process to > > apply to each patch. Now that we are in stage 3 then various testsuite > > issues will get dealt with. > > Having test bots running the interesting matrix of targets and > multilibs > might help a bit, although most likely only over changes already > committed > unless we have a way to submit candidate patches for testing. I believe > the Linux kernel project has actually got this covered as I do see > failure > reports about people's mailing list patch submissions sent right away > regularly, although for build errors only which are surely easier to > catch. Perhaps we could learn from their experience though sometime. Yes, I get started on the setup for this every once in a while and then run out of time. > > > With n64 (`-mabi=n64') testing none of the test cases under > > > gcc.target/mips/ were run and the test harness broke as follows: > > > > > > ERROR: (DejaGnu) proc "cc1: error: '-mfpxx' can only be used with > the > > > o32 ABI" does not exist. > > > The error code is NONE > > > The info on the error is: > > > invalid command name "cc1:" > > > while executing > > > "::tcl_unknown cc1: error: '-mfpxx' can only be used with the o32 > ABI" > > > ("uplevel" body line 1) > > > invoked from within > > > "uplevel 1 ::tcl_unknown $args" > > > > > > I take it as a bug in the harness, which ought to be looked into > > > separately, and not a problem with this change. > > > > I haven't seen this failure before which may be down to a different > way > > of invoking the testsuite I guess (I have run n64 testing fairly > recently). > > I have figured out that the precedence of compilation flags is set in > `default_target_compile' in /usr/share/dejagnu/target.exp, so it's not > something we can easily control locally unless (in the usual TCL way) we > override that procedure, which is quite a substantial piece of code > actually. > > Based on this observation however I have determined that moving > multilib > and related flags such as `-mabi=n64' and possibly also `-Wl,-rpath,...' > from `$board_info(...,cflags)' over to `$board_info(...,multilib_flags)' > helps a bit and reduces the number of test suite issues, although still > causes occasional troubles with link tests where wrong multilib matching > happens in the absence of an exact match, such as with `-mabi=n64 > -mmicromips' choosing the (o32) `-mmicromips' multilib which is not link > compatible rather than the (non-microMIPS) `-mabi=n64' which usually is > (with the minor issue of short delay slots in cross-mode jump > relaxation, > which might be solvable by the harness with the `-minterlink-compressed' > option where required). So have you been modifying the board file to add compile options? I normally just add them with --target_board=blah/-mabi=n64 which presumably places the options differently. I'll need to dig in to the code to see. Can you confirm your method of setting test options and the board file you are using? > But that's yet another problem, which needs to be addressed elsewhere > if > we choose to. I am a bit nervous about having board-dependent > compilation > flags split across multiple variables, but if that's what DejaGNU forces > upon us, then let it be (at least until/unless someone finds incentive > to > make any changes here). > > This particular ERROR however, that is trying to interpret a > compilation > error message as a TCL procedure name, is I think worth looking into, as > we should be getting the failure paths right, even the more exotic ones. > We've most likely missed a quotation or escapes somewhere, causing the > error message to be interpreted rather than being treated literally. Agreed. > > > With MIPS16 (`-mips16') and microMIPS (`-mmicromips') testing the > test > > > cases failed to compile as follows, respectively: > > > > > > spawn -ignore SIGHUP .../gcc/gcc/xgcc -B.../gcc/gcc/ > > > .../gcc/testsuite/gcc.target/mips/insn-tablejump.c -fno-diagnostics- > > > show-caret -fdiagnostics-color=never --sysroot=.../sysroot -O0 - > > > DNOMIPS16=__attribute__((nomips16)) - > > > DNOMICROMIPS=__attribute__((nomicromips)) - > > > DNOCOMPRESSION=__attribute__((nocompression)) -mmicromips -mno- > mips16 - > > > DMICROMIPS=__attribute__((micromips)) -EL -mips16 -Wl,-dynamic- > > > linker,.../sysroot/mipsel-r2-mips16-hard/lib/ld.so.1 -Wl,- > > > rpath,.../sysroot/mipsel-r2-mips16-hard/lib -Wl,- > > > rpath,.../sysroot/mipsel-r2-mips16-hard/usr/lib -lm -o insn- > > > tablejump.exe > > > cc1: error: unsupported combination: -mips16 -mmicromips compiler > exited > > > with status 1 output is: > > > cc1: error: unsupported combination: -mips16 -mmicromips > > > > > > FAIL: gcc.target/mips/insn-tablejump.c -O0 (test for excess > errors) > > > Excess errors: > > > cc1: error: unsupported combination: -mips16 -mmicromips > > > > > > and: > > > > > > spawn -ignore SIGHUP .../gcc/gcc/xgcc -B.../gcc/gcc/ > > > .../gcc/testsuite/gcc.target/mips/insn-casesi.c -fno-diagnostics- > show- > > > caret -fdiagnostics-color=never --sysroot=.../sysroot -O0 - > > > DNOMIPS16=__attribute__((nomips16)) - > > > DNOMICROMIPS=__attribute__((nomicromips)) - > > > DNOCOMPRESSION=__attribute__((nocompression)) -mno-micromips -mips16 > - > > > mcode-readable=yes -DMIPS16=__attribute__((mips16)) -EL -mmicromips- > Wl,- > > > dynamic-linker,.../sysroot/micromipsel-r2-hard/lib/ld.so.1 -Wl,- > > > rpath,.../sysroot/micromipsel-r2-hard/lib -Wl,- > > > rpath,.../sysroot/micromipsel-r2-hard/usr/lib -lm -o insn-casesi.exe > > > cc1: error: unsupported combination: -mips16 -mmicromips compiler > exited > > > with status 1 output is: > > > cc1: error: unsupported combination: -mips16 -mmicromips > > > > > > FAIL: gcc.target/mips/insn-casesi.c -O0 (test for excess errors) > > > Excess errors: > > > cc1: error: unsupported combination: -mips16 -mmicromips > > > > > > Again, I take it as a bug in the harness, which places the required > > > overrides specified and implied by `dg-options' before board > compilation > > > flags rather than afterwards. As such it's a separate problem which > has > > > nothing to do with this change and needs to be fixed independently. > > > > I see nothing wrong with the way the tests are described so it does > look > > like a harness issue. There are tests that fall into each of the two > > categories of failure but as above I don't recall seeing these kind of > > failures before. > > These as well have been eliminated with the use of `multilib_flags' > although as I noted above I still see link failures from a wrong inexact > multilib selection sometimes. Incorrect multilib selection is always a risk when we have a partial set built especially under the current matching logic for multilibs. I have a complicated set of multi-lib updates to post that allow a multilib'd toolchain to 'know' about all possible incompatible multilibs but not necessarily build them all. This means that instead of getting a link failure because of linking against the wrong library you will not find the library in the first place. I don't have a good enough writeup nor test coverage for other multilib toolchain builds to post it yet though. Matthew
On Wed, 16 Nov 2016, Matthew Fortune wrote: > > Based on this observation however I have determined that moving > > multilib > > and related flags such as `-mabi=n64' and possibly also `-Wl,-rpath,...' > > from `$board_info(...,cflags)' over to `$board_info(...,multilib_flags)' > > helps a bit and reduces the number of test suite issues, although still > > causes occasional troubles with link tests where wrong multilib matching > > happens in the absence of an exact match, such as with `-mabi=n64 > > -mmicromips' choosing the (o32) `-mmicromips' multilib which is not link > > compatible rather than the (non-microMIPS) `-mabi=n64' which usually is > > (with the minor issue of short delay slots in cross-mode jump > > relaxation, > > which might be solvable by the harness with the `-minterlink-compressed' > > option where required). > > So have you been modifying the board file to add compile options? I > normally just add them with --target_board=blah/-mabi=n64 which presumably > places the options differently. I'll need to dig in to the code to see. > Can you confirm your method of setting test options and the board file > you are using? Yes, I do set all the compiler flags within board description files themselves, because not all flags possible are compatible with individual board hardware, starting from the endianness. So I'd rather have a set of prearranged self-contained board description files than having to choose correct multilib flags each time on test suite invocation to match the board selected (and vice versa), and possibly getting it wrong. Also I having chains of multilib-specific `-Wl,-dynamic-linker,...' and `-Wl,-rpath,...' options -- needed to get at the right dynamic libraries from the sysroot on the target board -- passed on the command line is I think all but convenient, especially given that the mapping between these and the multilib selected with other compiler flags is bijective. Anything beyond that, needed for specific testing (say to check whether a change has not regressed `-mcode-readable=no' code generation on hardware which accepts any `-mcode-readable=' setting) may still be specified on the test suite invocation command line, or so I think at least. > > These as well have been eliminated with the use of `multilib_flags' > > although as I noted above I still see link failures from a wrong inexact > > multilib selection sometimes. > > Incorrect multilib selection is always a risk when we have a partial set > built especially under the current matching logic for multilibs. I have > a complicated set of multi-lib updates to post that allow a multilib'd > toolchain to 'know' about all possible incompatible multilibs but not > necessarily build them all. This means that instead of getting a link > failure because of linking against the wrong library you will not find > the library in the first place. I don't have a good enough writeup nor > test coverage for other multilib toolchain builds to post it yet though. Having a full multilib set built is I think impractical, and some may not even be fully supported although selectable. As long as we have a base line though, such as a set of regular MIPS multilibs built across the ABIs supported and both endiannesses, then we can always use them as the lowest common denominator, downgrading any higher ISA level or ISA options selected by a binary linked to those base-line multilibs, rather than switching the ABI or the endianness by accident. But that requires a lot of care in writing multilib selection recipes, as you have already observed. NB that is not limited to running the test suite only and may be useful in the field too, e.g. to build a MIPS16 or microMIPS app occasionally and make it use regular MIPS startup and system libraries where no respective multilib has been configured in the compiler. That is supposed to work in principle, and can usually still be achieved even with the driver being confused about multilib selection, by carefully switching between the relevant options at different build stages, i.e. compilation, assembly and linking. It would be good if such jumping through the hoops could be avoided though. Maciej
Index: gcc/gcc/config/mips/mips.c =================================================================== --- gcc.orig/gcc/config/mips/mips.c 2016-11-09 15:04:00.793911910 +0000 +++ gcc/gcc/config/mips/mips.c 2016-11-09 15:04:20.254759246 +0000 @@ -17133,6 +17133,8 @@ mips16_emit_constants (struct mips16_con int align; align = 0; + if (constants) + insn = emit_insn_after (gen_consttable (), insn); for (c = constants; c != NULL; c = next) { /* If necessary, increase the alignment of PC. */ @@ -19008,6 +19010,46 @@ mips16_split_long_branches (void) while (something_changed); } +/* Insert a `.insn' assembly pseudo-op after any labels followed by + a MIPS16 constant pool or no insn at all. This is needed so that + targets that have been optimised away are still marked as code + and therefore branches that remained and point to them are known + to retain the ISA mode and as such can be successfully assembled. */ + +static void +mips_insert_insn_pseudos (void) +{ + bool insn_pseudo_needed = TRUE; + rtx_insn *insn; + + for (insn = get_last_insn (); insn != NULL_RTX; insn = PREV_INSN (insn)) + switch (GET_CODE (insn)) + { + case INSN: + if (GET_CODE (PATTERN (insn)) == UNSPEC_VOLATILE + && XINT (PATTERN (insn), 1) == UNSPEC_CONSTTABLE) + { + insn_pseudo_needed = TRUE; + break; + } + /* Fall through. */ + case JUMP_INSN: + case CALL_INSN: + case JUMP_TABLE_DATA: + insn_pseudo_needed = FALSE; + break; + case CODE_LABEL: + if (insn_pseudo_needed) + { + emit_insn_after (gen_insn_pseudo (), insn); + insn_pseudo_needed = FALSE; + } + break; + default: + break; + } +} + /* Implement TARGET_MACHINE_DEPENDENT_REORG. */ static void @@ -19043,6 +19085,7 @@ mips_machine_reorg2 (void) optimizations, but this should be an extremely rare case anyhow. */ mips_reorg_process_insns (); mips16_split_long_branches (); + mips_insert_insn_pseudos (); return 0; } Index: gcc/gcc/config/mips/mips.md =================================================================== --- gcc.orig/gcc/config/mips/mips.md 2016-11-09 15:04:00.802108728 +0000 +++ gcc/gcc/config/mips/mips.md 2016-11-09 15:04:20.271031267 +0000 @@ -120,6 +120,7 @@ ;; MIPS16 constant pools. UNSPEC_ALIGN + UNSPEC_CONSTTABLE UNSPEC_CONSTTABLE_INT UNSPEC_CONSTTABLE_FLOAT @@ -151,6 +152,9 @@ ;; Stack checking. UNSPEC_PROBE_STACK_RANGE + + ;; The `.insn' pseudo-op. + UNSPEC_INSN_PSEUDO ]) (define_constants @@ -7174,6 +7178,14 @@ return "#nop"; } [(set_attr "type" "nop")]) + +;; The `.insn' pseudo-op. +(define_insn "insn_pseudo" + [(unspec_volatile [(const_int 0)] UNSPEC_INSN_PSEUDO)] + "" + ".insn" + [(set_attr "mode" "none") + (set_attr "insn_count" "0")]) ;; MIPS4 Conditional move instructions. @@ -7308,6 +7320,13 @@ ;; .................... ;; +(define_insn "consttable" + [(unspec_volatile [(const_int 0)] UNSPEC_CONSTTABLE)] + "" + "" + [(set_attr "mode" "none") + (set_attr "insn_count" "0")]) + (define_insn "consttable_tls_reloc" [(unspec_volatile [(match_operand 0 "tls_reloc_operand" "") (match_operand 1 "const_int_operand" "")] Index: gcc/gcc/testsuite/gcc.target/mips/insn-casesi.c =================================================================== --- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ gcc/gcc/testsuite/gcc.target/mips/insn-casesi.c 2016-11-09 18:55:39.856029933 +0000 @@ -0,0 +1,112 @@ +/* { dg-do run } */ +/* { dg-options "-mips16 -mcode-readable=yes" } */ + +int __attribute__ ((noinline)) +frob (int i) +{ + switch (i) + { + case -5: + return -2; + case -3: + return -1; + case 0: + return 0; + case 3: + return 1; + case 5: + break; + default: + __builtin_unreachable (); + } + return i; +} + +int +main (int argc, char **argv) +{ + asm ("" : "+r" (argc)); + argc = frob ((argc & 10) - 5); + asm ("" : "+r" (argc)); + return !argc; +} + +/* This will result in assembly like: + + .text + .align 2 + .globl frob + .set mips16 + .set nomicromips + .ent frob + .type frob, @function +frob: + .frame $sp,0,$31 # vars= 0, regs= 0/0, args= 0, gp= 0 + .mask 0x00000000,0 + .fmask 0x00000000,0 + addiu $2,$4,5 + sltu $2,11 + bteqz $L2 + sll $3,$2,1 + la $2,$L4 + addu $3,$2,$3 + lh $3,0($3) + addu $2,$2,$3 + j $2 + .align 1 + .align 2 +$L4: + .half $L3-$L4 + .half $L2-$L4 + .half $L9-$L4 + .half $L2-$L4 + .half $L2-$L4 + .half $L8-$L4 + .half $L2-$L4 + .half $L2-$L4 + .half $L7-$L4 + .half $L2-$L4 + .half $L8-$L4 +$L8: + .set noreorder + .set nomacro + jr $31 + move $2,$4 + .set macro + .set reorder + +$L9: + li $2,1 + .set noreorder + .set nomacro + jr $31 + neg $2,$2 + .set macro + .set reorder + +$L3: + li $2,2 + .set noreorder + .set nomacro + jr $31 + neg $2,$2 + .set macro + .set reorder + +$L7: + .set noreorder + .set nomacro + jr $31 + li $2,1 + .set macro + .set reorder + +$L2: + .insn + .end frob + .size frob, .-frob + + for `frob' and we want to make sure it links correctly owing to the + `.insn' pseudo-op which needs to be there at `$L2' as there's no + code following and the label is a MIPS16 branch target (even though + the branch is never taken. See also insn-tablejump.c. */ Index: gcc/gcc/testsuite/gcc.target/mips/insn-pseudo-1.c =================================================================== --- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ gcc/gcc/testsuite/gcc.target/mips/insn-pseudo-1.c 2016-11-09 18:55:39.866624599 +0000 @@ -0,0 +1,23 @@ +/* { dg-do compile } */ +/* { dg-options "-mno-micromips -mno-mips16" } */ + +void +unreachable (int i) +{ + asm volatile goto ("b\t.\n\tbeqz\t%0,%l1" : : "r" (i) : : punt); +punt: + __builtin_unreachable (); +} + +/* Expect assembly like: + + beqz $4,$L2 + # Anything goes here. +$L2: # The label must match. + .insn +$L3 = . # It's there, but we don't care. + .end unreachable + + that is .insn to be inserted if a code label is at function's end. */ + +/* { dg-final { scan-assembler "\tbeqz\t\\\$\[0-9\]+,(.L\[0-9\]+)\n.*\n\\1:\n\t\\.insn\n(?:.L\[0-9\]+ = \\.\n)?\t\\.end\tunreachable\n" } } */ Index: gcc/gcc/testsuite/gcc.target/mips/insn-pseudo-2.c =================================================================== --- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ gcc/gcc/testsuite/gcc.target/mips/insn-pseudo-2.c 2016-11-09 18:55:39.876167216 +0000 @@ -0,0 +1,23 @@ +/* { dg-do compile } */ +/* { dg-options "-mmicromips" } */ + +void +unreachable (int i) +{ + asm volatile goto ("b\t.\n\tbeqz\t%0,%l1" : : "r" (i) : : punt); +punt: + __builtin_unreachable (); +} + +/* Expect assembly like: + + beqz $4,$L2 + # Anything goes here. +$L2: # The label must match. + .insn +$L3 = . # It's there, but we don't care. + .end unreachable + + that is .insn to be inserted if a code label is at function's end. */ + +/* { dg-final { scan-assembler "\tbeqz\t\\\$\[0-9\]+,(.L\[0-9\]+)\n.*\n\\1:\n\t\\.insn\n(?:.L\[0-9\]+ = \\.\n)?\t\\.end\tunreachable\n" } } */ Index: gcc/gcc/testsuite/gcc.target/mips/insn-pseudo-3.c =================================================================== --- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ gcc/gcc/testsuite/gcc.target/mips/insn-pseudo-3.c 2016-11-09 18:55:39.892706789 +0000 @@ -0,0 +1,23 @@ +/* { dg-do compile } */ +/* { dg-options "-mips16" } */ + +void +unreachable (int i) +{ + asm volatile goto ("b\t.\n\tbeqz\t%0,%l1" : : "r" (i) : : punt); +punt: + __builtin_unreachable (); +} + +/* Expect assembly like: + + beqz $4,$L2 + # Anything goes here. +$L2: # The label must match. + .insn +$L3 = . # It's there, but we don't care. + .end unreachable + + that is .insn to be inserted if a code label is at function's end. */ + +/* { dg-final { scan-assembler "\tbeqz\t\\\$\[0-9\]+,(.L\[0-9\]+)\n.*\n\\1:\n\t\\.insn\n(?:.L\[0-9\]+ = \\.\n)?\t\\.end\tunreachable\n" } } */ Index: gcc/gcc/testsuite/gcc.target/mips/insn-pseudo-4.c =================================================================== --- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ gcc/gcc/testsuite/gcc.target/mips/insn-pseudo-4.c 2016-11-09 18:55:39.933208494 +0000 @@ -0,0 +1,27 @@ +/* { dg-do compile } */ +/* { dg-options "-mips16 -mcode-readable=yes" } */ + +void +unreachable (void) +{ + asm volatile goto ("b\t.\n\tbeqz\t%0,%l1" : : "r" (0x12345678) : : punt); +punt: + __builtin_unreachable (); +} + +/* Expect assembly like: + + lw $2,$L5 + # Anything goes here. + beqz $2,$L2 # The register must match. + # Anything goes here. +$L2: # The label must match. + .insn +$L3 = . # It's there, but we don't care. + .align 2 +$L5: # The label must match. + .word 305419896 + + that is .insn to be inserted if a code label is at a constant pool. */ + +/* { dg-final { scan-assembler "\tlw\t(\\\$\[0-9\]+),(.L\[0-9\]+)\n.*\tbeqz\t\\1,(.L\[0-9\]+)\n.*\n\\3:\n\t\\.insn\n(?:.L\[0-9\]+ = \\.\n)?\t\\.align\t2\n\\2:\n\t\\.word\t305419896\n" } } */ Index: gcc/gcc/testsuite/gcc.target/mips/insn-tablejump.c =================================================================== --- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ gcc/gcc/testsuite/gcc.target/mips/insn-tablejump.c 2016-11-09 18:55:39.954800846 +0000 @@ -0,0 +1,98 @@ +/* { dg-do run } */ +/* { dg-options "-mmicromips" } */ + +int __attribute__ ((noinline)) +frob (int i) +{ + switch (i) + { + case -5: + return -2; + case -3: + return -1; + case 0: + return 0; + case 3: + return 1; + case 5: + break; + default: + __builtin_unreachable (); + } + return i; +} + +int +main (int argc, char **argv) +{ + asm ("" : "+r" (argc)); + argc = frob ((argc & 10) - 5); + asm ("" : "+r" (argc)); + return !argc; +} + +/* This will result in assembly like: + + .text + .align 2 + .globl frob + .set nomips16 + .set micromips + .ent frob + .type frob, @function +frob: + .frame $sp,0,$31 # vars= 0, regs= 0/0, args= 0, gp= 0 + .mask 0x00000000,0 + .fmask 0x00000000,0 + .set noreorder + .set nomacro + addiu $3,$4,5 + sltu $2,$3,11 + beqzc $2,$L2 + lui $2,%hi($L4) + addiu $2,$2,%lo($L4) + lwxs $3,$3($2) + jrc $3 + .rdata + .align 2 + .align 2 +$L4: + .word $L3 + .word $L2 + .word $L9 + .word $L2 + .word $L2 + .word $L8 + .word $L2 + .word $L2 + .word $L7 + .word $L2 + .word $L8 + .text +$L8: + jr $31 + move $2,$4 + +$L9: + jr $31 + li $2,-1 # 0xffffffffffffffff + +$L3: + jr $31 + li $2,-2 # 0xfffffffffffffffe + +$L7: + jr $31 + li $2,1 # 0x1 + +$L2: + .insn + .set macro + .set reorder + .end frob + .size frob, .-frob + + for `frob' and we want to make sure it links correctly owing to the + `.insn' pseudo-op which needs to be there at `$L2' as there's no + code following and the label is a microMIPS branch target (even though + the branch is never taken. See also insn-casesi.c. */
Mark trailing labels, either at the end of a function or preceding a MIPS16 constant pool, with the `.insn' assembly pseudo-op so that they are considered code rather than data labels and consequently have ISA annotation added in the symbol table by the assembler. Such labels are created for some cases of unreachable code like with the following example: $ cat switch.c int foo (int i) { static int j; j += i; switch (i) { case -5: return -2; case -3: return -1; case 0: return 0; case 3: return 1; case 5: break; default: __builtin_unreachable (); } return j; } $ gcc -S -mips16 -O2 -o switch-mips16.s switch.c $ cat switch-mips16.s .file 1 "switch.c" .section .mdebug.abi32 .previous .nan legacy .module fp=xx .module nooddspreg .abicalls .option pic0 .text .align 2 .globl foo .set mips16 .set nomicromips .ent foo .type foo, @function foo: .frame $sp,0,$31 # vars= 0, regs= 0/0, args= 0, gp= 0 .mask 0x00000000,0 .fmask 0x00000000,0 lw $3,$L11 lw $2,0($3) addu $2,$4,$2 sw $2,0($3) addiu $3,$4,5 sltu $3, 11 bteqz $L2 sll $5, $3, 1 la $3, $L4 addu $5, $3, $5 lh $5, 0($5) addu $3, $3, $5 j $3 .align 1 .align 2 $L4: .half $L9-$L4 .half $L2-$L4 .half $L5-$L4 .half $L2-$L4 .half $L2-$L4 .half $L6-$L4 .half $L2-$L4 .half $L2-$L4 .half $L7-$L4 .half $L2-$L4 .half $L1-$L4 $L9: li $2,2 neg $2,$2 $L1: jr $31 $L7: .set noreorder .set nomacro jr $31 li $2,1 .set macro .set reorder $L6: .set noreorder .set nomacro jr $31 move $2,$4 .set macro .set reorder $L5: li $2,1 .set noreorder .set nomacro jr $31 neg $2,$2 .set macro .set reorder $L2: .align 2 $L11: .word j.1474 .end foo .size foo, .-foo .local j.1474 .comm j.1474,4,4 .ident "GCC: (GNU) 7.0.0 20160810 (experimental)" $ where `$L2' is placed before MIPS16 constant pool data, or: $ gcc -S -mmicromips -O2 -o switch-micromips.s switch.c $ cat switch-micromips.s .file 1 "switch.c" .section .mdebug.abi32 .previous .nan legacy .module fp=xx .module nooddspreg .abicalls .option pic0 .text .align 2 .globl foo .set nomips16 .set micromips .ent foo .type foo, @function foo: .frame $sp,0,$31 # vars= 0, regs= 0/0, args= 0, gp= 0 .mask 0x00000000,0 .fmask 0x00000000,0 .set noreorder .set nomacro lui $5,%hi(j.1401) addiu $3,$4,5 lw $2,%lo(j.1401)($5) sltu $6,$3,11 addu $2,$4,$2 beqz $6,$L2 sw $2,%lo(j.1401)($5) lui $5,%hi($L4) addiu $5,$5,%lo($L4) lwxs $3,$3($5) jrc $3 .rdata .align 2 .align 2 $L4: .word $L9 .word $L2 .word $L5 .word $L2 .word $L2 .word $L6 .word $L2 .word $L2 .word $L7 .word $L2 .word $L1 .text $L9: li $2,-2 # 0xfffffffffffffffe $L1: jrc $31 $L7: jr $31 li $2,1 # 0x1 $L6: jr $31 move $2,$4 $L5: jr $31 li $2,-1 # 0xffffffffffffffff $L2: .set macro .set reorder .end foo .size foo, .-foo .local j.1401 .comm j.1401,4,4 .ident "GCC: (GNU) 7.0.0 20160810 (experimental)" where the same label is placed at the end of a function. This in turn makes recent trunk versions of gas complain: $ as -o switch-mips16.o switch-mips16.s switch-mips16.s: Assembler messages: switch-mips16.s:26: Error: branch to a symbol in another ISA mode $ as commit 9d862524f6ae ("MIPS: Verify the ISA mode and alignment of branch and jump targets") closed a hole in branch processing, making relocation calculation respect the ISA mode of the symbol referred. This allowed diagnosing the situation where an attempt is made to pass control from code assembled for one ISA mode to code assembled for a different ISA mode and either relaxing the branch to a cross-mode jump or if that is not possible, then reporting this as an error rather than letting such code build and then fail unpredictably at the run time. This however requires the correct annotation of branch targets as code, because the ISA mode is not relevant for data symbols and is therefore not recorded for them. The `.insn' pseudo-op is used for this purpose and has been supported by GAS since: Wed Feb 12 14:36:29 1997 Ian Lance Taylor <ian@cygnus.com> * config/tc-mips.c (mips_pseudo_table): Add "insn". (s_insn): New static function. * doc/c-mips.texi: Document .insn. so there has been no reason to avoid it where required. More recently this pseudo-op has been documented, by the microMIPS architecture specification[1][2], as required for the correct interpretation of any code label which is not followed by an actual instruction in an assembly source. Let it be produced then, making it appear in output generated right after `$L2' definitions above and thus fixing the assembly. Use the `mach2' pass, after all the MIPS16 constant pools have been fixed, to scan the insn stream backwards, identifying any labels still present at the end of a function or immediately preceding a MIPS16 constant pool, using dummy `consttable' insns previously inserted to identify the beginning of each such constant pool. Insert the `insn_pseudo' insn immediately after these labels, which emits the `.insn' pseudo-op. References: [1] "MIPS Architecture for Programmers, Volume II-B: The microMIPS32 Instruction Set", MIPS Technologies, Inc., Document Number: MD00582, Revision 5.04, January 15, 2014, Section 7.1 "Assembly-Level Compatibility", p. 533 [2] "MIPS Architecture for Programmers, Volume II-B: The microMIPS64 Instruction Set", MIPS Technologies, Inc., Document Number: MD00594, Revision 5.04, January 15, 2014, Section 8.1 "Assembly-Level Compatibility", p. 623 gcc/ * config/mips/mips.c (mips16_emit_constants): Emit `consttable' insn at the beginning of the constant pool. (mips_insert_insn_pseudos): New function. (mips_machine_reorg2): Call it. * config/mips/mips.md (unspec): Add UNSPEC_CONSTTABLE and UNSPEC_INSN_PSEUDO enum values. (insn_pseudo, consttable): New insns. gcc/testsuite/ * gcc.target/mips/insn-casesi.c: New test case. * gcc.target/mips/insn-pseudo-1.c: New test case. * gcc.target/mips/insn-pseudo-2.c: New test case. * gcc.target/mips/insn-pseudo-3.c: New test case. * gcc.target/mips/insn-pseudo-4.c: New test case. * gcc.target/mips/insn-tablejump.c: New test case. --- I have successfully regression-tested it with the `mips-mti-linux-gnu' target, with a big-endian o32 regular MIPS multilib and a little-endian o32 MIPS16 multilib, with no regressions, except as noted below. I did some big-endian n64 regular MIPS and little-endian o32 microMIPS testing, including with the new cases, and things looked good, except as noted below. I also generated assembly manually (for the assembly-match cases) and examined output visually, including all the four above multilibs, and also -fPIC and -mno-abicalls variants, which I have no immediate way of testing automatically. With n64 (`-mabi=n64') testing none of the test cases under gcc.target/mips/ were run and the test harness broke as follows: ERROR: (DejaGnu) proc "cc1: error: '-mfpxx' can only be used with the o32 ABI" does not exist. The error code is NONE The info on the error is: invalid command name "cc1:" while executing "::tcl_unknown cc1: error: '-mfpxx' can only be used with the o32 ABI" ("uplevel" body line 1) invoked from within "uplevel 1 ::tcl_unknown $args" I take it as a bug in the harness, which ought to be looked into separately, and not a problem with this change. With MIPS16 (`-mips16') and microMIPS (`-mmicromips') testing the test cases failed to compile as follows, respectively: spawn -ignore SIGHUP .../gcc/gcc/xgcc -B.../gcc/gcc/ .../gcc/testsuite/gcc.target/mips/insn-tablejump.c -fno-diagnostics-show-caret -fdiagnostics-color=never --sysroot=.../sysroot -O0 -DNOMIPS16=__attribute__((nomips16)) -DNOMICROMIPS=__attribute__((nomicromips)) -DNOCOMPRESSION=__attribute__((nocompression)) -mmicromips -mno-mips16 -DMICROMIPS=__attribute__((micromips)) -EL -mips16 -Wl,-dynamic-linker,.../sysroot/mipsel-r2-mips16-hard/lib/ld.so.1 -Wl,-rpath,.../sysroot/mipsel-r2-mips16-hard/lib -Wl,-rpath,.../sysroot/mipsel-r2-mips16-hard/usr/lib -lm -o insn-tablejump.exe cc1: error: unsupported combination: -mips16 -mmicromips compiler exited with status 1 output is: cc1: error: unsupported combination: -mips16 -mmicromips FAIL: gcc.target/mips/insn-tablejump.c -O0 (test for excess errors) Excess errors: cc1: error: unsupported combination: -mips16 -mmicromips and: spawn -ignore SIGHUP .../gcc/gcc/xgcc -B.../gcc/gcc/ .../gcc/testsuite/gcc.target/mips/insn-casesi.c -fno-diagnostics-show-caret -fdiagnostics-color=never --sysroot=.../sysroot -O0 -DNOMIPS16=__attribute__((nomips16)) -DNOMICROMIPS=__attribute__((nomicromips)) -DNOCOMPRESSION=__attribute__((nocompression)) -mno-micromips -mips16 -mcode-readable=yes -DMIPS16=__attribute__((mips16)) -EL -mmicromips-Wl,-dynamic-linker,.../sysroot/micromipsel-r2-hard/lib/ld.so.1 -Wl,-rpath,.../sysroot/micromipsel-r2-hard/lib -Wl,-rpath,.../sysroot/micromipsel-r2-hard/usr/lib -lm -o insn-casesi.exe cc1: error: unsupported combination: -mips16 -mmicromips compiler exited with status 1 output is: cc1: error: unsupported combination: -mips16 -mmicromips FAIL: gcc.target/mips/insn-casesi.c -O0 (test for excess errors) Excess errors: cc1: error: unsupported combination: -mips16 -mmicromips Again, I take it as a bug in the harness, which places the required overrides specified and implied by `dg-options' before board compilation flags rather than afterwards. As such it's a separate problem which has nothing to do with this change and needs to be fixed independently. OK to apply? Maciej gcc-mips-label-insn.diff