From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 92985 invoked by alias); 25 Feb 2017 19:11:52 -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 92962 invoked by uid 89); 25 Feb 2017 19:11:51 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.1 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-wr0-f196.google.com Received: from mail-wr0-f196.google.com (HELO mail-wr0-f196.google.com) (209.85.128.196) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 25 Feb 2017 19:11:49 +0000 Received: by mail-wr0-f196.google.com with SMTP id 89so5904802wrr.1 for ; Sat, 25 Feb 2017 11:11:49 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=x7pA5XNQEezXNVLO7uir83Or30yb5zIRaQmYFPJw8YE=; b=j+f7n0VB5MtM7IZA77f2zQ0ppJI/FQ870VEbponNXeZ9Kbg0VA7/7WE+9zLuZ8o71I gu+d3uLpq2B1DxU0e3oKXujsEaubUIsgRJlvZJRp4esbhOPl4ZTJTiQ2bfwa33AGaVTX v5TwHUWdQwIPj55wcEmfvCkDo9nVF2S5aO+h5c23rdNQUNQxOsQgvrGuycBEl7QjY16T yF8Jo1CcGc9nrhmwUHfXvI3VQS/bbfer5/pOud8x8PuNScIoW1ugCjt+xZEp2QT+VAjl AzUn75XwhPVa5WnTcet4tXdBsOQjS/JKCZoJmvgLzN9tVdvH0q+QEDWHqM5D4hQjw4iR iSqg== X-Gm-Message-State: AMke39me7etHJrpNTMlMnQFT0s9MUZeMnuPBGuQWOPZgg1HkfA6dBBXHmC1YOkzw/cGXSg== X-Received: by 10.223.152.86 with SMTP id v80mr5836716wrb.78.1488049907128; Sat, 25 Feb 2017 11:11:47 -0800 (PST) Received: from [10.0.0.57] (host86-170-201-9.range86-170.btcentralplus.com. [86.170.201.9]) by smtp.gmail.com with ESMTPSA id 136sm7362415wms.32.2017.02.25.11.11.46 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 25 Feb 2017 11:11:46 -0800 (PST) Subject: Re: [PATCH] fix bug with command `printf "%s\n", $_as_string($pc)` To: Simon Marchi References: <959cdc8e-1e54-a2e7-53d0-d80aaaea9ea8@gmail.com> <1d49ea752aae175256c0278bf3a999bc@polymtl.ca> <1cbe8b68-b592-825a-c662-56096ef0f795@gmail.com> Cc: gdb-patches@sourceware.org From: Matthew Malcomson Message-ID: Date: Sat, 25 Feb 2017 19:11:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2017-02/txt/msg00697.txt.bz2 On 25/02/17 18:33, Simon Marchi wrote: > Hi Matthew, > > I noted mostly some minor formatting issues, in general it looks good > to me. One comment about malloc. Sure, I have just a few questions > On 2017-02-25 06:45, Matthew Malcomson wrote: >> CHANGELOG: >> >> 2017-02-19 Matthew Malcomson >> >> * python/py-value.c (convert_value_from_python): Include NULL >> terminator in result. >> testsuite/gdb.python/py-as-string.c, >> testsuite/gdb.python/py-as-string.exp: Update tests >> to account for NULL terminator from python string values. >> doc/gdb.texinfo ($trace_func): Mention this value can't be used >> with printf. > > There is a ChangeLog in the doc and testsuite directories, so you > should place these entries in the relevant ChangeLogs. Also, look at > this section of the GDB wiki for more info on the proper format. So I should include the changelog entry as part of the patch? (I just sent it in the email based on how I read the CONTRIBUTE file) > > https://sourceware.org/gdb/wiki/ContributionChecklist#Properly_Formatted_GNU_ChangeLog > > >> +static char arena[51] = >> "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"; >> + >> +/* Override malloc() so value_coerce_to_target() gets a known >> pointer, and we >> + know we'll see an error if $_as_string() returns a string that >> isn't NULL >> + terminated. */ > > IIUC, the goal of overriding malloc is to ensure that the memory > return by malloc is not all zeroes, which would potentially hide the > bug? If that's right, you could instead write a wrapper for malloc > instead of a replacement. The wrapper would memset the allocated > buffer to 'x'es, for example. This way, it will be safer in case > there are many calls to malloc or calls with size > 51. > > See option #2 of this answer: http://stackoverflow.com/a/262481 > Yes, that was the reason. I used this way because I read that gdb also worked on non-POSIX systems (windows especially) and thought having a working test on all systems would be preferred (though I didn't check that all systems support the testing framework). I believe that no other calls to malloc are made in the inferior for this test, and that this program isn't used anywhere else, so this limit of 51 bytes is never hit. I agree this is a bug waiting to happen, so I can accept if the alternate would be preferred, but I thought I'd mention my reasoning. >> +void *malloc(size_t size) > > We try to respect the GNU/GDB coding style even in tests, so please > put the return type on its own line and put a space after the function > name: My apologies > void * > malloc (size_t size) > { > ... > } > >> +{ >> + if (size > sizeof(arena)) > > Space after sizeof. > >> + return NULL; >> + >> + return arena; >> +} > > The indentation in C/C++ code sould be two spaces per indent, until > you have 8 spaces, it then becomes a tab. > >> + >> static enum EnumType enum_valid = ENUM_VALUE_B; >> static enum EnumType enum_invalid = 20; >> >> diff --git a/gdb/testsuite/gdb.python/py-as-string.exp >> b/gdb/testsuite/gdb.python/py-as-string.exp >> index 0c44d5f174..819442834c 100644 >> --- a/gdb/testsuite/gdb.python/py-as-string.exp >> +++ b/gdb/testsuite/gdb.python/py-as-string.exp >> @@ -35,6 +35,12 @@ proc test_as_string { } { >> gdb_test "p \$_as_string(2)" "\"2\"" >> gdb_test "p \$_as_string(enum_valid)" "\"ENUM_VALUE_B\"" >> gdb_test "p \$_as_string(enum_invalid)" "\"20\"" >> + # Test that the NULL character is included in the returned value. >> + gdb_test "printf \"%s\\n\", \$_as_string(\"hi\")" "\"hi\"" >> + # Quote once to define the string, and once for the regexp. >> + gdb_test "interpreter-exec mi '-var-create test * >> \$_as_string(\"Hello\")'" \ >> + "\\^done,name=\"test\",numchild=\"8\",value=\"\\\[8]\",type=\"char >> \\\[8]\",has_more=\"0\"" > > Indent this with a leading tab. > > If you want to avoid massive escaping, you can use {} strings instead > of "" strings. {} strings are treated literally, so there's no > $variable substitution, no [proc invocation], no need to escape a > literal backslash, etc. You still need to escape characters that have > a special meaning in regex though. > > "\\^done,name=\"test\",numchild=\"8\",value=\"\\\[8]\",type=\"char > \\\[8]\",has_more=\"0\"" > > would become (I think, I have not tested) > > {\^done,name="test",numchild="8",value="\[8]",type="char > \[8]",has_more="0"} Yes, this does work, I had chosen "" strings to match the previous lines (I figured I'd have a comment either mentioning why this string used different delimiters or why there was extra backslashing, and it looked neater to me this way). Would you prefer using {} strings? or was that just a heads-up in case I didn't know? > > Finally, feel free to add newlines between logical groups of lines to > make the code more readable. > > Thanks, > > Simon >