From: Simon Marchi <simon.marchi@polymtl.ca>
To: Matthew Malcomson <hardenedapple@gmail.com>
Cc: gdb-patches@sourceware.org
Subject: Re: [PATCH] fix bug with command `printf "%s\n", $_as_string($pc)`
Date: Sat, 25 Feb 2017 18:33:00 -0000 [thread overview]
Message-ID: <d0afcb18fedeb6cd1d228f4777e844c9@polymtl.ca> (raw)
In-Reply-To: <1cbe8b68-b592-825a-c662-56096ef0f795@gmail.com>
Hi Matthew,
I noted mostly some minor formatting issues, in general it looks good to
me. One comment about malloc.
On 2017-02-25 06:45, Matthew Malcomson wrote:
> CHANGELOG:
>
> 2017-02-19 Matthew Malcomson <hardenedapple@gmail.com>
>
> * 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.
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
> +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:
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"}
Finally, feel free to add newlines between logical groups of lines to
make the code more readable.
Thanks,
Simon
next prev parent reply other threads:[~2017-02-25 18:33 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-02-19 17:32 Matthew Malcomson
2017-02-20 18:20 ` Simon Marchi
2017-02-25 11:45 ` Matthew Malcomson
2017-02-25 18:33 ` Simon Marchi [this message]
2017-02-25 19:11 ` Matthew Malcomson
2017-02-25 21:33 ` Simon Marchi
2017-02-26 13:20 ` Matthew Malcomson
2017-02-26 14:44 ` Simon Marchi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d0afcb18fedeb6cd1d228f4777e844c9@polymtl.ca \
--to=simon.marchi@polymtl.ca \
--cc=gdb-patches@sourceware.org \
--cc=hardenedapple@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).