Re: [PATCH] sim: testsuite: push $arch out to targets

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Mike Frysinger <vapier@gentoo.org> [2021-01-22 01:36:05 -0500]:

> On 21 Jan 2021 09:22, Andrew Burgess wrote:
> > * Mike Frysinger <vapier@gentoo.org> [2021-01-18 13:01:53 -0500]:
> > > On 18 Jan 2021 09:52, Andrew Burgess wrote:
> > > > * Mike Frysinger [2021-01-17 11:09:45 -0500]:
> > > > > This is needed to move to automake & its dejagnu-provided logic,
> > > > > and eventually by the unified sim logic.
> > > > 
> > > > I looked through this patch and I didn't understand what's going on
> > > > here.
> > > > 
> > > > If this needs doing at all, could it not be done in some global location?
> > > 
> > > the sim ports use a unique subdir for their `run` program.  the tests
> > > need to find that path.  this $arch value is what binds the specific
> > > subdir to the test.
> > 
> > I don't understand that paragraph I'm afraid.
> > 
> > I guess my question is each *.exp file gets invoked by dejagnu, but
> > there are other hooks that dejagnu calls, like the ${tool}_init proc.
> > Could we not use that to set these things instead?  It seems like
> > there's a 1:1 mapping between [istarget ????] patterns and the values
> > pushed into both arch and all_machs.
> 
> i'm not seeing how any hooks would help.  the targets need to know where
> their sim program lives.  let me brain dump on you :p.
> 
> lets look at a bfin & frv build tree.
> build-bfin-elf/
> `-- sim/
>     `-- bfin/
>         `-- run
> build-frv-elf/
> `-- sim/
>     `-- frv/
>         `-- run
> 
> sim/testsuite/frv/*.exp files need to know to look for $(builddir)/frv/run
> while sim/testsuite/bfin/*.exp files need $(builddir)/bfin/run.
> 
> i know the variable is called $arch and that can be confusing.  i could
> rename it if it helps.  just keep in mind that this is used for exactly
> one thing: where to find `run` under $(builddir)/.
> 
> when we invoke dejagnu, it was with --tool=sim.  if we had combined trees
> with other tools (gas/gdb/etc...), this would make sense, but it doesn't,
> so we changed it to --tool="".  now when you run `runtest`, it will find
> all *.exp files under sim/testsuite/ and attempt to execute them.

OK.  I don't understand why changing tool from "sim" to "" is either
necessary, or a good thing, but, whatever....

For gdb we have a 'gdb_init' proc, which is called based on
'${tool}_init' from within GDB.  Now that we have no tool set does
this prevent us having an 'init' proc?

Lets assume for one moment that we _did_ have an init proc.  Could
that proc not just have one huge [istarget ???] { set arch ... } tree
that handled all known istarget patterns?

If we can't have an init proc because we no longer have a tool set,
then lets revisit that decision.

My understanding is that we can't have a tool name set because our
testsuite is organised like this:

  sim/testsuite/TESTSET/*.exp

In contrast, GDB's tests are named like:

  gdb/testsuite/gdb.TESTSET/*.exp

Could we not just rename our test directories along a similar pattern,
reset the tool name, then use the init function that gets called to
figure out all of the boiler plate stuff that needs setting.

I think you say that right now we have pretty much one test script for
each arch, but there's no reason which this has to be the case
forever, right?

It just seems weird to me to say that each test is required to start
with a highly predictable bit of boiler plate...

Thanks for indulging me,

Andrew



> 
> today, for any given target, there is at most one sim port available to
> them.  the opposite obviously cannot be said:
> * bfin-*-* will match the bare-metal, the Linux FLAT, and the Linux FDPIC
>   toolchains, not to mention every infinite variation of the vendor field
> * arm*-*-* will match every ARM CPU variant out there, plus every other
>   OS & vendor variant
> 
> if we focus on [istarget ????] to $arch mappings, they would only map to
> one $arch value, but more than one ???? can map to the same $arch.  i
> don't mean this in terms of the infinite tuples i mentioned above, but
> in a more real world sense:
> * mips/*.exp has many variations
> * h8300/*.exp has a few variations
> * cris/**.exp has a few variations
> 
> this is why stuffing the $arch from configure.tgt has worked OK up to
> now: it controls whether the sim port is enabled (e.g. bfin-*-*), and
> then the tests will run only for a subset (e.g. [istarget bfin-*-elf]).
> 
> but the [istarget] check is a relic of the past.  we should not care
> what we're targetting, we should care whether the sim port is enabled.
> so for most arches, i could flip the check like:
> 	-if [istarget bfin-*-elf] {
> 	+set arch "bfin"
> 	+set sim "$objdir/../$arch/run"
> 	+if [file exists $sim] {
> 
> but some ports have assumptions built into their testsuites that i
> don't want to untangle today.  and assumptions in the toolchain like
> gas not supporting multitarget well.
> 
> so getting back to why i'm pushing $arch out: as an incremental step
> to multibuild & eventually multitarget.  in multibuild, i have now:
> build/
> `-- sim/
>     |-- arm/
>     |   `-- run
>     |-- bfin/
>     |   `-- run
>     |-- frv/
>     |   `-- run
>     ...
> 
> with multitarget, we'd start with the multibuild layout above, but
> slowly migrate ports one-by-one to:
> build/
> `-- sim/
>     |-- run     <-- supports multiple arches in one binary e.g. bfin
>     |-- arm/    <-- arm hasn't migrated yet
>     |           <-- no bfin because it's in the multione above
>     |-- frv/    <-- frv hasn't migrated yet
>     ...
> even in a non-multitarget scenario, we'd still use the flat layout:
> build-bfin/
> `-- sim/
>     `-- run
> 
> as the arch cuts over to multitarget, we'd also update its corresponding
> *.exp settings so that it'd use sim/run instead of sim/$arch/run.  but
> all the ones that haven't migrated would still need $arch set.
> 
> one might argue this change is a little premature when i don't have a
> working multitarget build working now, but i found it helpful as i've
> been migrating things to automake & a centralized build to do this now,
> and i know we'll need to eventually kick the value out of configure.
> so might as well do it now and be done.
> -mike