From patchwork Tue Mar 26 13:17:02 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Alan Hayward X-Patchwork-Id: 31984 Received: (qmail 20607 invoked by alias); 26 Mar 2019 13:17:09 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 20591 invoked by uid 89); 26 Mar 2019 13:17:08 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.3 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, MIME_BASE64_BLANKS, RCVD_IN_DNSWL_NONE, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.1 spammy= X-HELO: EUR03-AM5-obe.outbound.protection.outlook.com Received: from mail-eopbgr30066.outbound.protection.outlook.com (HELO EUR03-AM5-obe.outbound.protection.outlook.com) (40.107.3.66) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 26 Mar 2019 13:17:06 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=armh.onmicrosoft.com; s=selector1-arm-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=eNws3Ch5nvLRTohrXXREyJN4kk/SrpxTwbKsO0KIlis=; b=XfwmihcKtNrlttTcp21ab0AwaFzsee2htSpk+s2CA563/5nff4lhCnIuLddCLPZ1gN9CgpUU8Fn9kZTU710AFHIn19CBMjv9rUZL3F3s2RQbseqm0bGmT8lHxnoH9PhPWN0OKzuGRzoYLIyKSBpA89icAXyMgqwn1yKAvcGFaYo= Received: from DB6PR0802MB2133.eurprd08.prod.outlook.com (10.172.227.22) by DB6PR0802MB2600.eurprd08.prod.outlook.com (10.172.251.150) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1730.18; Tue, 26 Mar 2019 13:17:02 +0000 Received: from DB6PR0802MB2133.eurprd08.prod.outlook.com ([fe80::2083:2d62:84fa:a547]) by DB6PR0802MB2133.eurprd08.prod.outlook.com ([fe80::2083:2d62:84fa:a547%3]) with mapi id 15.20.1730.019; Tue, 26 Mar 2019 13:17:02 +0000 From: Alan Hayward To: Simon Marchi CC: "gdb-patches@sourceware.org" , nd Subject: Re: [PATCH 2/2] gdbserver: Add linux_get_hwcap Date: Tue, 26 Mar 2019 13:17:02 +0000 Message-ID: References: <20190325120542.92123-1-alan.hayward@arm.com> <20190325120542.92123-2-alan.hayward@arm.com> In-Reply-To: authentication-results: spf=none (sender IP is ) smtp.mailfrom=Alan.Hayward@arm.com; received-spf: None (protection.outlook.com: arm.com does not designate permitted sender hosts) x-ms-exchange-senderadcheck: 1 Content-ID: <9A56660824ED704B82A4A58EC9E5DFC0@eurprd08.prod.outlook.com> MIME-Version: 1.0 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-IsSubscribed: yes > On 25 Mar 2019, at 15:41, Simon Marchi wrote: > > On 2019-03-25 8:05 a.m., Alan Hayward wrote: >> In gdbserver, Tidy up calls to read HWCAP (and HWCAP2) by adding common >> functions, removing the Arm, AArch64, PPC and S390 specific versions. >> No functionality differences. >> [ I wasn't sure in gdbserver when to use CORE_ADDR and when to use int/long. >> I'm assuming CORE_ADDR is fairly recent to gdbserver? ] > > I don't know if CORE_ADDR is a recent addition to gdbserver. But I suppose CORE_ADDR was chosen as the return type for functions reading arbitrary AUXV values, since some of them may be pointers. With CORE_ADDR, we know those values will fit in the data type. When we return the HWCAP value, we know it won't be a pointer though, so returning a CORE_ADDR is a bit confusing, IMO. Those functions returning the HWCAP value could return something else, an uint64_t maybe. But then I would change it in the gdb version as well to match. I’ve been flipping back and too in my head to change CORE_ADDR to uint64_t. On a 32bit build, CORE_ADDR is 32bits. So it would be making a difference. Not sure if that really matters or not? Left this change out of the update below for now. > >> /* Implementation of linux_target_ops method "arch_setup". */ >> static void >> @@ -545,8 +521,8 @@ aarch64_arch_setup (void) >> if (is_elf64) >> { >> uint64_t vq = aarch64_sve_get_vq (tid); >> - unsigned long hwcap = 0; >> - bool pauth_p = aarch64_get_hwcap (&hwcap) && (hwcap & AARCH64_HWCAP_PACA); >> + unsigned long hwcap = linux_get_hwcap (8); >> + bool pauth_p = hwcap & AARCH64_HWCAP_PACA; > > Just wondering, can the linux-aarch64-low.c code be used to debug a process "gdbserver :2345 a.out” works on an AArch64 box if that’s what you're asking? The code above is used to detect point auth. > >> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c >> index 6f703f589f..481919c205 100644 >> --- a/gdb/gdbserver/linux-low.c >> +++ b/gdb/gdbserver/linux-low.c >> @@ -7423,6 +7423,64 @@ linux_get_pc_64bit (struct regcache *regcache) >> return pc; >> } >> +/* Extract the auxiliary vector entry with a_type matching MATCH, storing the >> + value in VALP and returning true. If no entry was found, return false. */ >> + >> +static bool >> +linux_get_auxv (int wordsize, CORE_ADDR match, CORE_ADDR *valp) > > I think this function could return the result (CORE_ADDR) directly, > returning 0 on failure. Done. Agreed that given that this function is now static, there is really no need to keep the unused error. With this changed, I almost changed linux_get_hwcap and linux_get_hwcap2 into inline functions in the header. But that means adding extra includes into the header (for the AT_HWCAP defines) and reliving the static from linux_get_auxv. > > If 4 and 8 are the only supported wordsize values, I would suggest adding an assert to verify it. Done. > >> +{ >> + gdb_byte *data = (gdb_byte *) alloca (2 * wordsize); >> + int offset = 0; >> + >> + while ((*the_target->read_auxv) (offset, data, 2 * wordsize) == 2 * wordsize) >> + { >> + if (wordsize == 4) >> + { >> + unsigned int *data_p = (unsigned int *)data; >> + if (data_p[0] == match) >> + { >> + *valp = data_p[1]; >> + return true; >> + } >> + } >> + else >> + { >> + unsigned long *data_p = (unsigned long *)data; >> + if (data_p[0] == match) >> + { >> + *valp = data_p[1]; >> + return true; >> + } >> + } > > I am a bit worried about relying on the size of the "int" and "long" types in architecture-independent code. Could we use uint32_t and uint64_t instead? > Done. > Simon Updated patch below. Will push if you have no more comments. Alan. diff --git a/gdb/gdbserver/linux-aarch64-low.c b/gdb/gdbserver/linux-aarch64-low.c index 20c75493b0..dc4ee81d2a 100644 --- a/gdb/gdbserver/linux-aarch64-low.c +++ b/gdb/gdbserver/linux-aarch64-low.c @@ -505,30 +505,6 @@ aarch64_linux_new_fork (struct process_info *parent, /* Matches HWCAP_PACA in kernel header arch/arm64/include/uapi/asm/hwcap.h. */ #define AARCH64_HWCAP_PACA (1 << 30) -/* Fetch the AT_HWCAP entry from the auxv vector. */ - -static bool -aarch64_get_hwcap (unsigned long *valp) -{ - unsigned char *data = (unsigned char *) alloca (16); - int offset = 0; - - while ((*the_target->read_auxv) (offset, data, 16) == 16) - { - unsigned long *data_p = (unsigned long *)data; - if (data_p[0] == AT_HWCAP) - { - *valp = data_p[1]; - return true; - } - - offset += 16; - } - - *valp = 0; - return false; -} - /* Implementation of linux_target_ops method "arch_setup". */ static void @@ -545,8 +521,8 @@ aarch64_arch_setup (void) if (is_elf64) { uint64_t vq = aarch64_sve_get_vq (tid); - unsigned long hwcap = 0; - bool pauth_p = aarch64_get_hwcap (&hwcap) && (hwcap & AARCH64_HWCAP_PACA); + unsigned long hwcap = linux_get_hwcap (8); + bool pauth_p = hwcap & AARCH64_HWCAP_PACA; current_process ()->tdesc = aarch64_linux_read_description (vq, pauth_p); } diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c index 8cad5c5fd4..ff72a489cb 100644 --- a/gdb/gdbserver/linux-arm-low.c +++ b/gdb/gdbserver/linux-arm-low.c @@ -847,40 +847,15 @@ get_next_pcs_syscall_next_pc (struct arm_get_next_pcs *self) return next_pc; } -static int -arm_get_hwcap (unsigned long *valp) -{ - unsigned char *data = (unsigned char *) alloca (8); - int offset = 0; - - while ((*the_target->read_auxv) (offset, data, 8) == 8) - { - unsigned int *data_p = (unsigned int *)data; - if (data_p[0] == AT_HWCAP) - { - *valp = data_p[1]; - return 1; - } - - offset += 8; - } - - *valp = 0; - return 0; -} - static const struct target_desc * arm_read_description (void) { int pid = lwpid_of (current_thread); - unsigned long arm_hwcap = 0; + unsigned long arm_hwcap = linux_get_hwcap (4); /* Query hardware watchpoint/breakpoint capabilities. */ arm_linux_init_hwbp_cap (pid); - if (arm_get_hwcap (&arm_hwcap) == 0) - return tdesc_arm; - if (arm_hwcap & HWCAP_IWMMXT) return tdesc_arm_with_iwmmxt; diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c index 6f703f589f..7158a6798c 100644 --- a/gdb/gdbserver/linux-low.c +++ b/gdb/gdbserver/linux-low.c @@ -7423,6 +7423,53 @@ linux_get_pc_64bit (struct regcache *regcache) return pc; } +/* Fetch the entry MATCH from the auxv vector, where entries are length + WORDSIZE. If no entry was found, return zero. */ + +static CORE_ADDR +linux_get_auxv (int wordsize, CORE_ADDR match) +{ + gdb_byte *data = (gdb_byte *) alloca (2 * wordsize); + int offset = 0; @@ -7423,6 +7423,53 @@ linux_get_pc_64bit (struct regcache *regcache) return pc; } +/* Fetch the entry MATCH from the auxv vector, where entries are length + WORDSIZE. If no entry was found, return zero. */ + +static CORE_ADDR +linux_get_auxv (int wordsize, CORE_ADDR match) +{ + gdb_byte *data = (gdb_byte *) alloca (2 * wordsize); + int offset = 0; + + gdb_assert (wordsize == 4 || wordsize == 8); + + while ((*the_target->read_auxv) (offset, data, 2 * wordsize) == 2 * wordsize) + { + if (wordsize == 4) + { + uint32_t *data_p = (uint32_t *)data; + if (data_p[0] == match) + return data_p[1]; + } + else + { + uint64_t *data_p = (uint64_t *)data; + if (data_p[0] == match) + return data_p[1]; + } + + offset += 2 * wordsize; + } + + return 0; +} + +/* See linux-low.h. */ + +CORE_ADDR +linux_get_hwcap (int wordsize) +{ + return linux_get_auxv (wordsize, AT_HWCAP); +} + +/* See linux-low.h. */ + +CORE_ADDR +linux_get_hwcap2 (int wordsize) +{ + return linux_get_auxv (wordsize, AT_HWCAP2); +} static struct target_ops linux_target_ops = { linux_create_inferior, diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h index 1ade35d648..d825184835 100644 --- a/gdb/gdbserver/linux-low.h +++ b/gdb/gdbserver/linux-low.h @@ -435,4 +435,14 @@ bool thread_db_thread_handle (ptid_t ptid, gdb_byte **handle, int *handle_len); extern int have_ptrace_getregset; +/* Fetch the AT_HWCAP entry from the auxv vector, where entries are length + WORDSIZE. If no entry was found, return zero. */ + +CORE_ADDR linux_get_hwcap (int wordsize); + +/* Fetch the AT_HWCAP2 entry from the auxv vector, where entries are length + WORDSIZE. If no entry was found, return zero. */ + +CORE_ADDR linux_get_hwcap2 (int wordsize); + #endif /* GDBSERVER_LINUX_LOW_H */ diff --git a/gdb/gdbserver/linux-ppc-low.c b/gdb/gdbserver/linux-ppc-low.c index 1b695e53fe..8deb0ce068 100644 --- a/gdb/gdbserver/linux-ppc-low.c +++ b/gdb/gdbserver/linux-ppc-low.c @@ -323,43 +323,6 @@ ppc_set_pc (struct regcache *regcache, CORE_ADDR pc) } } - -static int -ppc_get_auxv (unsigned long type, unsigned long *valp) -{ - const struct target_desc *tdesc = current_process ()->tdesc; - int wordsize = register_size (tdesc, 0); - unsigned char *data = (unsigned char *) alloca (2 * wordsize); - int offset = 0; - - while ((*the_target->read_auxv) (offset, data, 2 * wordsize) == 2 * wordsize) - { - if (wordsize == 4) - { - unsigned int *data_p = (unsigned int *)data; - if (data_p[0] == type) - { - *valp = data_p[1]; - return 1; - } - } - else - { - unsigned long *data_p = (unsigned long *)data; - if (data_p[0] == type) - { - *valp = data_p[1]; - return 1; - } - } - - offset += 2 * wordsize; - } - - *valp = 0; - return 0; -} - #ifndef __powerpc64__ static int ppc_regmap_adjusted; #endif @@ -944,8 +907,8 @@ ppc_arch_setup (void) /* The value of current_process ()->tdesc needs to be set for this call. */ - ppc_get_auxv (AT_HWCAP, &ppc_hwcap); - ppc_get_auxv (AT_HWCAP2, &ppc_hwcap2); + ppc_hwcap = linux_get_hwcap (features.wordsize); + ppc_hwcap2 = linux_get_hwcap2 (features.wordsize); features.isa205 = ppc_linux_has_isa205 (ppc_hwcap); diff --git a/gdb/gdbserver/linux-s390-low.c b/gdb/gdbserver/linux-s390-low.c index edbef77fe9..f65a1ec38e 100644 --- a/gdb/gdbserver/linux-s390-low.c +++ b/gdb/gdbserver/linux-s390-low.c @@ -467,36 +467,6 @@ s390_set_pc (struct regcache *regcache, CORE_ADDR newpc) } } -/* Get HWCAP from AUXV, using the given WORDSIZE. Return the HWCAP, or - zero if not found. */ - -static unsigned long -s390_get_hwcap (int wordsize) -{ - gdb_byte *data = (gdb_byte *) alloca (2 * wordsize); - int offset = 0; - - while ((*the_target->read_auxv) (offset, data, 2 * wordsize) == 2 * wordsize) - { - if (wordsize == 4) - { - unsigned int *data_p = (unsigned int *)data; - if (data_p[0] == AT_HWCAP) - return data_p[1]; - } - else - { - unsigned long *data_p = (unsigned long *)data; - if (data_p[0] == AT_HWCAP) - return data_p[1]; - } - - offset += 2 * wordsize; - } - - return 0; -} - /* Determine the word size for the given PID, in bytes. */ #ifdef __s390x__ @@ -548,7 +518,7 @@ s390_arch_setup (void) /* Determine word size and HWCAP. */ int pid = pid_of (current_thread); int wordsize = s390_get_wordsize (pid); - unsigned long hwcap = s390_get_hwcap (wordsize); + unsigned long hwcap = linux_get_hwcap (wordsize); /* Check whether the kernel supports extra register sets. */ int have_regset_last_break