From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 54347 invoked by alias); 3 May 2017 16:16:29 -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 54328 invoked by uid 89); 3 May 2017 16:16:28 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.8 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 spammy=barely, insist X-HELO: mx0a-001b2d01.pphosted.com Received: from mx0a-001b2d01.pphosted.com (HELO mx0a-001b2d01.pphosted.com) (148.163.156.1) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 03 May 2017 16:16:25 +0000 Received: from pps.filterd (m0098396.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.20/8.16.0.20) with SMTP id v43GEQ9p016087 for ; Wed, 3 May 2017 12:16:26 -0400 Received: from e06smtp14.uk.ibm.com (e06smtp14.uk.ibm.com [195.75.94.110]) by mx0a-001b2d01.pphosted.com with ESMTP id 2a7j8210bv-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Wed, 03 May 2017 12:16:26 -0400 Received: from localhost by e06smtp14.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 3 May 2017 17:16:24 +0100 Received: from b06cxnps4076.portsmouth.uk.ibm.com (9.149.109.198) by e06smtp14.uk.ibm.com (192.168.101.144) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Wed, 3 May 2017 17:16:22 +0100 Received: from d06av24.portsmouth.uk.ibm.com (mk.ibm.com [9.149.105.60]) by b06cxnps4076.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id v43GGMW839452908; Wed, 3 May 2017 16:16:22 GMT Received: from d06av24.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 8B6F242047; Wed, 3 May 2017 17:15:02 +0100 (BST) Received: from d06av24.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 529B34203F; Wed, 3 May 2017 17:15:02 +0100 (BST) Received: from ThinkPad (unknown [9.152.212.148]) by d06av24.portsmouth.uk.ibm.com (Postfix) with ESMTP; Wed, 3 May 2017 17:15:02 +0100 (BST) Date: Wed, 03 May 2017 16:16:00 -0000 From: Philipp Rudo To: Yao Qi Cc: gdb-patches@sourceware.org, Yao Qi , Peter Griffin , Omair Javaid , Andreas Arnez Subject: Re: [RFC v3 4/8] Add kernel module support for linux-kernel target In-Reply-To: <868tmf8kn4.fsf@gmail.com> References: <20170316165739.88524-1-prudo@linux.vnet.ibm.com> <20170316165739.88524-5-prudo@linux.vnet.ibm.com> <868tmf8kn4.fsf@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 x-cbid: 17050316-0016-0000-0000-00000490967E X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17050316-0017-0000-0000-000027814A0A Message-Id: <20170503181620.53d0bd3f@ThinkPad> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-05-03_13:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=2 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1703280000 definitions=main-1705030297 X-IsSubscribed: yes X-SW-Source: 2017-05/txt/msg00063.txt.bz2 Hi, On Tue, 02 May 2017 14:15:43 +0100 Yao Qi wrote: > Philipp Rudo writes: > > > +/* Translate a kernel virtual address ADDR to a physical address. */ > > + > > +CORE_ADDR > > +lk_kvtop (CORE_ADDR addr) > > How about lk_kernel_vir_to_phy_addr? I prefer kvtop. It's much shorter and (for my taste) is just as readable. But I don't insist on keeping the name. Are there other opinions? > > > +{ > > + CORE_ADDR pgd = lk_read_addr (LK_ADDR (init_mm) > > + + LK_OFFSET (mm_struct, pgd)); > > + return LK_HOOK->vtop (pgd, addr); > > +} > > + > > +/* Restore current_target to TARGET. */ > > +static void > > +restore_current_target (void *target) > > +{ > > + current_target.beneath = (struct target_ops *) target; > > +} > > + > > +/* Function for targets to_xfer_partial hook. */ > > + > > +enum target_xfer_status > > +lk_xfer_partial (struct target_ops *ops, enum target_object object, > > + const char *annex, gdb_byte *readbuf, > > + const gdb_byte *writebuf, ULONGEST offset, ULONGEST len, > > + ULONGEST *xfered_len) > > +{ > > + enum target_xfer_status ret_val; > > + struct cleanup *old_chain = make_cleanup (restore_current_target, > > ops); > > Use make_scoped_restore instead of make_cleanup? Using a scoped_restore probably makes sense. Although I don't see the advantage over old style cleanups other than having marginally shorter code ... > > + > > + current_target.beneath = ops->beneath; > > + > > Any reasons you switch current_target.beneath temporarily? Yes. lk_kvtop (at least for s390) reads memory if the address is not physical. Thus reading a virtual address calls xfer_partial twice. Once for the actual address and a second time for the data lk_kvtop needs. This can lead to an endless recursion if there is a bug or memory corruption. Switching to the target beneath prevents this. > > + if (LK_HOOK->is_kvaddr (offset)) > > + offset = lk_kvtop (offset); > > + > > + ret_val = ops->beneath->to_xfer_partial (ops->beneath, object, annex, > > + readbuf, writebuf, offset, len, > > + xfered_len); > > Two spaces after "=". Fixed, thanks. > > diff --git a/gdb/lk-modules.c b/gdb/lk-modules.c > > new file mode 100644 > > index 0000000..f3c559d > > --- /dev/null > > +++ b/gdb/lk-modules.c > > @@ -0,0 +1,412 @@ > > +/* Handle Linux kernel modules as shared libraries. > > + > > + Copyright (C) 2016 Free Software Foundation, Inc. > > + > > + This file is part of GDB. > > + > > + This program is free software; you can redistribute it and/or modify > > + it under the terms of the GNU General Public License as published by > > + the Free Software Foundation; either version 3 of the License, or > > + (at your option) any later version. > > + > > + This program is distributed in the hope that it will be useful, > > + but WITHOUT ANY WARRANTY; without even the implied warranty of > > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + GNU General Public License for more details. > > + > > + You should have received a copy of the GNU General Public License > > + along with this program. If not, see . > > */ + > > +#include "defs.h" > > + > > +#include "common/filestuff.h" > > +#include "filenames.h" > > +#include "gdbcmd.h" > > +#include "gdbcore.h" > > +#include "gdb_regex.h" > > +#include "lk-lists.h" > > +#include "lk-low.h" > > +#include "lk-modules.h" > > +#include "objfiles.h" > > +#include "observer.h" > > +#include "readline/readline.h" > > +#include "solib.h" > > +#include "solist.h" > > +#include "utils.h" > > + > > +#include > > +#include > > + > > +struct target_so_ops *lk_modules_so_ops = NULL; > > + > > +/* Info for single section type. */ > > + > > +struct lm_info_sec > > +{ > > + CORE_ADDR start; > > + CORE_ADDR offset; > > + unsigned int size; > > +}; > > + > > +/* Link map info to include in an allocated so_list entry. */ > > + > > +struct lm_info > > +{ > > + CORE_ADDR base; > > + unsigned int size; > > + > > + struct lm_info_sec text; > > + struct lm_info_sec init_text; > > + struct lm_info_sec ro_data; > > + struct lm_info_sec rw_data; > > + struct lm_info_sec percpu; > > +}; > > Comments to these fields are needed. Yes, you are right. I will fix it when I adjust to the classification of lm_info. > > + > > +/* Build map between module name and path to binary file by reading file > > + modules.order. Returns unordered_map with module name as key and its > > + path as value. */ > > + > > +std::unordered_map > > +lk_modules_build_path_map () > > +{ > > + std::unordered_map umap; > > + FILE *mod_order; > > + struct cleanup *old_chain; > > + char line[SO_NAME_MAX_PATH_SIZE + 1]; > > + > > + mod_order = lk_modules_open_mod_order (); > > + old_chain = make_cleanup_fclose (mod_order); > > + > > + line[SO_NAME_MAX_PATH_SIZE] = '\0'; > > + std::string search_path = lk_modules_expand_search_path (); > > + while (fgets (line, SO_NAME_MAX_PATH_SIZE, mod_order)) > > + { > > + /* Remove trailing newline. */ > > + line[strlen (line) - 1] = '\0'; > > + > > + std::string name = lbasename (line); > > + > > + /* 3 = strlen (".ko"). */ > > + if (!endswith (name.c_str (), ".ko") > > + || name.length () >= LK_MODULE_NAME_LEN + 3) > > + continue; > > + > > + name = name.substr (0, name.length () - 3); > > + > > + /* Kernel modules are named after the files they are stored in with > > + all minus '-' replaced by underscore '_'. Do the same to enable > > + mapping. */ > > + for (size_t p = name.find('-'); p != std::string::npos; > > + p = name.find ('-', p + 1)) > > + name[p] = '_'; > > + > > + umap[name] = concat_path(search_path, line); > > + } > > + > > + do_cleanups (old_chain); > > + return umap; > > +} > > + > > +/* Allocate and fill a copy of struct lm_info for module at address MOD. > > */ + > > +struct lm_info * > > +lk_modules_read_lm_info (CORE_ADDR mod) > > +{ > > + struct lm_info *lmi = XNEW (struct lm_info); > > + struct cleanup *old_chain = make_cleanup (xfree, lmi); > > + > > use std::unique_ptr to avoid cleanup. > > sdt::unique_ptr lmi (new lm_info ()); Yes, a unique_ptr is better. There are probably many cleanups I can get rid off with the latest C++-yfication. The only problem I currently have is that the C++-yfication rate is too high I can barely follow and add new features to have a reliable kernel debugger. Thanks for the hint anyway. > > + if (LK_FIELD (module, module_core)) /* linux -4.4 */ > > + { > > + lmi->base = lk_read_addr (mod + LK_OFFSET (module, module_core)); > > + lmi->size = lk_read_addr (mod + LK_OFFSET (module, core_size)); > > + > > + lmi->text.start = lmi->base; > > + lmi->text.offset = LK_HOOK->get_module_text_offset (mod); > > + lmi->text.size = lk_read_uint (mod + LK_OFFSET (module, > > core_text_size)); + > > + lmi->ro_data.start = lmi->base + lmi->text.size; > > + lmi->ro_data.offset = 0; > > + lmi->ro_data.size = lk_read_uint (mod + LK_OFFSET (module, > > + core_ro_size)); > > + } > > + else /* linux 4.5+ */ > > + { > > + CORE_ADDR mod_core = mod + LK_OFFSET (module, core_layout); > > + > > + lmi->base = lk_read_addr (mod_core > > + + LK_OFFSET (module_layout, base)); > > + lmi->size = lk_read_uint (mod_core > > + + LK_OFFSET (module_layout, size)); > > + > > + lmi->text.start = lmi->base; > > + lmi->text.offset = LK_HOOK->get_module_text_offset (mod); > > + lmi->text.size = lk_read_uint (mod_core > > + + LK_OFFSET (module_layout, > > text_size)); + > > + lmi->ro_data.start = lmi->base + lmi->text.size; > > + lmi->ro_data.offset = 0; > > + lmi->ro_data.size = lk_read_uint (mod_core > > + + LK_OFFSET (module_layout, > > ro_size)); > > + } > > + > > + lmi->rw_data.start = lmi->base + lmi->ro_data.size; > > + lmi->rw_data.offset = 0; > > + lmi->rw_data.size = lmi->size - lmi->ro_data.size; > > + > > + lmi->init_text.start = lk_read_addr (mod + LK_OFFSET (module, init)); > > + lmi->init_text.offset = 0; > > + > > + lmi->percpu.start = lk_read_addr (mod + LK_OFFSET (module, percpu)); > > + lmi->percpu.size = lk_read_uint (mod + LK_OFFSET (module, percpu_size)); > > + lmi->percpu.offset = 0; > > + > > + discard_cleanups (old_chain); > > + return lmi; > > +} > > + >