Re: [PATCH 6/6] New test: gdb.trace/mi-trace-frame-collected.exp

Pedro Alves
2013-06-25T17:31:55Z

On 06/13/2013 02:28 AM, Yao Qi wrote:

> +# Define an action.
> +set pcreg ""

Suggest giving it an empty line after the comment, as the set
is not yet defining the action.

> +if [is_amd64_regs_target] {
> + set pcreg "rip"
> +} elseif [is_x86_like_target] {
> + set pcreg "eip"
> +} else {
> + # Other ports support tracepoing should set the name of pc
> + # register.

# Other ports that support tracepoints should set the name
# of the pc register here.

> + fail "set the number and the name of pc register"
> + return -1
> +}
> +
> +mi_gdb_test "-break-commands 3 \"collect gdb_char_test\" \"collect gdb_union1_test\" \"collect gdb_struct1_test.l\" \"collect gdb_arr_test\[0\]\" \"collect $${pcreg}\" \"teval \$tsv += 1\" \"collect \$tsv\" \"end\" " \
> + {\^done} "set action"
> +
> +mi_gdb_test "-break-insert -a gdb_c_test" \
> + "\\^done,bkpt=\{number=\"${decimal}\",type=\"tracepoint\".*\"\}" \
> + "insert tracepoint on gdb_c_test"
> +# Collect a global variable to make sure no registers are collected
> +# expect PC.

Typo: except. I do that one often too. :-)

Suggest s/make sure/be sure/.

> +# Test MI command '-trace-frame-collected' from different data source.

"from different" threw me off a second (different compared to what?).

Suggest:

# Test the MI command '-trace-frame-collected' with DATA_SOURCE as
# trace data source.

> +
> +proc test_trace_frame_collected { data_source } {
> + global decimal hex
> + global mi_gdb_prompt
> +

This looks OK to me with those changes.

Thanks!

--
Pedro Alves