From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11351 invoked by alias); 3 May 2017 14:38:13 -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 11276 invoked by uid 89); 3 May 2017 14:38:12 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.1 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_LOW autolearn=no version=3.3.2 spammy=sick, classified, Kindly 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; Wed, 03 May 2017 14:38:10 +0000 Received: from pps.filterd (m0098414.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.20/8.16.0.20) with SMTP id v43EXauR114318 for ; Wed, 3 May 2017 10:38:10 -0400 Received: from e06smtp12.uk.ibm.com (e06smtp12.uk.ibm.com [195.75.94.108]) by mx0b-001b2d01.pphosted.com with ESMTP id 2a790sf6pj-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Wed, 03 May 2017 10:38:10 -0400 Received: from localhost by e06smtp12.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 3 May 2017 15:38:08 +0100 Received: from b06cxnps3074.portsmouth.uk.ibm.com (9.149.109.194) by e06smtp12.uk.ibm.com (192.168.101.142) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Wed, 3 May 2017 15:38:06 +0100 Received: from d06av25.portsmouth.uk.ibm.com (d06av25.portsmouth.uk.ibm.com [9.149.105.61]) by b06cxnps3074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id v43Ec57h14090612; Wed, 3 May 2017 14:38:05 GMT Received: from d06av25.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id E1B0D11C069; Wed, 3 May 2017 15:36:57 +0100 (BST) Received: from d06av25.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id ACFF611C05C; Wed, 3 May 2017 15:36:57 +0100 (BST) Received: from ThinkPad (unknown [9.152.212.148]) by d06av25.portsmouth.uk.ibm.com (Postfix) with ESMTP; Wed, 3 May 2017 15:36:57 +0100 (BST) Date: Wed, 03 May 2017 14:38:00 -0000 From: Philipp Rudo To: Omair Javaid Cc: "gdb-patches@sourceware.org" , Yao Qi , Peter Griffin , Andreas Arnez , Lee Jones , Russell Wayman Subject: Re: [RFC v3 3/8] Add basic Linux kernel support In-Reply-To: References: <20170316165739.88524-1-prudo@linux.vnet.ibm.com> <20170316165739.88524-4-prudo@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-TM-AS-GCONF: 00 x-cbid: 17050314-0008-0000-0000-0000043A7389 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17050314-0009-0000-0000-00001D83A45D Message-Id: <20170503163803.6589319a@ThinkPad> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-05-03_11:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=1 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1703280000 definitions=main-1705030271 X-IsSubscribed: yes X-SW-Source: 2017-05/txt/msg00056.txt.bz2 Hi Omair, sorry for the late reply but I was sick the last two weeks... On Mon, 17 Apr 2017 03:58:35 +0500 Omair Javaid wrote: > Hi Philip, >=20 > I like your handling of linux kernel data structures though I havent > been able to get your code working on arm. I'm glad to hear it. > There are some challenges with regards to live debugging support which > I am trying to figure out. There is no reliable way to tell between a > kernel direct mapped address, vmalloc address and module address when > we also have user address available. I'm afraid you will find more challenges like these. Although I tried to b= e as general as possible I most likely missed some things you will need for live debugging ... I don't think there is a reliable way to tell what kind of address you have when you only have its unsigned long value. On s390 (at least in theory) it should be possible to find out via lowcore ("percpu state"). There the currently running task is stored. So you could try something like lowcore->current_task->(mm|active_mm). I actually never needed to find out what kind of address I have as for dump= s the user space is usually stripped off. This makes live a lot easier as both (direct mapped and vmalloc'ed addresses (including modules)) can be accessed via the kernels page table (init_mm->pgd). So you don't have to care about what kind of address you have. Furthermore they never get swapp= ed out. In theory this method could also be used to access user space memory if you know the corresponding page table and the memory isn't swapped out. But he= re GDB has the problem, that to_xfer_partial (in gdb/target.h) only gets the address as unsigned long but has absolutely no information about the address space it lives in ... =20 > Also there is no way to switch between stratums if we need to do so in > case we try to support switching between userspace and kernel space. No, GDB currently doesn't allow switching between straums. This is one poi= nt on my long ToDo-list... > As far as this patch is concerned there are no major issues that can > block me from further progressing towards live debugging support. >=20 > I have compiled this patch with arm support on top and overall this > looks good. See some minor inline comments. >=20 > Yao: Kindly check if there are any coding convention or styling issues he= re. >=20 > PS: I have not looked at module support or s390 target specific code in > detail. No Problem just do one step after the other and don't waste too much time on s390. The architecture has some quirks ;) > Thanks! >=20 > -- > Omair >=20 >=20 [...] > > + > > +size_t > > +lk_bitmap_find_next_bit (ULONGEST *bitmap, size_t size, size_t bit) > > +{ > > + size_t ulong_size, bits_per_ulong, elt; > > + > > + ulong_size =3D lk_builtin_type_size (unsigned_long); > > + bits_per_ulong =3D ulong_size * LK_BITS_PER_BYTE; > > + elt =3D bit / bits_per_ulong; > > + > > + while (bit < size) > > + {=20=20 >=20 > Will this be portable across endianess? The generic implementation for the bitmap functions in the kernel rely on BITMAP_LAST_WORD_MASK. As Andreas mentioned earlier this macro creates a m= ask for the least significant bits. So this implementation should be portable.= At least it works on s390, which is big-endian. =20 > > + /* FIXME: Explain why using lsb0 bit order. */ > > + if (bitmap[elt] & (1UL << (bit % bits_per_ulong))) > > + return bit; > > + > > + bit++; > > + if (bit % bits_per_ulong =3D=3D 0) > > + elt++; > > + } > > + > > + return size; > > +} > > +=20=20 >=20 > lk_bitmap_hweight seems un-used. > I wonder if there is generic implementation available for this > function somewhere in binutils-gdb sources. > Can we use something like __builtin_popcount from GCC intrinsic? See reply to your next mail. =20 > > +/* Returns the Hamming weight, i.e. number of set bits, of bitmap BITM= AP > > + with size SIZE (in bits). */ > > + > > +size_t > > +lk_bitmap_hweight (ULONGEST *bitmap, size_t size) > > +{ > > + size_t ulong_size, bit, bits_per_ulong, elt, retval; > > + > > + ulong_size =3D lk_builtin_type_size (unsigned_long); > > + bits_per_ulong =3D ulong_size * LK_BITS_PER_BYTE; > > + elt =3D bit =3D 0; > > + retval =3D 0; > > + > > + while (bit < size) > > + { > > + if (bitmap[elt] & (1 << bit % bits_per_ulong)) > > + retval++; > > + > > + bit++; > > + if (bit % bits_per_ulong =3D=3D 0) > > + elt++; > > + } > > + > > + return retval; > > +} > > + [...] > This function throws an error while compiling for arm-linux target on > x86_64 host. >=20 > lk-low.c: In function =E2=80=98void init_linux_kernel_ops()=E2=80=99: > lk-low.c:812:20: error: invalid conversion from =E2=80=98char* > (*)(target_ops*, ptid_t)=E2=80=99 to =E2=80=98const char* (*)(target_ops*= , ptid_t)=E2=80=99 > [-fpermissive] > t->to_pid_to_str =3D lk_pid_to_str; Could it be that you applied the patch on a master with Pedros "Enable -Wwrite-strings" series? https://sourceware.org/ml/gdb-patches/2017-04/msg00028.html In this series the hook got "const-ified" (7a1149643). It isn't considered= in my patches yet (aka. need to rebase to the current master, but that will be= a lot of work with all the C++-ification ...). =20 > > +/* Function for targets to_pid_to_str hook. Marks running tasks with = an > > + asterisk "*". */ > > + > > +static char * > > +lk_pid_to_str (struct target_ops *target, ptid_t ptid) > > +{ > > + static char buf[64]; > > + long pid; > > + CORE_ADDR task; > > + > > + pid =3D ptid_get_lwp (ptid); > > + task =3D (CORE_ADDR) ptid_get_tid (ptid); > > + > > + xsnprintf (buf, sizeof (buf), "PID: %5li%s, 0x%s", > > + pid, ((lk_task_running (task) !=3D LK_CPU_INVAL) ? "*" : "= "), > > + phex (task, lk_builtin_type_size (unsigned_long))); > > + > > + return buf; > > +} [...] > Nice to have comments for all structs/fields below, a kernel tree > reference may be? I'm not 100% sure what you mean. Is it a comment like "/* Defined in /include/linux/sched.h. */"? If so I don't think its necessary as you could quickly grep for the definition. In particular I am afraid that after a while, when fields get renamed and moved from one file to an other, the comments will confuse more than they help. That's why I haven't added any comments here (besides in w= hat linux version a field is valid). > > + LK_DECLARE_FIELD (task_struct, tasks); > > + LK_DECLARE_FIELD (task_struct, pid); > > + LK_DECLARE_FIELD (task_struct, tgid); > > + LK_DECLARE_FIELD (task_struct, thread_group); > > + LK_DECLARE_FIELD (task_struct, comm); > > + LK_DECLARE_FIELD (task_struct, thread); > > + > > + LK_DECLARE_FIELD (list_head, next); > > + LK_DECLARE_FIELD (list_head, prev); > > + > > + LK_DECLARE_FIELD (rq, curr); > > + > > + LK_DECLARE_FIELD (cpumask, bits); > > + > > + LK_DECLARE_ADDR (init_task); > > + LK_DECLARE_ADDR (runqueues); > > + LK_DECLARE_ADDR (__per_cpu_offset); > > + LK_DECLARE_ADDR (init_mm); > > + > > + LK_DECLARE_ADDR_ALIAS (__cpu_online_mask, cpu_online_mask); /* linux > > 4.5+ */ > > + LK_DECLARE_ADDR_ALIAS (cpu_online_bits, cpu_online_mask); /* linux > > -4.4 */ > > + if (LK_ADDR (cpu_online_mask) =3D=3D -1) > > + error (_("Could not find address cpu_online_mask. Aborting.")); > > +} [...] > > +/* Initialize the cpu to old ptid map. Prefer the arch dependent > > + map_running_task_to_cpu hook if provided, else assume that the PID = used > > + by target beneath is the same as in task_struct PID task_struct. S= ee > > + comment on lk_ptid_map in lk-low.h for details. */ > > + > > +static void > > +lk_init_ptid_map () > > +{ > > + struct thread_info *ti; > > + ULONGEST *cpu_online_mask; > > + size_t size; > > + unsigned int cpu; > > + struct cleanup *old_chain; > > + > > + if (LK_PRIVATE->old_ptid !=3D NULL) > > + lk_free_ptid_map (); > > + > > + size =3D LK_BITMAP_SIZE (cpumask); > > + cpu_online_mask =3D lk_read_bitmap (LK_ADDR (cpu_online_mask), size); > > + old_chain =3D make_cleanup (xfree, cpu_online_mask); > > + > > + ALL_THREADS (ti) > > + { > > + struct lk_ptid_map *ptid_map =3D XCNEW (struct lk_ptid_map); > > + CORE_ADDR rq, curr; > > + int pid; > > + > > + /* Give the architecture a chance to overwrite default behaviour= . */ > > + if (LK_HOOK->map_running_task_to_cpu) > > + { > > + ptid_map->cpu =3D LK_HOOK->map_running_task_to_cpu (ti); > > + } > > + else > > + { > > + LK_BITMAP_FOR_EACH_SET_BIT (cpu_online_mask, size, cpu) > > + { > > + rq =3D LK_ADDR (runqueues) + lk_get_percpu_offset (cpu); > > + curr =3D lk_read_addr (rq + LK_OFFSET (rq, curr)); > > + pid =3D lk_read_int (curr + LK_OFFSET (task_struct, pid)); > > + > > + if (pid =3D=3D ptid_get_lwp (ti->ptid)) > > + { > > + ptid_map->cpu =3D cpu; > > + break; > > + } > > + } > > + if (cpu =3D=3D size) > > + error (_("Could not map thread with pid %d, lwp %lu to a cp= u."), > > + ti->ptid.pid, ti->ptid.lwp);=20=20 >=20 > Accessing pid and lwp fields directly is not recommended. May be use > something like > error (_("Could not map thread with pid %d, lwp %ld to a cpu."), > ptid_get_pid (ti->ptid), ptid_get_lwp (ti->ptid)); Yes, you are right. I did the quick and dirty solution here because I alre= ady knew that this ptid_map "solution" would only be temporary. Besides ptid_t= got classified and it's API changed. So this needs to be adjusted anyway when I rebase to the current master... [...] > > +/* Hook to return the per_cpu_offset of cpu CPU. Only architectures t= hat > > + do not use the __per_cpu_offset array to determin the offset have to > > + supply this hook. */=20=20 >=20 > ^Typo in comment. Thanks, fixed. > Also if its not too much trouble can you kindly put Linux kernel > source tree references like > __per_cpu_offset: Linux/include/asm-generic/percpu.h 4.10. in comments. Like above. I'm not sure if it is worth the effort and if outdated comments confuse more than help. =20 [...] > > +/* Divides numinator N by demoniator D and rounds up the result. */= =20=20 >=20 > ^ Spell check above. What the hell did I do here? Thanks for the hint, fixed. =20 Hope I didn't miss anything. Thanks Philipp