From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9350 invoked by alias); 27 Jun 2012 20:45:31 -0000 Received: (qmail 9341 invoked by uid 22791); 27 Jun 2012 20:45:29 -0000 X-SWARE-Spam-Status: No, hits=-6.5 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 27 Jun 2012 20:45:08 +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 q5RKj11i016061 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 27 Jun 2012 16:45:01 -0400 Received: from barimba (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q5RKixea006143 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Wed, 27 Jun 2012 16:45:00 -0400 From: Tom Tromey To: Stan Shebs Cc: Yao Qi , gdb-patches@sourceware.org Subject: Re: [PATCH] Dynamic printf for a target agent References: <4FC57340.6070306@earthlink.net> <4FC70975.7020604@codesourcery.com> <4FE8841A.7090100@earthlink.net> Date: Wed, 27 Jun 2012 20:45:00 -0000 In-Reply-To: <4FE8841A.7090100@earthlink.net> (Stan Shebs's message of "Mon, 25 Jun 2012 08:30:34 -0700") Message-ID: <87mx3o5vsk.fsf@fleche.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain 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 X-SW-Source: 2012-06/txt/msg00840.txt.bz2 >>>>> "Stan" == Stan Shebs writes: Stan> +static void Stan> +maint_agent_printf_command (char *exp, int from_tty) Stan> +{ [...] Stan> + expr = parse_exp_1 (&cmd1, (struct block *) 0, 1); Now that Hui's "-at" patch is going in, perhaps this function should have the same treatment. Stan> +void Stan> +ax_string (struct agent_expr *x, char *str, int slen) Stan> +{ Stan> + int i; Stan> + Stan> + grow_expr (x, slen + 3); Stan> + x->buf[x->len++] = ((slen + 1) >> 8) & 0xff; Stan> + x->buf[x->len++] = (slen + 1) & 0xff; I think this should check that the length fits in 2 bytes. Stan> +@item @code{printf} (0x34) @var{numargs} @var{string} @result{} Stan> +Do a formatted print, in the style of the C function @code{printf}). Stan> +The value of @var{numargs} is the number of arguments to expect on the Stan> +stack, while @var{string} is the format string, prefixed with a Stan> +two-byte length, and suffixed with a zero byte. The format string I think the docs should whether the length includes the zero byte. Stan> + case string_arg: Stan> + { Stan> + gdb_byte *str; Stan> + CORE_ADDR tem; Stan> + int j; Stan> + Stan> + tem = args[i]; Stan> + Stan> + /* This is a %s argument. Find the length of the string. */ Stan> + for (j = 0;; j++) Stan> + { Stan> + gdb_byte c; Stan> + Stan> + read_inferior_memory (tem + j, &c, 1); Stan> + if (c == 0) Stan> + break; Stan> + } Stan> + Stan> + /* Copy the string contents into a string inside GDB. */ Stan> + str = (gdb_byte *) alloca (j + 1); Stan> + if (j != 0) Stan> + read_inferior_memory (tem, str, j); Stan> + str[j] = 0; Stan> + Stan> + printf (current_substring, (char *) str); Is it ever possible for the argument to "%s" to be NULL? It seems like it should be; but then the length-finding code seems wrong, and the printing of a NULL should be handled directly, not left to the host printf. Stan> + case gdb_agent_op_printf: Stan> + { Stan> + int nargs, slen, i; Stan> + CORE_ADDR fn = 0, chan = 0; Stan> + /* Can't have more args than the entire size of the stack. */ Stan> + ULONGEST args[STACK_MAX]; Stan> + char *format; Stan> + Stan> + nargs = aexpr->bytes[pc++]; Stan> + slen = aexpr->bytes[pc++]; Stan> + slen = (slen << 8) + aexpr->bytes[pc++]; Stan> + format = (char *) &(aexpr->bytes[pc]); Perhaps double-check that the terminating \0 byte is in fact present. Stan> +if $target_can_dprintf { Stan> + Stan> + gdb_run_cmd Stan> + Stan> + gdb_test "" "Breakpoint" This seems weird. FWIW I rather liked the format-parsing refactoring. Tom