From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 71816 invoked by alias); 24 Nov 2017 16:55:22 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 71806 invoked by uid 89); 24 Nov 2017 16:55:21 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.9 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,KB_WAM_FROM_NAME_SINGLEWORD,RCVD_IN_DNSWL_LOW autolearn=no version=3.3.2 spammy=nasty, uli X-HELO: mx0a-001b2d01.pphosted.com Received: from mx0b-001b2d01.pphosted.com (HELO mx0a-001b2d01.pphosted.com) (148.163.158.5) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 24 Nov 2017 16:55:20 +0000 Received: from pps.filterd (m0098417.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id vAOGsdCc074036 for ; Fri, 24 Nov 2017 11:55:18 -0500 Received: from e06smtp12.uk.ibm.com (e06smtp12.uk.ibm.com [195.75.94.108]) by mx0a-001b2d01.pphosted.com with ESMTP id 2eek8rakgs-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Fri, 24 Nov 2017 11:55:18 -0500 Received: from localhost by e06smtp12.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 24 Nov 2017 16:55:16 -0000 Received: from b06cxnps4074.portsmouth.uk.ibm.com (9.149.109.196) by e06smtp12.uk.ibm.com (192.168.101.142) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Fri, 24 Nov 2017 16:55:13 -0000 Received: from d06av25.portsmouth.uk.ibm.com (d06av25.portsmouth.uk.ibm.com [9.149.105.61]) by b06cxnps4074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id vAOGtDkr44761174; Fri, 24 Nov 2017 16:55:13 GMT Received: from d06av25.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id F0FD011C04C; Fri, 24 Nov 2017 16:49:51 +0000 (GMT) Received: from d06av25.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id C85D211C04A; Fri, 24 Nov 2017 16:49:51 +0000 (GMT) Received: from ThinkPad (unknown [9.152.212.148]) by d06av25.portsmouth.uk.ibm.com (Postfix) with ESMTP; Fri, 24 Nov 2017 16:49:51 +0000 (GMT) Date: Fri, 24 Nov 2017 16:55:00 -0000 From: Philipp Rudo To: "Ulrich Weigand" Cc: gdb-patches@sourceware.org, arnez@linux.vnet.ibm.com (Andreas Arnez) Subject: Re: [PATCH 1/4] Split up s390-linux-tdep.c into two files In-Reply-To: <20171123202603.2FDEDD803CD@oc3748833570.ibm.com> References: <20171123153733.31261-2-prudo@linux.vnet.ibm.com> <20171123202603.2FDEDD803CD@oc3748833570.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 x-cbid: 17112416-0008-0000-0000-000004AF487A X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17112416-0009-0000-0000-00001E421BC4 Message-Id: <20171124175511.33626ff8@ThinkPad> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-11-24_06:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 lowpriorityscore=0 impostorscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1709140000 definitions=main-1711240227 X-IsSubscribed: yes X-SW-Source: 2017-11/txt/msg00620.txt.bz2 Hi Uli, thanks for the review. 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. More details are below. On Thu, 23 Nov 2017 21:26:03 +0100 (CET) "Ulrich Weigand" wrote: > Philipp Rudo wrote: > > > Currently all target dependent code for s390 is in one file > > (s390-linux-tdep.c). This includes code general for the architecture as > > well as code specific for uses in GNU/Linux (user space). Up until now > > this was ok as GNU/Linux was the only supported OS. In preparation to > > support the new Linux kernel 'OS' split up the existing s390 code into a > > general s390-tdep and a GNU/Linux specific s390-linux-tdep. > > Thanks for working on this! > > A couple of comments on the particular split: > > > > -static void > > -s390_write_pc (struct regcache *regcache, CORE_ADDR pc) > > -{ > > - struct gdbarch *gdbarch = regcache->arch (); > > - struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); > > - > > - regcache_cooked_write_unsigned (regcache, tdep->pc_regnum, pc); > > - > > - /* Set special SYSTEM_CALL register to 0 to prevent the kernel from > > - messing with the PC we just installed, if we happen to be within > > - an interrupted system call that the kernel wants to restart. > > - > > - Note that after we return from the dummy call, the SYSTEM_CALL and > > - ORIG_R2 registers will be automatically restored, and the kernel > > - continues to restart the system call at this point. */ > > - if (register_size (gdbarch, S390_SYSTEM_CALL_REGNUM) > 0) > > - regcache_cooked_write_unsigned (regcache, S390_SYSTEM_CALL_REGNUM, 0); > > -} > > This seems a typical Linux issue. Other OSes won't have the ORIG_R2 > and SYSTEM_CALL registers and may not need this particular handling. You are right for the kernel we will have to treat it different. I moved it back to s390-linux-tdep. > > -/* Maps for register sets. */ > > - > > -static const struct regcache_map_entry s390_gregmap[] = > > - { > > - { 1, S390_PSWM_REGNUM }, > > - { 1, S390_PSWA_REGNUM }, > > - { 16, S390_R0_REGNUM }, > > - { 16, S390_A0_REGNUM }, > > - { 1, S390_ORIG_R2_REGNUM }, > > - { 0 } > > - }; > > - > > -static const struct regcache_map_entry s390_fpregmap[] = > > - { > > - { 1, S390_FPC_REGNUM, 8 }, > > - { 16, S390_F0_REGNUM, 8 }, > > - { 0 } > > - }; > > - > > static const struct regcache_map_entry s390_regmap_upper[] = > > { > > { 16, S390_R0_UPPER_REGNUM, 4 }, > > 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. > > +/* Set up GNU/Linux gdbarch. Allocates struct gdbarch if needed. */ > > > > static struct gdbarch * > > -s390_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) > > +s390_linux_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches) > > { > > This is wrong. The way the split setup between generic part and Linux > part is supposed to be working is this: > > - The generic part (and only it) has a _initialize routine that calls > register_gdbarch_init (bfd_arch_s390, s390_gdbarch_init); > > - In the s390_gdbarch_init routine, the generic setup is done, and at > the point where the platform-specific setup should begin, we call > gdbarch_init_osabi (info, gdbarch); > > - The OS-specific part also has a _initialize routine, which calls > gdbarch_register_osabi (bfd_arch_s390, bfd_mach_s390_31, GDB_OSABI_LINUX, > s390_linux_init_abi); > gdbarch_register_osabi (bfd_arch_s390, bfd_mach_s390_64, GDB_OSABI_LINUX, > s390_linux_init_abi); > (may be the same or two different handlers for 31- vs 64-bit, whatever > is more practical) > > - The common gdbarch_init_osabi detects which OS handler to use and calls it. > > 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. > > - /* Optional GNU/Linux-specific "registers". */ > > - feature = tdesc_find_feature (tdesc, "org.gnu.gdb.s390.linux"); > > - if (feature) > > - { > > - tdesc_numbered_register (feature, tdesc_data, > > - S390_ORIG_R2_REGNUM, "orig_r2"); > > - > > - if (tdesc_numbered_register (feature, tdesc_data, > > - S390_LAST_BREAK_REGNUM, "last_break")) > > - have_linux_v1 = 1; > > - > > - if (tdesc_numbered_register (feature, tdesc_data, > > - S390_SYSTEM_CALL_REGNUM, "system_call")) > > - have_linux_v2 = 1; > > - > > - if (have_linux_v2 > have_linux_v1) > > - valid_p = 0; > > - } > > 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... > > -/* GNU/Linux-specific optional registers. */ > > -#define S390_ORIG_R2_REGNUM 67 > > -#define S390_LAST_BREAK_REGNUM 68 > > -#define S390_SYSTEM_CALL_REGNUM 69 > > Ideally, these should also remain here as Linux specific registers. > This can be done e.g. like ppc-linux-tdep.h does it. The ppc way is actually pretty nice. I will use it. Thanks Philipp