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

[Philippe Waroquiers <philippe.waroquiers@skynet.be>]

Ping ?

Thanks

Philippe

On Thu, 2020-06-04 at 22:42 +0200, Philippe Waroquiers 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.
> 
> 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.  */
> +
> +  /* 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);
> +
>    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.
> +
> +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" {
> +	-re "Attaching to program.*exec-file-mismatch handling is currently \"ask\".*Load new symbol table from .*attach\".*\(y or n\)" {
> +	    pass "$test attach1 again"
> +	}
> +    }
> +    gdb_test "y" "Reading symbols from .*attach.*" "$test load attach1 again"
> +
> +    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"
> +	}
> +    }
> +    gdb_test "y" "Reading symbols from .*attach2.*" \
> +	"$test load attach2 exec-file changed and load another exec file"
> +
> +    # Restore initial build situation.
> +    remote_exec build "mv ${binfile} ${binfile3}"
> +    remote_exec build "mv ${binfile}.initial ${binfile}"
>  
>      # Don't leave a process around
>      kill_wait_spawned_process $test_spawn_id
>      kill_wait_spawned_process $test_spawn_id2
> +    kill_wait_spawned_process $test_spawn_id3
>  }
>  
>  do_attach_tests
> diff --git a/gdb/testsuite/gdb.base/attach2.c b/gdb/testsuite/gdb.base/attach2.c
> index 44d37258fb..d070d933b0 100644
> --- a/gdb/testsuite/gdb.base/attach2.c
> +++ b/gdb/testsuite/gdb.base/attach2.c
> @@ -9,12 +9,14 @@
>  #include <unistd.h>
>  
>  float  bidule = 0.0;
> -int  should_exit = 0;
> +volatile int  should_exit = 0;
>  
>  int main ()
>  {
>    int  local_i = 0;
>  
> +  /* Cannot sleep a very long time, as attach.exp assumes the
> +     process will exit before the standard GDB timeout.  */
>    sleep( 10 ); /* System call causes register fetch to fail */
>                 /* This is a known HPUX "feature"            */
>    while (! should_exit)
> diff --git a/gdb/testsuite/gdb.base/attach3.c b/gdb/testsuite/gdb.base/attach3.c
> new file mode 100644
> index 0000000000..09a6d886df
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/attach3.c
> @@ -0,0 +1,25 @@
> +/* This program is intended to be started outside of gdb, and then
> +   attached to by gdb.  Thus, it simply spins in a loop.  The loop
> +   is exited when & if the variable 'should_exit' is non-zero.  (It
> +   is initialized to zero in this program, so the loop will never
> +   exit unless/until gdb sets the variable to non-zero.)
> +   */
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +
> +double  bidule = 0.0;
> +volatile int  should_exit = 0;
> +
> +int main ()
> +{
> +  int  local_i = 0;
> +
> +  sleep( 60 ); /* System call causes register fetch to fail */
> +               /* This is a known HPUX "feature"            */
> +  while (! should_exit)
> +    {
> +      local_i++;
> +    }
> +  return (0);
> +}