Re: [PATCH 4/4] gdb: Clear tilecfg.start_row for any PC modification.

[Pedro Alves <pedro@palves.net>]

Hi Felix,

The GDB change LGTM.  

But just to be clear, it is always OK to reset start_row, even if we didn't stop for a tile
instruction fault?  It won't ever lead to inferior corruption, for example?  I'm thinking of how
for instance, we always write to the PC after a breakpoint, to adjust it by -1, so a breakpoint on
a tile insn will always lead to resetting start_row AFAICT.

Some comments on the testcase below.

On 2022-05-06 13:12, Felix Willgerodt via Gdb-patches wrote:
> AMX tile instructions are restartable, e.g. on faults.  Tilecfg.start_row
> is used to restart the interrupted instructions at the right row.
> On inferior calls, jumps or any other PC modification, start_row needs
> to be reset.  It binds to the current instruction and not to the one we
> would start executing next in these cases.
> ---
>  gdb/amd64-linux-tdep.c                        |  24 ++++
>  gdb/testsuite/gdb.arch/amd64-amx-startrow.c   | 122 ++++++++++++++++++
>  gdb/testsuite/gdb.arch/amd64-amx-startrow.exp |  91 +++++++++++++
>  3 files changed, 237 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.arch/amd64-amx-startrow.c
>  create mode 100755 gdb/testsuite/gdb.arch/amd64-amx-startrow.exp
> 
> diff --git a/gdb/amd64-linux-tdep.c b/gdb/amd64-linux-tdep.c
> index cda90de54c6..65cce7f36ed 100644
> --- a/gdb/amd64-linux-tdep.c
> +++ b/gdb/amd64-linux-tdep.c
> @@ -314,6 +314,30 @@ amd64_linux_write_pc (struct regcache *regcache, CORE_ADDR pc)
>       within GDB.  In all other cases the system call will not be
>       restarted.  */
>    regcache_cooked_write_unsigned (regcache, AMD64_LINUX_ORIG_RAX_REGNUM, -1);
> +
> +  /* If we have interrupted a restart-able AMX instruction we should clear

Odd hyphenization given "restartable" is a word.  (and you used it in the commit log.)

> +     start_row.  Any instructions we will now run should start at row 0.  */
> +  i386_gdbarch_tdep *tdep
> +      = (i386_gdbarch_tdep *) gdbarch_tdep (regcache->arch ());
> +  if (tdep != nullptr && tdep->tilecfg_raw_regnum != -1)
> +    {
> +      gdb_byte tilecfg_buf[register_size (regcache->arch (),
> +					  tdep->tilecfg_raw_regnum)];
> +
> +      if (regcache->raw_read (tdep->tilecfg_raw_regnum, tilecfg_buf)
> +	  != REG_VALID)
> +	{
> +	  warning (_ ("Could not reset $tilecfg.start_row."));
> +	  return;
> +	}
> +
> +      /* start_row is the second byte.  */
> +      if (tilecfg_buf[1] != 0)
> +	{
> +	  tilecfg_buf[1] = 0;
> +	  regcache->raw_write (AMD64_AMX_TILECFG_RAW_REGNUM, tilecfg_buf);
> +	}
> +    }
>  }
>  


> +standard_testfile
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} debug] } {
> +    return -1
> +}

I'd use build_executable instead here.  See below for why.

> +
> +proc test_startrow {test} {
> +    global gdb_prompt hex decimal srcfile
> +
> +    if { ![runto_main] } {
> +	untested "could not run to main"
> +	return -1
> +    }
> +
> +    set line1 [gdb_get_line_number "BP1"]
> +    set line2 [gdb_get_line_number "BP2"]
> +    set line_jump [gdb_get_line_number "Jump"]
> +    gdb_breakpoint $line1
> +    gdb_breakpoint $line2
> +
> +    gdb_continue_to_breakpoint "line1" ".*$srcfile:$line1.*"
> +
> +    # Set a watchpoint on the first page, which is un-protected.

un-protected -> unprotected ?

> +    set watch_addr 0
> +    gdb_test_multiple "p/x p2 - 8" "get watch_addr" {
> +	-re -wrap "= ($hex)" {
> +	    set watch_addr $expect_out(1,string)
> +	    pass $gdb_test_name
> +	}
> +    }

This could be instead:

  set watch_addr [get_valueof "/x" "p2 - 8" 0 "get watch_addr"]

> +
> +    # If we didn't get a watch_addr, it makes no sense to continue.
> +    if { $watch_addr == 0 } {
> +	return -1
> +    }
> +
> +    gdb_test "rwatch *(int*) $watch_addr" \
> +	"atchpoint $decimal: \\*\\(int\\*\\) $watch_addr"

This needs an explicit test name to avoid having addresses leak to gdb.sum results.

> +
> +    gdb_test "continue" \
> +	"Continuing.*atchpoint $decimal: \\*\\(int\\*\\) $watch_addr.*"
> +
> +    gdb_test "p \$tilecfg.start_row" "= \[1-9\]+" "print non-zero start_row"
> +
> +    if { $test == "jump" } {
> +	# Test jump.
> +	gdb_test "jump $line_jump" "Breakpoint $decimal, .*$srcfile:$line2.*"

Likewise, better avoid line numbers in gdb.sum.

Please double check other tests your adding for addresses and line numbers
in gdb.sum.

> +	gdb_test "p \$tilecfg.start_row" "= 0"
> +    } else {
> +	# Test infcall.
> +	gdb_test "p square (2, 2)" "Breakpoint $decimal, .*$srcfile:$line2.*"
> +	gdb_test "p \$tilecfg.start_row" "= 0"
> +    }
> +}
> +
> +with_test_prefix "infcall" {
> +    test_startrow ""
> +}
> +
> +clean_restart $binfile
> +
> +with_test_prefix "jump" {
> +    test_startrow "jump"
> +}

I'd write this instead:

foreach_with_prefix test {"infcall" "jump"} {
    clean_restart $binfile
    test_startrow $test
}

and this is the reason for using build_executable instead of prepare_for_testing
at the top -- because this loop calls clean_restart itself.