Re: [PATCH 1/4] Split up s390-linux-tdep.c into two files

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

Hi Phillip,

> I already feared you will find something. The short answer to most of your
> comments is that the split isn't a 100% clean. The new s390-tdep isn't pure
> architecture code but mixed with common Linux ELF abi code. If you wanted
> it clean we had to split up s390-tdep even further. But I don't know is if
> it is worth the work.

So in general, for all platforms where there is split between ARCH-tdep.c
and ARCH-linux-tdep.c, the former contains code that is applicable to all
targets on that architecture, while the latter contains code that is used
only on Linux targets.  We should really do the same on s390.

Now, it is true that in many cases the split isn't 100% clean.  For example,
on many platforms most OSes use mostly the same calling convention, but with
maybe some minor changes.  Then, it makes sense to have to bulk of the
calling convention stuff in ARCH-tdep.c, but have it check some ABI variant
flag in tdep to modify its behavior.  So your moving all those parts to
s390-tdep.c is perfectly fine with me.

However, s390-tdep.c should *not* contain anything that would *break*
running GDB on some non-Linux OS (I'm e.g. thinking of BSD or OpenSolaris
here, both of which were actually ported to s390 by some people).  That's
why e.g. moving the orig_r2 stuff to s390-tdep.c is not a good idea.

Now, your use case is special.  We currently do not support recognizing
the Linux kernel as a separate target, different from Linux user-space
programs, on *any* architecture.  It is well possible that we'll have to
add something here.  But that is not a s390-specific problem; we'll want
to have that capability on *all* Linux architectures in the end.  So
creating some non-standard s390-tdep.c / s390-linux-tdep.c split just
for that reason isn't useful since it wouldn't help other platforms.

Whether the Linux kernel target should use (or not use) the ARCH-linux-tdep.c
file is an interesting question, but it should be answered the same way on
all architectures.

> > Not sure I understand your split here.  "ORIG_R2" is not a real register
> > and only exists on Linux, so it shouldn't move to s390-tdep.c.  On the
> > other hand, the split between upper halves and lower halves seems more
> > generic and could happen elsewhere ...
> 
> This is a little more complicated.
> 
> For the upper/lower split you are right, it is architecture. However i see
> it as legacy code and I don't plan to add any support for 31-bit systems on
> 64-bit machines. So i decided to leave the definition where it is used.
> 
> For orig_r2 you are right (again), it only exist in user space. However
> (again) the kernel uses the same prstatus definitions like the user space,
> including a field for orig_r2 (although it is not used). Thats why i moved it
> to the common 'Linux ELF abi' s390-tdep.

Thinking about this again, all the regsets should probably remain
in s390-linux-tdep.c.  This stuff is usually different between OSes anyway.

For the kernel structure, you can provide your own regset that simply
ignores the orig_r2 field in the structure.

> > This method is same across all Linux (and really all) targets; there is
> > no point for us to try to do it differently, that would just cause confusion.
> 
> I played around with osabi and never got it to work the way I wanted it to.
> My problem was that ELF doesn't distinguish the kernel and user space. So the
> default ELF sniffer always used the GDB_OSABI_LINUX and never gave my
> GDB_OSABI_LINUX_KERNEL a chance. I cannot recall all details and will give it
> a second chance. Please give me some time.

As said above, this is really a different problem.  The first issue is that
on s390, we should use the same split between ARCH-tdep.c and ARCH-linux-tdep.c
as everybody else, and that means using an OSABI sniffer.

The second issue is that we somehow need to distinguish between Linux kernel
debugging and Linux user-space debugging.  I'm not sure what the best way to
do this is; we could have a separate GDB_OSABI_LINUX_KERNEL as you mention
and would then have to adapt the (common code) sniffer routines.

Or maybe both kernel and user space should use GDB_OSABI_LINUX and then
the Linux ABI routine just handles them differently?  Hard to say for me
at the moment (probably because I haven't seen the actual kernel code yet).


> > The Linux specific feature should be handled here, the others in the 
> > generic part.
> 
> My plan was to take care of this when we change to dynamically creating
> the tdesc. Then we could number the registers directly when creating the tdesc
> (at least I hope so). To be honest I haven't done it yet as with all those
> have_* variables there are some nasty dependencies in the function...

It shouldn't be really difficult to split those up.  Again, e.g. ppc does
the same thing already today.  All the have_ flags can stay in the common
tdep struct, which makes this easier.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com