From patchwork Tue Jul 15 13:01:22 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 2057 Received: (qmail 16558 invoked by alias); 15 Jul 2014 13:01:36 -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 16498 invoked by uid 89); 15 Jul 2014 13:01:35 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD, SPF_HELO_PASS, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 15 Jul 2014 13:01:31 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s6FD1Sk9021923 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 15 Jul 2014 09:01:28 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s6FD1NJB011955; Tue, 15 Jul 2014 09:01:25 -0400 Message-ID: <53C52622.3000405@redhat.com> Date: Tue, 15 Jul 2014 14:01:22 +0100 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 MIME-Version: 1.0 To: Yao Qi , gdb-patches@sourceware.org Subject: Re: [PATCH] Tweak gdb.trace/tfile.c for thumb mode References: <1404100222-2312-1-git-send-email-yao@codesourcery.com> <53BD5710.5040105@redhat.com> <53BDEBD8.5030201@codesourcery.com> In-Reply-To: <53BDEBD8.5030201@codesourcery.com> On 07/10/2014 02:26 AM, Yao Qi wrote: > On 07/09/2014 10:52 PM, Pedro Alves wrote: >> #if defined(__thumb__) || defined(__thumb2__) >> /* Although Thumb functions are two-byte aligned, function >> pointers have the Thumb bit set. Clear it. */ >> func_addr &= ~1; >> #endif >> >> (This bit is widely known as the "Thumb bit", so call it that, >> and remove a few "the"'s that sound odd to me, and say >> "two-byte aligned".) > > I often associate "Thumb bit" with the bit in CPSR. Look into > arm-tdep.c and arm-linux-tdep.c, "Thumb bit" is used for both cases > (LSB of function address and the bit in CPSR). It shouldn't cause any > confusion. > > Patch is updated as you suggested and pushed in. > Thanks. The use of 'long' for holding an address made me take another look. I thought the test must be failing to build on Win64 due to warnings, but it turns out warnings are being suppressed currently. I took a stab at fixing it. While doing that I noticed that write_error_trace_file needs the Thumb treatment too? WDYT of the patch below? I don't think we need to worry about targets that don't have stdint.h in this test and at this day and age. 8<------------------------------- From fbd5a81a0cb775c3bc7798a0d2510321c41bcc53 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Tue, 15 Jul 2014 13:44:16 +0100 Subject: [PATCH] gdb.trace/tfile.c: Remove Thumb bit in one more more, general cleanup I noticed that the existing code casts a function's address to 'long', but that doesn't work correctly on some ABIs, like Win64, where long is 32-bit and while pointers are 64-bit: func_addr = (long) &write_basic_trace_file; Fixing that showed there's actually another place in the file that writes a function address to file, and therefore should clear the Thumb bit. This commit adds a macro+function pair to centralize the Thumb bit handling, and uses it in both places. The rest is just enough changes to make the file build without warnings with "-Wall -Wextra" with x86_64-w64-mingw32-gcc and i686-w64-mingw32-gcc cross compilers, and with -m32/-m64 on x86_64 GNU/Linux. Currently with x86_64-w64-mingw32-gcc we get: $ x86_64-w64-mingw32-gcc tfile.c -Wall -DTFILE_DIR=\"\" tfile.c: In function 'start_trace_file': tfile.c:51:23: error: 'S_IRGRP' undeclared (first use in this function) S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH); ^ tfile.c:51:23: note: each undeclared identifier is reported only once for each function it appears in tfile.c:51:31: error: 'S_IROTH' undeclared (first use in this function) S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH); ^ tfile.c: In function 'add_memory_block': tfile.c:79:10: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] ll_x = (unsigned long) addr; ^ tfile.c: In function 'write_basic_trace_file': tfile.c:113:15: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] func_addr = (long) &write_basic_trace_file; ^ tfile.c:137:3: warning: passing argument 1 of 'add_memory_block' from incompatible pointer type [enabled by default] add_memory_block (&testglob, sizeof (testglob)); ^ tfile.c:72:1: note: expected 'char *' but argument is of type 'int *' add_memory_block (char *addr, int size) ^ tfile.c:139:3: warning: passing argument 1 of 'add_memory_block' from incompatible pointer type [enabled by default] add_memory_block (&testglob2, 1); ^ tfile.c:72:1: note: expected 'char *' but argument is of type 'int *' add_memory_block (char *addr, int size) ^ tfile.c: In function 'write_error_trace_file': tfile.c:185:3: warning: implicit declaration of function 'alloca' [-Wimplicit-function-declaration] char *hex = alloca (len * 2 + 1); ^ tfile.c:185:15: warning: incompatible implicit declaration of built-in function 'alloca' [enabled by default] char *hex = alloca (len * 2 + 1); ^ tfile.c:211:6: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast] (long) &write_basic_trace_file); ^ The test still passes for me on x86_64 Fedora 20. gdb/testsuite/ 2014-07-15 Pedro Alves * gdb.trace/tfile.c: Include unistd.h and stdint.h. (start_trace_file): Guard S_IRGRP and S_IROTH uses behind #ifdef. (tfile_write_64, tfile_write_16, tfile_write_8, tfile_write_addr) (tfile_write_buf): New functions. (add_memory_block): Rewrite using the above. (adjust_function_address): New function. (FUNCTION_ADDRESS): New macro. (write_basic_trace_file): Remove short_x local, and use tfile_write_16. Change type of func_addr local to unsigned long long. Use FUNCTION_ADDRESS instead of handling the Thumb bit here. Cast argument of add_memory_block to char pointer. (write_error_trace_file): Avoid alloca. Use FUNCTION_ADDRESS. (main): Remove parameters. * gdb.trace/tfile.exp: Remove nowarnings. --- gdb/testsuite/gdb.trace/tfile.c | 113 ++++++++++++++++++++++++++------------ gdb/testsuite/gdb.trace/tfile.exp | 2 +- 2 files changed, 80 insertions(+), 35 deletions(-) diff --git a/gdb/testsuite/gdb.trace/tfile.c b/gdb/testsuite/gdb.trace/tfile.c index bc25b80..9702afa 100644 --- a/gdb/testsuite/gdb.trace/tfile.c +++ b/gdb/testsuite/gdb.trace/tfile.c @@ -20,9 +20,11 @@ GDB. */ #include +#include #include #include #include +#include char spbuf[200]; @@ -46,9 +48,17 @@ int start_trace_file (char *filename) { int fd; + mode_t mode = S_IRUSR | S_IWUSR; - fd = open (filename, O_WRONLY|O_CREAT|O_APPEND, - S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH); +#ifdef S_IRGRP + mode |= S_IRGRP; +#endif + +#ifdef S_IROTH + mode |= S_IROTH; +#endif + + fd = open (filename, O_WRONLY|O_CREAT|O_APPEND, mode); if (fd < 0) return fd; @@ -67,31 +77,73 @@ finish_trace_file (int fd) close (fd); } +static void +tfile_write_64 (uint64_t value) +{ + memcpy (trptr, &value, sizeof (uint64_t)); + trptr += sizeof (uint64_t); +} -void -add_memory_block (char *addr, int size) +static void +tfile_write_16 (uint16_t value) +{ + memcpy (trptr, &value, sizeof (uint16_t)); + trptr += sizeof (uint16_t); +} + +static void +tfile_write_8 (uint8_t value) +{ + memcpy (trptr, &value, sizeof (uint8_t)); + trptr += sizeof (uint8_t); +} + +static void +tfile_write_addr (char *addr) +{ + tfile_write_64 ((uint64_t) (uintptr_t) addr); +} + +static void +tfile_write_buf (const void *addr, size_t size) { - short short_x; - unsigned long long ll_x; - - *((char *) trptr) = 'M'; - trptr += 1; - ll_x = (unsigned long) addr; - memcpy (trptr, &ll_x, sizeof (unsigned long long)); - trptr += sizeof (unsigned long long); - short_x = size; - memcpy (trptr, &short_x, 2); - trptr += 2; memcpy (trptr, addr, size); trptr += size; } void +add_memory_block (char *addr, int size) +{ + tfile_write_8 ('M'); + tfile_write_addr (addr); + tfile_write_16 (size); + tfile_write_buf (addr, size); +} + +/* Adjust a function's address to account for architectural + particularities. */ + +static uintptr_t +adjust_function_address (uintptr_t func_addr) +{ +#if defined(__thumb__) || defined(__thumb2__) + /* Although Thumb functions are two-byte aligned, function + pointers have the Thumb bit set. Clear it. */ + return func_addr & ~1; +#else + return func_addr; +#endif +} + +/* Get a function's address as an integer. */ + +#define FUNCTION_ADDRESS(FUN) adjust_function_address ((uintptr_t) &FUN) + +void write_basic_trace_file (void) { int fd, int_x; - short short_x; - long func_addr; + unsigned long long func_addr; fd = start_trace_file (TFILE_DIR "tfile-basic.tf"); @@ -110,14 +162,9 @@ write_basic_trace_file (void) /* Dump tracepoint definitions, in syntax similar to that used for reconnection uploads. */ /* FIXME need a portable way to print function address in hex */ - func_addr = (long) &write_basic_trace_file; -#if defined(__thumb__) || defined(__thumb2__) - /* Although Thumb functions are two-byte aligned, function - pointers have the Thumb bit set. Clear it. */ - func_addr &= ~1; -#endif + func_addr = FUNCTION_ADDRESS (write_basic_trace_file); - snprintf (spbuf, sizeof spbuf, "tp T1:%lx:E:0:0\n", func_addr); + snprintf (spbuf, sizeof spbuf, "tp T1:%llx:E:0:0\n", func_addr); write (fd, spbuf, strlen (spbuf)); /* (Note that we would only need actions defined if we wanted to test tdump.) */ @@ -129,14 +176,13 @@ write_basic_trace_file (void) /* (Encapsulate better if we're going to do lots of this; note that buffer endianness is the target program's enddianness.) */ trptr = trbuf; - short_x = 1; - memcpy (trptr, &short_x, 2); - trptr += 2; + tfile_write_16 (1); + tfsizeptr = trptr; trptr += 4; - add_memory_block (&testglob, sizeof (testglob)); + add_memory_block ((char *) &testglob, sizeof (testglob)); /* Divide a variable between two separate memory blocks. */ - add_memory_block (&testglob2, 1); + add_memory_block ((char *) &testglob2, 1); add_memory_block (((char*) &testglob2) + 1, sizeof (testglob2) - 1); /* Go back and patch in the frame size. */ int_x = trptr - tfsizeptr - sizeof (int); @@ -181,8 +227,8 @@ write_error_trace_file (void) { int fd; const char made_up[] = "made-up error"; + char hex[(sizeof (made_up) - 1) * 2 + 1]; int len = sizeof (made_up) - 1; - char *hex = alloca (len * 2 + 1); fd = start_trace_file (TFILE_DIR "tfile-error.tf"); @@ -206,9 +252,8 @@ write_error_trace_file (void) /* Dump tracepoint definitions, in syntax similar to that used for reconnection uploads. */ - /* FIXME need a portable way to print function address in hex */ - snprintf (spbuf, sizeof spbuf, "tp T1:%lx:E:0:0\n", - (long) &write_basic_trace_file); + snprintf (spbuf, sizeof spbuf, "tp T1:%llx:E:0:0\n", + (unsigned long long) FUNCTION_ADDRESS (write_basic_trace_file)); write (fd, spbuf, strlen (spbuf)); /* (Note that we would only need actions defined if we wanted to test tdump.) */ @@ -233,7 +278,7 @@ done_making_trace_files (void) } int -main (int argc, char **argv, char **envp) +main (void) { write_basic_trace_file (); diff --git a/gdb/testsuite/gdb.trace/tfile.exp b/gdb/testsuite/gdb.trace/tfile.exp index d6a60e5..54648b8 100644 --- a/gdb/testsuite/gdb.trace/tfile.exp +++ b/gdb/testsuite/gdb.trace/tfile.exp @@ -37,7 +37,7 @@ if {![is_remote host] && ![is_remote target]} { standard_testfile if { [gdb_compile "$srcdir/$subdir/$srcfile" $binfile \ executable \ - [list debug nowarnings \ + [list debug \ "additional_flags=-DTFILE_DIR=\"$tfile_dir\""]] \ != "" } { untested ${testfile}.exp