From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5814 invoked by alias); 16 Jan 2014 17:22: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 5803 invoked by uid 89); 16 Jan 2014 17:22:55 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 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 ESMTP; Thu, 16 Jan 2014 17:22:54 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s0GHMpQa008425 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 16 Jan 2014 12:22:51 -0500 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s0GHMnxP021102; Thu, 16 Jan 2014 12:22:50 -0500 Message-ID: <52D81569.3080006@redhat.com> Date: Thu, 16 Jan 2014 17:22:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: Doug Evans CC: yao@codesourcery.com, gdb-patches@sourceware.org Subject: Re: [PATCH 4/6] gdbserver: Delimit debugging output for readability References: <52B1842F.5020401@redhat.com> <21205.55987.69477.892571@ruffy.mtv.corp.google.com> In-Reply-To: <21205.55987.69477.892571@ruffy.mtv.corp.google.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2014-01/txt/msg00625.txt.bz2 On 01/15/2014 12:47 AM, Doug Evans wrote: > Hi. > While going through the reviews, I found myself wanting something more, > namely timestamps (like "set debug timestamp on" in gdb). > > This patch adds a check for gettimeofday and doesn't fall back to using one > in libiberty or gnulib, leaving that for another day. > > It also adds macro FUNCTION_NAME, based on the implementation in gdb_assert.h. > A subsequent patch will use this when replacing abbreviations with > function names. > > Unlike gdb, I didn't add an option to enable timestamps, you get them > automagically with --debug. I can do that if people want. I'd like that. I've had to diff logs (side by side in e.g., kdiff3) more than once in the past to track tricky stuff, and timestamps unfortunately make that impossible. > I had a version of this patch that used indentation to represent > the heirarchy of debug_level_{enter,exit}. I left the basic support > in thinking it might be useful. I can either remove it or add the indentation, > but I found myself not really wanting the indentation. I don't have a strong opinion, as I didn't make use of this yet, but I think I wouldn't want indentation. (E.g., maybe printing a level counter instead of indentation could be better.). > diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c > index 13b3ff6..3706577 100644 > --- a/gdb/gdbserver/tracepoint.c > +++ b/gdb/gdbserver/tracepoint.c > @@ -61,6 +61,8 @@ > > */ > > +#ifdef IN_PROCESS_AGENT > + > static void trace_vdebug (const char *, ...) ATTRIBUTE_PRINTF (1, 2); > > static void > @@ -81,6 +83,19 @@ trace_vdebug (const char *fmt, ...) > trace_vdebug ((fmt), ##args); \ > } while (0) > > +#else > + > +#define trace_debug_1(level, fmt, args...) \ > + do { \ > + if (level <= debug_threads) \ > + { \ > + debug_printf ((fmt), ##args); \ > + debug_printf ("\n"); \ > + } \ > + } while (0) > + Please write this in a way that preserves printing PROG in gdbserver. When debugging gdbserver and the IPA both tracing at the same time (fast tracepoints and regular tracepoints), it's confusing not to have there explicitly who printed what, because both programs print the same messages. > +#endif > + > #define trace_debug(FMT, args...) \ > trace_debug_1 (1, FMT, ##args) > > @@ -338,7 +353,7 @@ tracepoint_look_up_symbols (void) > if (look_up_one_symbol (symbol_list[i].name, addrp, 1) == 0) > { > if (debug_threads) > - fprintf (stderr, "symbol `%s' not found\n", symbol_list[i].name); > + debug_printf ("symbol `%s' not found\n", symbol_list[i].name); > return; > } > } > diff --git a/gdb/gdbserver/utils.c b/gdb/gdbserver/utils.c > index eff4499..1ce5512 100644 > --- a/gdb/gdbserver/utils.c > +++ b/gdb/gdbserver/utils.c Could this new debug support code be put in a new file instead? E.g., gdbserver/debug.c ? Otherwise this all looks good to me. > /* Temporary storage using circular buffer. */ > #define NUMCELLS 10 > #define CELLSIZE 50 > diff --git a/gdb/gdbserver/utils.h b/gdb/gdbserver/utils.h > index 6d3df71..6c781c0 100644 > --- a/gdb/gdbserver/utils.h > +++ b/gdb/gdbserver/utils.h > @@ -19,10 +19,44 @@ > #ifndef UTILS_H > #define UTILS_H > > +/* Version 2.4 and later of GCC define a magical variable `__PRETTY_FUNCTION__' > + which contains the name of the function currently being defined. > + This is broken in G++ before version 2.6. > + C9x has a similar variable called __func__, but prefer the GCC one since > + it demangles C++ function names. */ > +#if (GCC_VERSION >= 2004) > +#define FUNCTION_NAME __PRETTY_FUNCTION__ > +#else > +#if defined __STDC_VERSION__ && __STDC_VERSION__ >= 199901L > +#define FUNCTION_NAME __func__ > +#endif > +#endif It'd be nice if this and gdb_assert.h's version of the same were shared. That is, e.g., put this in common/common-utils.h instead, and make gdb_assert.h define ASSERT_FUNCTION as FUNCTION_NAME (or eliminate ASSERT_FUNCTION entirely). Are you planning on doing it? -- Pedro Alves