RE: [PATCH 3/3] MIPS/GCC: Mark trailing labels with `.insn'
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
RE: [PATCH 3/3] MIPS/GCC: Mark trailing labels with `.insn'
Maciej Rozycki 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 opt
RE: [PATCH 3/3] MIPS/GCC: Mark trailing labels with `.insn'
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 compi
RE: [PATCH 3/3] MIPS/GCC: Mark trailing labels with `.insn'
Maciej Rozycki 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 -mmicr
