[RFAv2] Ensure 'exec-file has changed' check has priority over 'exec-file-mismatch' check

[Pedro Alves]

On 6/4/20 9:42 PM, Philippe Waroquiers via Gdb-patches wrote:
> This is version 2. Compared to first version:
>    rebased to last master
>    removed a comment duplicated by error
>    'git add-ed' the attach3.c file I forgot.
> 
> Following the implementation of exec-file-mismatch based on build-id,
> an attach to a process that runs a modified exec-file was triggering
> the exec-file-mismatch handling, giving a warning such as:
>   warning: Mismatch between current exec-file /bd/home/philippe/gdb/git/build_termours/gdb/testsuite/outputs/gdb.base/attach/attach
>   and automatically determined exec-file /bd/home/philippe/gdb/git/build_termours/gdb/testsuite/outputs/gdb.base/attach/attach
>   exec-file-mismatch handling is currently "ask"
> as the build-ids differ when an exec-file is recompiled.

I wonder whether the warning should mention Build IDs, like

- warning: Mismatch between ...
+ warning: Build ID mismatch between ...

> 
> This patch ensures that the exec-file-mismatch check is done with an up to date
> build-id.  With this, exec-file-mismatch check will only trigger when the
> PID file really differs from the (build-id refreshed) current exec-file.
> Note that the additional check does not (yet) reload the symbols if
> the exec-file is changed: this reload will happen later if needed.
> 
> gdb/ChangeLog
> YYYY-MM-DD  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* exec.c (validate_exec_file): Ensure the build-id is up to
> 	date by calling reopen_exec_file (that checks file timestamp
> 	to decide to re-read the file).
> 
> gdb/testsuite/ChangeLog
> 
> YYYY-MM-DD  Philippe Waroquiers  <philippe.waroquiers@skynet.be>
> 
> 	* gdb.base/attach.exp: Test priority of 'exec-file' changed
> 	over 'exec-file-mismatch'.
> 	* gdb.base/attach.c: Mark should_exit volatile.
> 	* gdb.base/attach2.c: Likewise.  Add a comment explaining
> 	why the sleep cannot be big.
> 	* gdb.base/attach3.c: New file.
> ---
>  gdb/exec.c                        | 12 +++++--
>  gdb/testsuite/gdb.base/attach.c   |  2 +-
>  gdb/testsuite/gdb.base/attach.exp | 56 +++++++++++++++++++++++++++++--
>  gdb/testsuite/gdb.base/attach2.c  |  4 ++-
>  gdb/testsuite/gdb.base/attach3.c  | 25 ++++++++++++++
>  5 files changed, 93 insertions(+), 6 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.base/attach3.c
> 
> diff --git a/gdb/exec.c b/gdb/exec.c
> index ee13c5e027..fe4f94f634 100644
> --- a/gdb/exec.c
> +++ b/gdb/exec.c
> @@ -254,8 +254,16 @@ validate_exec_file (int from_tty)
>    if (current_exec_file == NULL || pid_exec_file == NULL)
>      return;
>  
> -  /* Try validating via build-id, if available.  This is the most
> -     reliable check.  */
> +   /* Try validating via build-id, if available.  This is the most
> +      reliable check.  */

Spurious change.

> +
> +  /* In case current_exec_file was changed, reopen_exec_file ensures
> +     an up to date build_id (will do nothing if the file timestamp
> +     did not change).  If exec file changed, reopen_exec_file has
> +     allocated another file name, so get_exec_file again.  */
> +  reopen_exec_file ();
> +  current_exec_file = get_exec_file (0);
> +

I think that ideally we wouldn't reopen the exec file here, but instead
move the exec validation to setup_inferior, after the reopen/reread:

