Maciej Rozycki <maciej.rozy...@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
