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

[Philipp Rudo <prudo at linux dot vnet dot ibm dot com>]

Hi Uli,

On Fri, 24 Nov 2017 20:25:11 +0100 (CET)
"Ulrich Weigand" <uweigand@de.ibm.com> wrote:

> 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.

Yeah i know. I just hoped i could take the quick and dirty way out...

I think it is best when we defer the question whether the kernel target should
use s390-linux-tdep till after the split is final. Then we have a stable base
we can use.

Is there anything in s390-tdep.c left you think that should be moved?
Otherwise ...
 
> > > 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.

... i'll move the regsets and s390_register_call_saved to s390-linux-tdep. The
way the latter currently is implemented doesn't support any other OS than Linux.

For the kernel target I would provide appropriate replacements.
 
> > > 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).

Totally fair. I played a little bit around with OSABI for s390 yesterday. In
principle i got it to work. However I had to refractor s390_gdbarch_init quite
a bit and introduced a bug somewhere. Let me have a closer look at it and come
back to you when I have a proper patch set.

My plan is to add the usage of OSABI on top of the split as is (with the small
changes described above). This makes it easier to find bugs i might introduce
which would otherwise be cloaked by the split. Any rejections?

For the kernel target I think it is best when we defer the discussion till we
have the s390 code work with OSABI. Then we have a way to test.

In principle i think that introducing a GDB_OSABI_LINUX_KERNEL and adept the
sniffer will be much cleaner. However the second way will be much easier to
implement.
 
> > > 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.

My problem aren't the tdep->have_* fields but the corresponding variables
defined in s390_gdbarch_init. With them all there are dependencies all over the
function.

For the hack i did yesterday i looked at i386. Meaning i allocated the
gdbarch_tdep right at the beginning of the function and used its fields instead
of the variables. The way I see it ppc/rs6k works more the less the same.

Thanks
Philipp