void
setup_inferior (int from_tty)
{
  struct inferior *inferior;

  inferior = current_inferior ();
  inferior->needs_setup = 0;

  /* If no exec file is yet known, try to determine it from the
     process itself.  */
  if (get_exec_file (0) == NULL)
    exec_file_locate_attach (inferior_ptid.pid (), 1, from_tty);
  else
    {
      reopen_exec_file ();
      reread_symbols ();
+     validate_exec_file (from_tty);
    }


That would centralize things better, and also, when we later teach
GDB to validate the symbol-file 's build ID as well, it would all
be done together.  We could also query debuginfod for a matching
binary around here.

But that's easier said than done.  Particularly the remote target's
initial setup doesn't always end up here in setup_inferior; it's quite
messy.  I gave it a try, but gave up for now, because it was requiring
changing too much of the initial remote connection setup code and taking
too long.  Can always revisit later.

So let's go with what you have.  Some comments on the testcase below.
But otherwise, with those addressed, please go ahead and push.

>    const bfd_build_id *exec_file_build_id = build_id_bfd_get (exec_bfd);
>    if (exec_file_build_id != nullptr)
>      {
> diff --git a/gdb/testsuite/gdb.base/attach.c b/gdb/testsuite/gdb.base/attach.c
> index 2e87f9b710..b3c5498401 100644
> --- a/gdb/testsuite/gdb.base/attach.c
> +++ b/gdb/testsuite/gdb.base/attach.c
> @@ -8,7 +8,7 @@
>  #include <unistd.h>
>  
>  int  bidule = 0;
> -int  should_exit = 0;
> +volatile int  should_exit = 0;
>  
>  int main ()
>  {
> diff --git a/gdb/testsuite/gdb.base/attach.exp b/gdb/testsuite/gdb.base/attach.exp
> index 32f72e2a9a..84aede7e2a 100644
> --- a/gdb/testsuite/gdb.base/attach.exp
> +++ b/gdb/testsuite/gdb.base/attach.exp
> @@ -17,12 +17,13 @@ if {![can_spawn_for_attach]} {
>      return 0
>  }
>  
> -standard_testfile attach.c attach2.c
> +standard_testfile attach.c attach2.c attach3.c
>  set binfile2 ${binfile}2
> +set binfile3 ${binfile}3
>  set escapedbinfile  [string_to_regexp $binfile]
>  
>  #execute_anywhere "rm -f ${binfile} ${binfile2}"
> -remote_exec build "rm -f ${binfile} ${binfile2}"
> +remote_exec build "rm -f ${binfile} ${binfile2} ${binfile3}"
>  # For debugging this test
>  #
>  #log_user 1
> @@ -41,6 +42,13 @@ if  { [gdb_compile "${srcdir}/${subdir}/${srcfile2}" "${binfile2}" executable {d
>      return -1
>  }
>  
> +# Build the third file, used to check attach when exec-file has changed.

Missing "the" in "when the exec-file".

> +
> +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile3}" "${binfile3}" executable {debug}] != "" } {
> +    untested "failed to compile attach exec-file changed test"
> +    return -1
> +}
> +
>  if [get_compiler_info] {
>      return -1
>  }
> @@ -515,6 +523,7 @@ proc_with_prefix do_attach_exec_mismatch_handling_tests {} {
>      global gdb_prompt
>      global binfile
>      global binfile2
> +    global binfile3
>  
>      clean_restart $binfile
>  
> @@ -577,10 +586,53 @@ proc_with_prefix do_attach_exec_mismatch_handling_tests {} {
>      # Detach the process.
>      gdb_test "detach" "Detaching from program: .* detached\\\]" "$test detach attach"
>  
> +    # Test that the 'exec-file' changed is checked before exec-file-mismatch.
> +    set test "mismatch exec-file changed has priority"
> +    gdb_test_no_output "set exec-file-mismatch ask"
> +    gdb_test_multiple "attach $testpid" "$test attach1 again, initial exec-file" {

The second parameter here, should match ...

> +	-re "Attaching to program.*exec-file-mismatch handling is currently \"ask\".*Load new symbol table from .*attach\".*\(y or n\)" {
> +	    pass "$test attach1 again"

... this message here.  Otherwise a FAIL catch by the internal gdb_test_multiple
patters ends up with a difference message from a PASS.  

Best is to use $gdb_test_name:

    pass $gdb_test_name

> +	}
> +    }
> +    gdb_test "y" "Reading symbols from .*attach.*" "$test load attach1 again"
> +

It's usually better if this "y" is done within the -re above, as it's all
part of the same test, like:

   gdb_test_multiple "attach $testpid" "$test attach1 again, initial exec-file" {
	-re "Attaching to program.*exec-file-mismatch handling is currently \"ask\".*Load new symbol table from .*attach\".*\(y or n\)" {
       gdb_test "y" "Reading symbols from .*attach.*" $gdb_test_name
	}
   }

Note this removes the separate "pass" call from within the -re.


> +    gdb_test "detach" "Detaching from program: .* detached\\\]" "$test detach attach initial exec-file"
> +
> +    # Change the exec-file and attach to a new process using the changed file.
> +    remote_exec build "mv ${binfile} ${binfile}.initial"
> +    remote_exec build "mv ${binfile3} ${binfile}"
> +    # Ensure GDB detects ${binfile} has changed when checking timestamp.
> +    sleep 1
> +    remote_exec build "touch ${binfile}"
> +    set test_spawn_id3 [spawn_wait_for_attach $binfile]
> +    set testpid3 [spawn_id_get_pid $test_spawn_id3]
> +
> +    gdb_test "attach $testpid3" "Attaching to program.*attach' has changed; re-reading symbols.*" \
> +	"$test attach1 again, after changing exec-file"
> +    gdb_test "detach" "Detaching from program: .* detached\\\]" "$test detach after attach changed exec-file"
> +
> +    # Now, test the situation when current exec-file has changed
> +    # and we attach to a pid using another file.
> +    # Ensure GDB detects ${binfile} has changed when checking timestamp.
> +    sleep 1
> +    remote_exec build "touch ${binfile}"
> +
> +    gdb_test_multiple "attach $testpid2" "$test attach2" {
> +	-re "Attaching to program.*exec-file-mismatch handling is currently \"ask\".*Load new symbol table from .*attach2\".*\(y or n\)" {
> +	    pass "$test attach2 with exec-file changed and need to load another exec-file"

Same message mismatch issue as above.

Thanks,
Pedro Alves