From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29936 invoked by alias); 16 Jan 2014 19:01:28 -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 29927 invoked by uid 89); 16 Jan 2014 19:01:28 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.1 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-ig0-f180.google.com Received: from mail-ig0-f180.google.com (HELO mail-ig0-f180.google.com) (209.85.213.180) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Thu, 16 Jan 2014 19:01:26 +0000 Received: by mail-ig0-f180.google.com with SMTP id m12so8409254iga.1 for ; Thu, 16 Jan 2014 11:01:24 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=UQ9FZiGK6l7mbngXtXv1CGlS3bpM7XmLIOvAf2RJaNY=; b=SIjknhGX5nsKJfNL/oPGsJAqd80X58BK8K4qoaaM4/xQghABoI1QJlMyZpHTnrNF/j OFfBbb3wUoriJsO0xGOxp6t9oclyQHXIb9QulnBfm8jYn1VpabXP2BdoqNkK0s+JgC4I +SRwQjEJHY+4HOO8xN5KGkxGlVrvL6Itl4eEl/ANrFpiAxHBvFRU9PoyCDKrD9JxVCsY yXfl0JeAw/uO0CWZOJLuJkVLX9f9S1AddxnHVjHokXl9VVN12Hwk9O3DcTqPD2wzkXkA FZbUpW3vRY+Ms2nYmcrDxdi+xTvgomMWfiqowMfgg6D0FLpCAuqK12pphfxtivAAiGNQ 73tQ== X-Gm-Message-State: ALoCoQktAyF9tXq2N2e/GTLeFKZRYFiTK8POpMMaD+3WQKvIw9/y0YFbq6G/nRU6yctX/d2wtakoQEQ8d6Gh+4byZLYxMUy3hC1Fih+cDM9PyHC0axzPb00vZJRDkp4wDzZEfeBF4CnEI0xnNbJKdmg4Sm9LlUfRqmXRILig95reoW1tC5oVYGh3WqEaEvyuKQor8AaS9v5wzjf485n8J2d86DzlREx7wg== MIME-Version: 1.0 X-Received: by 10.50.67.180 with SMTP id o20mr11412593igt.43.1389898884665; Thu, 16 Jan 2014 11:01:24 -0800 (PST) Received: by 10.64.58.77 with HTTP; Thu, 16 Jan 2014 11:01:24 -0800 (PST) In-Reply-To: <52D826DF.4000505@codesourcery.com> References: <52B1842F.5020401@redhat.com> <21205.55987.69477.892571@ruffy.mtv.corp.google.com> <52D826DF.4000505@codesourcery.com> Date: Thu, 16 Jan 2014 19:01:00 -0000 Message-ID: Subject: Re: [PATCH 4/6] gdbserver: Delimit debugging output for readability From: Doug Evans To: Yao Qi Cc: Pedro Alves , gdb-patches Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2014-01/txt/msg00642.txt.bz2 On Thu, Jan 16, 2014 at 10:37 AM, Yao Qi wrote: > On 01/15/2014 08:47 AM, Doug Evans wrote: >> While going through the reviews, I found myself wanting something more, >> namely timestamps (like "set debug timestamp on" in gdb). >> > > Hi Doug, > Could you help me to understand how useful timestamps are for > measuring performance? or you use timestamps for other purposes? All of the above. I can use it for measuring performance, debugging(!) performance issues, and debugging in general. >> This patch adds a check for gettimeofday and doesn't fall back to using one >> in libiberty or gnulib, leaving that for another day. > > gdbserver has used gnulib, which means we can use gettimeofday > unconditionally in gdbserver? Left for another day. There's no loss in splitting this up into steps. [The next step will itself need to be spit up into several steps: get gettimeofday from gnulib, and then have gdbserver (and gdb) use it - it may be a nop for gdb, say gnulib being preferred over libiberty, but something to be confirmed nonetheless.] >> diff --git a/gdb/gdbserver/linux-aarch64-low.c b/gdb/gdbserver/linux-aarch64-low.c >> index 1b0da6c..e7d3e4f 100644 >> --- a/gdb/gdbserver/linux-aarch64-low.c >> +++ b/gdb/gdbserver/linux-aarch64-low.c >> @@ -323,7 +323,7 @@ aarch64_get_pc (struct regcache *regcache) >> >> collect_register_by_name (regcache, "pc", &pc); >> if (debug_threads) >> - fprintf (stderr, "stop pc is %08lx\n", pc); >> + debug_printf ("stop pc is %08lx\n", pc); >> return pc; > > IWBN to move "if (debug_threads)" into debug_printf too. I thought of that, but there are times when you want to check debug_threads before calling debug_printf. So then it's a case of some code checking debug_threads and some not, and then how often will cut-n-paste hacking proliferate unnecessary tests of debug_printf. Another alternative is of course to call a different function that does the check, but now we have two debug printf functions instead of one. btw, One thought I have is to make debug_printf a macro (or create DEBUG_PRINTF or use a different name) so that adding FUNCTION_NAME becomes automagic. OTOH, I'm not sure always including the function name will be a net win - it could be, guess it might depend on personal preference (--debug=timestamp,funcname ? :-)). If people really want debug_threads tests in debug_printf (which is fine by me), that can be left for another day too. It is arguably a separate step since it's an additional change on top of replacing fprintf (stderr, ...) with debug_printf. >> 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 >> @@ -17,6 +17,7 @@ > >> + >> +/* Increase or decrease the debug printf call nesting level. >> + FUNCTION_NAME is the name of the calling function, or NULL if unknown. >> + Call this when entering major routines that can trigger a lot of debug >> + output before it exits. It allows the reader to associate subsequent >> + debug output to the call that ultimately triggered it. */ >> + >> +void >> +debug_level_incr (int incr, const char *function_name) >> +{ >> + gdb_assert (incr == 1 || incr == -1); >> + >> + /* Increment(/decrement) the level by one before printing the function name, >> + to distinguish this as an entry(/exit) point. >> + Then increment(/decrement) it again so that debugging printfs within >> + the function are recognized as such. */ >> + if (function_name != NULL) >> + { >> + debug_nesting_level += incr; >> + debug_printf ("%s %s\n", >> + incr > 0 ? ">>>>entering" : "<<<> + function_name); >> + } >> + debug_nesting_level += incr; >> + >> + /* Don't crash on mismatched enter/exit, but still inform the user. */ >> + if (debug_nesting_level < 0) >> + { >> + debug_printf ("ERROR: mismatch in debug_level_enter/exit, level < 0\n"); >> + debug_nesting_level = 0; >> + } >> +} >> + > > utils.c is compiled to both gdbserver and ipa. IMO, > ipa code should be thread-safe, because it can be used by a > multi-threaded program. That would argue for removing the indentation support, at least for now. Fine by me. OTOH, it seemed like ipa code has its own debug printf'ing so it can prepend PROG, so I'm not sure this is necessary. OTOOH, it'd be less preferable to assume(!) ipa code would never call debug_printf. Thoughts?