[MI non-stop 04/11] Implement --thread and --frame.
Marc Khouzam
marc.khouzam@ericsson.com
Sat Jun 28 18:13:00 GMT 2008
Hi,
I don't know enough to thoroughly review so I'll give the minor comments
I have.
> + if (parse->frame != -1 && !parse->thread == -1)
You probably didn't mean to have ! before parse->thread
> + error ("Cannot specify --frame without --thread");
> +
> + if (parse->thread != -1)
> + {
> + struct thread_info *tp = find_thread_id (parse->thread);
> + if (!tp)
> + error (_("Invalid thread id: %d"), parse->thread);
> +
> + if (non_stop)
> + context_switch_to (tp->ptid);
> + else
> + switch_to_thread (tp->ptid);
> + }
> +
> + if (parse->frame != -1)
> + {
> + struct frame_info *fid;
> + int frame = parse->frame;
> + fid = find_relative_frame (get_current_frame (), &frame);
> + if (frame == 0)
> + /* find_relative_frame was successful */
> + select_frame (fid);
> + else
> + error (_("Invalid frame id: %s"), frame_str);
> + }
> +
> if (parse->cmd->argv_func != NULL)
> {
> if (target_can_async_p ()
> @@ -1171,6 +1202,7 @@ mi_cmd_execute (struct mi_parse *parse)
> error_stream (stb);
> }
> }
> +
> current_token = xstrdup (parse->token);
> cleanup = make_cleanup (free_current_contents, ¤t_token);
> parse->cmd->argv_func (parse->command, parse->argv, parse->argc);
> diff --git a/gdb/mi/mi-parse.c b/gdb/mi/mi-parse.c
> index a2dc50d..835fb74 100644
> --- a/gdb/mi/mi-parse.c
> +++ b/gdb/mi/mi-parse.c
> @@ -150,6 +150,8 @@ mi_parse (char *cmd)
> char *chp;
> struct mi_parse *parse = XMALLOC (struct mi_parse);
> memset (parse, 0, sizeof (*parse));
> + parse->thread = -1;
> + parse->frame = -1;
>
> /* Before starting, skip leading white space. */
> while (isspace (*cmd))
> @@ -199,6 +201,38 @@ mi_parse (char *cmd)
> while (isspace (*chp))
> chp++;
>
> + /* Parse the --thread and --frame options, if present. At present,
> + some important commands, like '-break-*' are implemented by forwarding
> + to the CLI layer directly. We want to parse --thread and --frame
> + here, so as not to leave those option in the string that will be passed
> + to CLI. */
> + for (;;)
> + {
> + char *start = chp;
> + if (strncmp (chp, "--thread", 8) == 0)
Although this works, if you later add a new param like --thread-list,
it will falsly match here. Maybe you want to strncmp with "--thread "?
(with a space at the end)
I'm not sure what the coding style is for GDB in this case, but I have
found that it is more maintainable not to hard-code the lengths, but to use
something like
char* threadStr = "--thread ";
int len = strlen(threadStr);
> + {
> + if (parse->thread != -1)
> + error ("Duplicate '--thread' option");
> + chp += 8;
> + parse->thread = strtol (chp, &chp, 10);
> + }
> + else if (strncmp (chp, "--frame", 7) == 0)
> + {
> + if (parse->frame != -1)
> + error ("Duplicate '--frame' option");
> + chp += 7;
> + parse->frame = strtol (chp, &chp, 10);
> + }
> + else
> + break;
Are --frame and --thread the only options that can be found when we are
running this code? If yes, then its fine; if no, then you shouldn't break
on else but should skip the uninteresting option.
> +
> + if (*chp != '\0' && !isspace (*chp))
> + error ("Invalid value for the '%s' option",
> + start[2] == 't' ? "--thread" : "--frame");
> + while (isspace (*chp))
> + chp++;
> + }
> +
> /* For new argv commands, attempt to return the parsed argument
[...]
More information about the Gdb-patches
mailing list