From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15007 invoked by alias); 25 Nov 2013 21:11:36 -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 14996 invoked by uid 89); 25 Nov 2013 21:11:35 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL,BAYES_40,RDNS_NONE,SPF_HELO_PASS,SPF_PASS autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from Unknown (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 25 Nov 2013 21:11:34 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rAPLBN3d013105 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 25 Nov 2013 16:11:24 -0500 Received: from psique (ovpn-113-74.phx2.redhat.com [10.3.113.74]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id rAPLBKXt015276 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Mon, 25 Nov 2013 16:11:22 -0500 From: Sergio Durigan Junior To: Marcus Shawcroft Cc: GDB Patches Subject: Re: [PATCH] Fix for PR tdep/15653: Implement SystemTap SDT probe support for AArch64 References: <1385336092-19621-1-git-send-email-sergiodj@redhat.com> <52931DB7.6030205@arm.com> X-URL: http://www.redhat.com Date: Mon, 25 Nov 2013 23:55:00 -0000 In-Reply-To: <52931DB7.6030205@arm.com> (Marcus Shawcroft's message of "Mon, 25 Nov 2013 09:51:51 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2013-11/txt/msg00794.txt.bz2 Hey, On Monday, November 25 2013, Marcus Shawcroft wrote: > On 24/11/13 23:34, Sergio Durigan Junior wrote: >> This commit implements the needed bits for SystemTap SDT probe support >> on AArch64 architectures. The core of the patch is in the >> aarch64-linux-tdep.c file; all the rest are support/cleanup >> modifications. >> >> First, I started by looking at AArch64 assembly specification and >> filling the necessary options on gdbarch's stap machinery in order to >> make the generic asm parser (implemented in stap-probe.c) recognize >> AArch64's asm. >> >> AArch64 has the same idiosincrasy as 32-bit ARM: the syntax for >> register indirection is very specific, and demands its own function to >> parse this special expression. However, both AArch64 and 32-bit ARM >> share the same syntax (with only one little difference between them), > > A64 assembler syntax is similar to A32 and T32 in some areas and > differs somewhat in others. The syntax used specifically for indirect > register offset addressing is very similar. It might make sense in > some circumstances to share code that encapsulates u-architectural > details that underpin an a64/a32/t32 implementation, but, building a > shared assembler syntax parser in the fashion proposed could be > described as coincidental cohesion. Is that desirable going forward? I am not sure I completely understood your comment. Are you criticizing the shared asm parser for ARM targets only, or the generic asm parser in stap-probe.c? I assume it is the former. And I also fail to see why it can be considered coincidental cohesion... The shared parser covers one specific ARM assembly syntax, which is pretty similar to both 32- and 64-bit ARM. Correct me if I'm wrong, but you're actually against the way I've chosen to share the code between the targets, right? Or am I missing something here. If that's the case, I can try to come up with a simpler/cleaner way to share this code... Suggestions are appreciated, of course :-). >> which made me think of a way to share the specific parser between both > >> +/* Implementation of gdbarch_stap_is_single_operand. */ >> + >> +static int >> +aarch64_stap_is_single_operand (struct gdbarch *gdbarch, const char *s) >> +{ >> + return (*s == '#' /* Literal number. */ > > The '#' before a literal is optional, therefore this clause ought to > permit isdigit()? Thanks, I wasn't aware that it is optional. I will fix the code. >> + || *s == '[' /* Register indirection or displacement. */ >> + || isalpha (*s)); /* Register value. */ >> +} >> + >> +/* Implementation of gdbarch_stap_parse_special_token. */ >> + >> +static int >> +aarch64_stap_parse_special_token (struct gdbarch *gdbarch, >> + struct stap_parse_info *p) >> +{ >> + return arm_generic_stap_parse_special_token (gdbarch, p, 0); >> +} >> > > Looking over in arm-linux-tdep.c:arm_generic_stap_parse_special_token(): > > """ > /* If we are dealing with a register whose name begins with a > digit, it means we should prefix the name with the letter > `r', because GDB expects this name pattern. Otherwise (e.g., > we are dealing with the register `fp'), we don't need to > add such a prefix. */ > > """ > > Does this really apply to a64 where there are no register names that > begin with 'r'? No, it doesn't apply, but then, AArch64 registers' names don't begin with a digit, so this case is never triggered. However, I agree that the comment should be expanded in order to explain why it doesn't apply. >> +int >> +arm_generic_stap_parse_special_token (struct gdbarch *gdbarch, >> + struct stap_parse_info *p, >> + int skip_hash_sign) >> { >> if (*p->arg == '[') >> { >> @@ -1183,7 +1177,7 @@ arm_stap_parse_special_token (struct gdbarch *gdbarch, >> >> ++tmp; >> tmp = skip_spaces_const (tmp); >> - if (*tmp++ != '#') >> + if (skip_hash_sign && *tmp++ != '#') > > The '#' at this point is optional in a64. OK, thanks. >> @@ -59,6 +61,24 @@ void arm_linux_collect_nwfpe (const struct regset *regset, >> const struct regcache *regcache, >> int regnum, void *regs_buf, size_t len); >> >> +/* This routine is used to parse a special token in ARM's assembly. >> + It works for both 32-bit and 64-bit ARM architectures. >> + >> + The special tokens parsed by it are: >> + >> + - Register displacement (e.g, [fp, #-8] on ARM, and [fp, -8] on AArch64) >> + > > A # is optional in a64, therefore [fp, #-8] is legal. Aha, nice to know. I will expand the code to take that into consideration as well. Thanks for the review. I will post a refreshed patch soon. -- Sergio