[PATCHv2,4/5] gdbserver: cleanup in handle_v_run
Checks
Context |
Check |
Description |
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_gdb_build--master-arm |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_gdb_check--master-arm |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 |
success
|
Testing passed
|
Commit Message
After the previous commit there is now a redundant string copy in
handle_v_run, this commit cleans that up.
There should be no functional change after this commit.
---
gdbserver/server.cc | 24 +++++-------------------
1 file changed, 5 insertions(+), 19 deletions(-)
Comments
>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
Andrew> After the previous commit there is now a redundant string copy in
Andrew> handle_v_run, this commit cleans that up.
Andrew> + /* Buffer to decode the argument into. */
Andrew> char *arg = (char *) xmalloc (len + 1);
...
Andrew> hex2bin (p, (gdb_byte *) arg, len);
Andrew> arg[len] = '\0';
Not really your problem, but since you're changing it anyway, using the
byte_vector form of hex2bin would remove some manual memory management
here.
Tom
Tom Tromey <tom@tromey.com> writes:
>>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Andrew> After the previous commit there is now a redundant string copy in
> Andrew> handle_v_run, this commit cleans that up.
>
> Andrew> + /* Buffer to decode the argument into. */
> Andrew> char *arg = (char *) xmalloc (len + 1);
> ...
> Andrew> hex2bin (p, (gdb_byte *) arg, len);
> Andrew> arg[len] = '\0';
>
> Not really your problem, but since you're changing it anyway, using the
> byte_vector form of hex2bin would remove some manual memory management
> here.
I see two problems with that plan. First, we currently run hex2bin on a
slice of a larger string buffer, the slice is not null-terminated. The
current hex2bin that returns a gdb::byte_vector expects a
null-terminated string (it uses strlen internally). We can easily solve
this by adding an overload of hex2bin that takes a gdb::string_view,
except...
... for the second problem, which is the result of calling hex2bin is
passed off to either new_program_name or the new_argv vector. There's
no way to "release" the memory from a gdb::byte_vector, so we'd end up
copying the contents into a malloc'd buffer anyway, like:
gdb::byte_vector vec = hex2bin (gdb::string_view (p, (next_p - p)));
const char *arg = xstrdup ((const char *) vec.data ());
Which I'm not sure is really any better than we have right now?
Did you have something else in mind? Let me know and I'm happy to have
a look at an alternative approach.
Thanks,
Andrew
>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
Andrew> Which I'm not sure is really any better than we have right now?
Andrew> Did you have something else in mind? Let me know and I'm happy to have
Andrew> a look at an alternative approach.
Thanks, I agree your current approach is the better of the two.
Tom
@@ -2989,33 +2989,19 @@ handle_v_run (char *own_buf)
}
else
{
+ /* The length of the decoded argument. */
size_t len = (next_p - p) / 2;
- /* ARG is the unquoted argument received via the RSP. */
+
+ /* Buffer to decode the argument into. */
char *arg = (char *) xmalloc (len + 1);
- /* FULL_ARGS will contain the quoted version of ARG. */
- char *full_arg = (char *) xmalloc ((len + 1) * 2);
- /* These are pointers used to navigate the strings above. */
- char *tmp_arg = arg;
- char *tmp_full_arg = full_arg;
hex2bin (p, (gdb_byte *) arg, len);
arg[len] = '\0';
- while (*tmp_arg != '\0')
- {
- *tmp_full_arg = *tmp_arg;
- ++tmp_full_arg;
- ++tmp_arg;
- }
-
- /* Finish FULL_ARG and push it into the vector containing
- the argv. */
- *tmp_full_arg = '\0';
if (i == 0)
- new_program_name = full_arg;
+ new_program_name = arg;
else
- new_argv.push_back (full_arg);
- xfree (arg);
+ new_argv.push_back (arg);
}
if (*next_p == '\0')
break;