From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20999 invoked by alias); 16 Jul 2014 18:36:56 -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 20929 invoked by uid 89); 16 Jul 2014 18:36:55 -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; Wed, 16 Jul 2014 18:36:52 +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 s6GIamk5004080 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Wed, 16 Jul 2014 14:36:48 -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 s6GIak0v030415; Wed, 16 Jul 2014 14:36:47 -0400 Message-ID: <53C6C63D.3040100@redhat.com> Date: Wed, 16 Jul 2014 19:02:00 -0000 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> <53C52622.3000405@redhat.com> <53C53319.7090001@codesourcery.com> <53C537A1.9080802@redhat.com> <53C5DC6F.6060007@codesourcery.com> In-Reply-To: <53C5DC6F.6060007@codesourcery.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2014-07/txt/msg00450.txt.bz2 On 07/16/2014 02:59 AM, Yao Qi wrote: > On 07/15/2014 10:16 PM, Pedro Alves wrote: >> Do you want to test it first, or shall we push it in now, and it gets >> coverage next time it percolates to your setups through mainline? > > Sorry for being unclear, I meant you can push it first. It's my own fault for not being explicit in the first place. :-) > FWIW, I tested the patch for mainline on arm targets too, it is OK. Great! I've pushed it in now, with the FIXME comment you pointed out in the other email removed. Thanks! -------------- >From 1b5d0ab34c53d6e896d2c0958b1176e324bb7878 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Wed, 16 Jul 2014 19:25:41 +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); ^ Tested on x86_64 Fedora 20, -m64 and -m32. Tested by Yao on arm targets. gdb/testsuite/ 2014-07-16 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/ChangeLog | 17 ++++++ gdb/testsuite/gdb.trace/tfile.c | 114 ++++++++++++++++++++++++++------------ gdb/testsuite/gdb.trace/tfile.exp | 2 +- 3 files changed, 97 insertions(+), 36 deletions(-) diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog index 3f3ba60..0d0c9a9 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,20 @@ +2014-07-16 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. + 2014-07-15 Simon Marchi * gdb.base/debug-expr.exp: Test string evaluation with diff --git a/gdb/testsuite/gdb.trace/tfile.c b/gdb/testsuite/gdb.trace/tfile.c index bc25b80..e69240a 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"); @@ -109,15 +161,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 +175,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 +226,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 +251,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 +277,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 -- 1.9.3