Windows testsuite failures

Windows testsuite failures

[Hannes Domani]

I went through all testsuite failures in gdb.base, and fixed everything
I could figure out.

So this series is about the bugs in gdb itself.

After this, there are still quite a few unexpected failures, and even
some unexpected successes.

Like in these, gdb returns for virtual inheritance classes "vtable" where
"VTT" is expected:
FAIL: gdb.base/max-depth-c++.exp: exp='v2': depth=1: p v2
FAIL: gdb.base/max-depth-c++.exp: exp='v2': depth=2: p v2
FAIL: gdb.base/max-depth-c++.exp: exp='v2': depth=unlimited: p v2
FAIL: gdb.base/max-depth-c++.exp: exp='v3': depth=1: p v3

Or this one, where gdb is started with --write, and then crashes when
actually trying to write the executable at the end:
FAIL: gdb.base/write_mem.exp: x /xh main

I don't know if these are bugs, so I'm not sure what to do with them (the
crash part is obviously bad, but should --write even be possible?).

I can provide any logs if necessary.

                === gdb Summary ===

# of expected passes            31937
# of unexpected failures        61
# of unexpected successes       4
# of expected failures          24
# of known failures             27
# of unresolved testcases       2
# of untested testcases         81
# of unsupported tests          81
# of duplicate test names       68

Failures:
FAIL: gdb.base/step-over-no-symbols.exp: displaced=off: advanced
FAIL: gdb.base/charset.exp: parse character literal in UTF-32
FAIL: gdb.base/charset.exp: check value of parsed character literal in UTF-32
FAIL: gdb.base/charset.exp: check value of '\a' in UTF-32
FAIL: gdb.base/charset.exp: check value of '\b' in UTF-32
FAIL: gdb.base/charset.exp: check value of '\f' in UTF-32
FAIL: gdb.base/charset.exp: check value of '\n' in UTF-32
FAIL: gdb.base/charset.exp: check value of '\r' in UTF-32
FAIL: gdb.base/charset.exp: check value of '\t' in UTF-32
FAIL: gdb.base/charset.exp: check value of '\v' in UTF-32
FAIL: gdb.base/charset.exp: print escape that doesn't exist in UTF-32
FAIL: gdb.base/charset.exp: check value of escape that doesn't exist in UTF-32
FAIL: gdb.base/charset.exp: basic wide character
FAIL: gdb.base/charset.exp: set_prefix=L: display String String32 with x/ws
FAIL: gdb.base/index-cache.exp: test_cache_enabled_miss: expected file is there
FAIL: gdb.base/index-cache.exp: test_cache_enabled_miss: check index-cache stats
FAIL: gdb.base/index-cache.exp: test_cache_enabled_hit: no files were created
FAIL: gdb.base/index-cache.exp: test_cache_enabled_hit: check index-cache stats
FAIL: gdb.base/exprs.exp: truncate (void*) 0x00000000ffffffff + 1
FAIL: gdb.base/exprs.exp: truncate (void*) 0xffffffff00000000 - 1
FAIL: gdb.base/ui-redirect.exp: redirect: set logging active on (timeout)
FAIL: gdb.base/ui-redirect.exp: redirect: save breakpoints cmds.txt (timeout)
FAIL: gdb.base/ui-redirect.exp: redirect: userdefined (timeout)
FAIL: gdb.base/float128.exp: print large128 (GDB may be missing MPFR support!)
FAIL: gdb.base/dmsym.exp: info line test_minsym
FAIL: gdb.base/long_long.exp: p/a *(long *)l
FAIL: gdb.base/wchar.exp: print single
FAIL: gdb.base/wchar.exp: print difficile
FAIL: gdb.base/wchar.exp: print difficile[2]
FAIL: gdb.base/wchar.exp: print repeat
FAIL: gdb.base/wchar.exp: print repeat_p
FAIL: gdb.base/wchar.exp: print repeat (print null on)
FAIL: gdb.base/wchar.exp: print repeat (print elements 3)
FAIL: gdb.base/wchar.exp: print repeat_p (print elements 3)
FAIL: gdb.base/longjmp.exp: next over call_longjmp (2)
FAIL: gdb.base/trace-commands.exp: nested trace-commands test (pattern 2)
FAIL: gdb.base/trace-commands.exp: depth resets on error part 1 (pattern 2)
FAIL: gdb.base/sepdebug.exp: CRC mismatch is reported
FAIL: gdb.base/max-depth-c++.exp: exp='v2': depth=1: p v2
FAIL: gdb.base/max-depth-c++.exp: exp='v2': depth=2: p v2
FAIL: gdb.base/max-depth-c++.exp: exp='v2': depth=unlimited: p v2
FAIL: gdb.base/max-depth-c++.exp: exp='v3': depth=1: p v3
FAIL: gdb.base/max-depth-c++.exp: exp='v3': depth=2: p v3
FAIL: gdb.base/max-depth-c++.exp: exp='v3': depth=unlimited: p v3
FAIL: gdb.base/max-depth-c++.exp: exp='v4': depth=2: p v4
FAIL: gdb.base/max-depth-c++.exp: exp='v4': depth=3: p v4
FAIL: gdb.base/max-depth-c++.exp: exp='v4': depth=unlimited: p v4
FAIL: gdb.base/max-depth-c++.exp: exp='v5': depth=2: p v5
FAIL: gdb.base/max-depth-c++.exp: exp='v5': depth=3: p v5
FAIL: gdb.base/max-depth-c++.exp: exp='v5': depth=unlimited: p v5
FAIL: gdb.base/max-depth-c++.exp: exp='v6': depth=2: p v6
FAIL: gdb.base/max-depth-c++.exp: exp='v6': depth=3: p v6
FAIL: gdb.base/max-depth-c++.exp: exp='v6': depth=unlimited: p v6
FAIL: gdb.base/max-depth-c++.exp: exp='v7': depth=3: p v7
FAIL: gdb.base/max-depth-c++.exp: exp='v7': depth=4: p v7
FAIL: gdb.base/max-depth-c++.exp: exp='v7': depth=unlimited: p v7
FAIL: gdb.base/write_mem.exp: x /xh main
FAIL: gdb.base/step-indirect-call-thunk.exp: stepi into call thunk
FAIL: gdb.base/step-indirect-call-thunk.exp: stepi into return thunk
FAIL: gdb.base/step-indirect-call-thunk.exp: stepi out of return thunk back into thrice
FAIL: gdb.base/with.exp: user-defined: define usercmd (timeout)

Unexpected passes:
XPASS: gdb.base/sepdebug.exp: build-id: breakpoint function, optimized file
XPASS: gdb.base/sepdebug.exp: build-id: breakpoint small function, optimized file
XPASS: gdb.base/sepdebug.exp: build-id: run until function breakpoint, optimized file
XPASS: gdb.base/sepdebug.exp: build-id: run until breakpoint set at small function, optimized file (line bp_location14)


PS: It takes 40 minutes to run all tests of gdb.base, how does this
compare to Linux?


Hannes

[PATCH 1/7] Fix function argument and return value locations

[Hannes Domani]

Fixes these testsuite fails on Windows:
FAIL: gdb.base/callfuncs.exp: p t_float_complex_values(fc1, fc2)
FAIL: gdb.base/callfuncs.exp: p t_float_complex_many_args(fc1, fc2, fc3, fc4, fc1, fc2, fc3, fc4, fc1, fc2, fc3, fc4, fc1, fc2, fc3, fc4)
FAIL: gdb.base/callfuncs.exp: noproto: p t_float_complex_values(fc1, fc2)
FAIL: gdb.base/callfuncs.exp: noproto: p t_float_complex_many_args(fc1, fc2, fc3, fc4, fc1, fc2, fc3, fc4, fc1, fc2, fc3, fc4, fc1, fc2, fc3, fc4)
FAIL: gdb.base/call-sc.exp: p/c fun(); call call-sc-tld
FAIL: gdb.base/call-sc.exp: advance to fun for return; return call-sc-tld
FAIL: gdb.base/call-sc.exp: zed L for return; return call-sc-tld
FAIL: gdb.base/call-sc.exp: return foo; return call-sc-tld
FAIL: gdb.base/call-sc.exp: return foo; synchronize pc to main() for 'call-sc-tld'
FAIL: gdb.base/call-sc.exp: return foo; synchronize pc to main() for 'call-sc-tld'
FAIL: gdb.base/call-sc.exp: advance to fun for finish; return call-sc-tld
FAIL: gdb.base/call-sc.exp: zed L for finish; return call-sc-tld
FAIL: gdb.base/call-sc.exp: finish foo; return call-sc-tld (the program is no longer running)
FAIL: gdb.base/call-sc.exp: value foo finished; return call-sc-tld

For function arguments (callfuncs.exp), only TYPE_CODE_COMPLEX was
missing in the types passed via XMM registers.

For return values, there were a lot more issues:
- TYPE_CODE_DECFLOAT is NOT returned via XMM0.
- long double is NOT returned via XMM0.
- but __int128 IS returned via XMM0.
- the comments for TYPE_CODE_FLT state that __m128, __m128i and __m128d are
  returned by XMM0, and this is correct, but it doesn't actually check for
  them, because they are TYPE_CODE_ARRAY with TYPE_VECTOR

So I had to add TYPE_CODE_DECFLOAT to the arguments passed via XMM register,
but I had to remove it from the values returned via XMM0 register.

gdb/ChangeLog:

2020-05-25  Hannes Domani  <ssbssa@yahoo.de>

	* amd64-windows-tdep.c (amd64_windows_passed_by_xmm_register):
	Add TYPE_CODE_COMPLEX.
	(amd64_windows_return_value): Fix types returned via XMM0.

gdb/testsuite/ChangeLog:

2020-05-25  Hannes Domani  <ssbssa@yahoo.de>

	* gdb.base/call-sc.c: Fix return struct on stack test case.
	* gdb.base/call-sc.exp: Likewise.
---
 gdb/amd64-windows-tdep.c           | 23 ++++++++++++++++++-----
 gdb/testsuite/gdb.base/call-sc.c   |  6 +++++-
 gdb/testsuite/gdb.base/call-sc.exp | 13 +++++++++++++
 3 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/gdb/amd64-windows-tdep.c b/gdb/amd64-windows-tdep.c
index 487dfd45fc..db9845203f 100644
--- a/gdb/amd64-windows-tdep.c
+++ b/gdb/amd64-windows-tdep.c
@@ -77,7 +77,8 @@ static int
 amd64_windows_passed_by_xmm_register (struct type *type)
 {
   return ((type->code () == TYPE_CODE_FLT
-	   || type->code () == TYPE_CODE_DECFLOAT)
+	   || type->code () == TYPE_CODE_DECFLOAT
+	   || type->code () == TYPE_CODE_COMPLEX)
           && (TYPE_LENGTH (type) == 4 || TYPE_LENGTH (type) == 8));
 }
 
@@ -299,17 +300,29 @@ amd64_windows_return_value (struct gdbarch *gdbarch, struct value *function,
   switch (type->code ())
     {
       case TYPE_CODE_FLT:
-      case TYPE_CODE_DECFLOAT:
-        /* __m128, __m128i, __m128d, floats, and doubles are returned
-           via XMM0.  */
-        if (len == 4 || len == 8 || len == 16)
+        /* floats, and doubles are returned via XMM0.  */
+        if (len == 4 || len == 8)
           regnum = AMD64_XMM0_REGNUM;
         break;
+      case TYPE_CODE_ARRAY:
+        /* __m128, __m128i and __m128d are returned via XMM0.  */
+	if (TYPE_VECTOR (type) && len == 16)
+	  {
+	    enum type_code code = TYPE_TARGET_TYPE (type)->code ();
+	    if (code == TYPE_CODE_INT || code == TYPE_CODE_FLT)
+	      {
+		regnum = AMD64_XMM0_REGNUM;
+		break;
+	      }
+	  }
+	/* fall through */
       default:
         /* All other values that are 1, 2, 4 or 8 bytes long are returned
            via RAX.  */
         if (len == 1 || len == 2 || len == 4 || len == 8)
           regnum = AMD64_RAX_REGNUM;
+	else if (len == 16 && type->code () == TYPE_CODE_INT)
+	  regnum = AMD64_XMM0_REGNUM;
         break;
     }
 
diff --git a/gdb/testsuite/gdb.base/call-sc.c b/gdb/testsuite/gdb.base/call-sc.c
index ba80576ac3..eb140cd9cf 100644
--- a/gdb/testsuite/gdb.base/call-sc.c
+++ b/gdb/testsuite/gdb.base/call-sc.c
@@ -35,6 +35,7 @@ typedef t T;
 #endif
 
 T foo = '1', L;
+T init = '9';
 
 T fun()
 {
@@ -55,7 +56,10 @@ int main()
 {
   int i;
 
-  Fun(foo);	
+  /* Use a different initial value then is later used in the
+     "value foo returned" test, so in case the struct is then returned
+     on the stack, it doesn't have the correct value by accident.  */
+  Fun(init);
 
   /* An infinite loop that first clears all the variables and then
      calls the function.  This "hack" is to make re-testing easier -
diff --git a/gdb/testsuite/gdb.base/call-sc.exp b/gdb/testsuite/gdb.base/call-sc.exp
index 719000435a..9697c5ac24 100644
--- a/gdb/testsuite/gdb.base/call-sc.exp
+++ b/gdb/testsuite/gdb.base/call-sc.exp
@@ -291,6 +291,19 @@ proc test_scalar_returns { } {
 		fail "${test}"
 	    }
 	}
+	-re " = 57 .*${gdb_prompt} $" {
+	    if $return_value_unknown {
+		# The struct return case.
+		# The return value is stored on the stack, and since GDB
+		# didn't override it, it still has value that was stored
+		# there in the earlier Foo(init) call.
+		pass "${test}"
+	    } else {
+		# This contradicts the above claim that GDB knew
+		# the location of the return-value.
+		fail "${test}"
+	    }
+	}
 	-re ".*${gdb_prompt} $" {
 	    if $return_value_unimplemented {
 		# What a suprize.  The architecture hasn't implemented
-- 
2.26.2

[PATCH 2/7] Handle Windows drives in auto-load script paths

[Hannes Domani]

Fixes this testsuite fail on Windows:
FAIL: gdb.base/auto-load.exp: print $script_loaded

Converts the debugfile path from c:/dir/file to /c/dir/file, so it can be
appended to the auto-load path.

gdb/ChangeLog:

2020-05-25  Hannes Domani  <ssbssa@yahoo.de>

	* auto-load.c (auto_load_objfile_script_1): Convert drive part
	of debugfile path on Windows.
---
 gdb/auto-load.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/gdb/auto-load.c b/gdb/auto-load.c
index 99bd96b971..88221d9f3d 100644
--- a/gdb/auto-load.c
+++ b/gdb/auto-load.c
@@ -784,6 +784,13 @@ auto_load_objfile_script_1 (struct objfile *objfile, const char *realname,
 					  "scripts-directory' path \"%s\".\n"),
 			    auto_load_dir);
 
+      /* Convert Windows debugfile path from c:/dir/file to /c/dir/file.  */
+      if (HAS_DRIVE_SPEC (debugfile))
+	{
+	  debugfile_holder = STRIP_DRIVE_SPEC (debugfile);
+	  filename = std::string("/") + debugfile[0] + debugfile_holder;
+	}
+
       for (const gdb::unique_xmalloc_ptr<char> &dir : vec)
 	{
 	  /* FILENAME is absolute, so we don't need a "/" here.  */
-- 
2.26.2

[PATCH 3/7] Handle Windows drives in rbreak paths

[Hannes Domani]

Fixes this testsuite fail on Windows:
FAIL: gdb.base/fullpath-expand.exp: rbreak XXX/fullpath-expand-func.c:func

If the found colon is actually part of a Windows drive, look for another.

gdb/ChangeLog:

2020-05-25  Hannes Domani  <ssbssa@yahoo.de>

	* symtab.c (rbreak_command): Ignore Windows drive colon.
---
 gdb/symtab.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index dc771c5679..d399ccef9b 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -5210,6 +5210,11 @@ rbreak_command (const char *regexp, int from_tty)
     {
       const char *colon = strchr (regexp, ':');
 
+      /* Ignore the colon if it is part of a Windows drive.  */
+      if (HAS_DRIVE_SPEC (regexp)
+	  && (regexp[2] == '/' || regexp[2] == '\\'))
+	colon = strchr (STRIP_DRIVE_SPEC (regexp), ':');
+
       if (colon && *(colon + 1) != ':')
 	{
 	  int colon_index;
-- 
2.26.2

[PATCH 4/7] Use errno value of first openp failure

[Hannes Domani]

Fixes this testsuite fail on Windows:
FAIL: gdb.base/bad-file.exp: directory

If both tries to open the file fail (without and with ".exe"), use the
errno value of the first try.

gdb/ChangeLog:

2020-05-25  Hannes Domani  <ssbssa@yahoo.de>

	* exec.c (exec_file_attach): Use errno value of first openp failure.
---
 gdb/exec.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/gdb/exec.c b/gdb/exec.c
index 14c77495a3..ee13c5e027 100644
--- a/gdb/exec.c
+++ b/gdb/exec.c
@@ -435,6 +435,7 @@ exec_file_attach (const char *filename, int from_tty)
 #if defined(__GO32__) || defined(_WIN32) || defined(__CYGWIN__)
 	  if (scratch_chan < 0)
 	    {
+	      int first_errno = errno;
 	      char *exename = (char *) alloca (strlen (filename) + 5);
 
 	      strcat (strcpy (exename, filename), ".exe");
@@ -443,6 +444,8 @@ exec_file_attach (const char *filename, int from_tty)
 				    O_RDWR | O_BINARY
 				    : O_RDONLY | O_BINARY,
 				    &scratch_storage);
+	      if (scratch_chan < 0)
+		errno = first_errno;
 	    }
 #endif
 	  if (scratch_chan < 0)
-- 
2.26.2

[PATCH 5/7] Close file handle of empty history file

[Hannes Domani]

Happened while trying to reproduce a gdb.base/gdbinit-history.exp failure:
warning: Could not rename C:/gdb/build64/gdb-git/gdb/testsuite/outputs/gdb.base/gdbinit-history/gdbinit-history.gdb_history to C:/gdb/build64/gdb-git/gdb/testsuite/outputs/gdb.base/gdbinit-history/gdbinit-history.gdb_history-gdb5228~: Permission denied

I had an empty gdbinit-history.gdb_history-gdb5228~ file, and the file
handle was not closed on startup, so it couldn't rename it at the end
when trying to write a new one.

readline/readline/ChangeLog:

2020-05-25  Hannes Domani  <ssbssa@yahoo.de>

	* histfile.c (read_history_range): Close file handle.
---
 readline/readline/histfile.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/readline/readline/histfile.c b/readline/readline/histfile.c
index dc64bde1c5..a8a92aa360 100644
--- a/readline/readline/histfile.c
+++ b/readline/readline/histfile.c
@@ -305,6 +305,7 @@ read_history_range (const char *filename, int from, int to)
   if (file_size == 0)
     {
       free (input);
+      close (file);
       return 0;	/* don't waste time if we don't have to */
     }
 
-- 
2.26.2

[PATCH 6/7] Move exit_status_set_internal_vars out of GLOBAL_CURDIR

[Hannes Domani]

Fixes these testsuite fails on Windows:
FAIL: gdb.base/shell.exp: shell success exitcode
FAIL: gdb.base/shell.exp: shell fail exitcode

The convenience variables $_shell_exitcode and $_shell_exitsignal don't
depend on the GLOBAL_CURDIR define.

gdb/ChangeLog:

2020-05-25  Hannes Domani  <ssbssa@yahoo.de>

	* cli/cli-cmds.c (shell_escape): Move exit_status_set_internal_vars.
---
 gdb/cli/cli-cmds.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
index eb6e32b046..7e657c2e76 100644
--- a/gdb/cli/cli-cmds.c
+++ b/gdb/cli/cli-cmds.c
@@ -830,8 +830,8 @@ shell_escape (const char *arg, int from_tty)
   /* Make sure to return to the directory GDB thinks it is, in case
      the shell command we just ran changed it.  */
   chdir (current_directory);
-  exit_status_set_internal_vars (rc);
 #endif
+  exit_status_set_internal_vars (rc);
 #else /* Can fork.  */
   int status, pid;
 
-- 
2.26.2

[PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point

[Hannes Domani]

Fixes these testsuite fails on Windows (actually more, but the others are
cascading failures):
FAIL: gdb.base/hbreak2.exp: hardware breakpoint insertion (the program exited)
FAIL: gdb.base/hbreak2.exp: run until function breakpoint (the program exited)
FAIL: gdb.base/hbreak2.exp: run to factorial(6) (the program exited)
FAIL: gdb.base/hbreak2.exp: run until hardware function breakpoint, optimized file (the program exited)

The problem happens if you only have hardware breakpoints active when
(re-)starting the program:

(gdb) start
Temporary breakpoint 1 at 0x401650: file C:/src/repos/binutils-gdb.git/gdb/tests
uite/gdb.base/break.c, line 43.
Starting program: C:\gdb\build64\gdb-git\gdb\testsuite\outputs\gdb.base\hbreak2\
hbreak2.exe

Temporary breakpoint 1, main (argc=1, argv=0x7e2120, envp=0x7e2900)
    at C:/src/repos/binutils-gdb.git/gdb/testsuite/gdb.base/break.c:43
43          if (argc == 12345) {  /* an unlikely value < 2^16, in case uninited
*/ /* set breakpoint 6 here */
(gdb) hb factorial
Hardware assisted breakpoint 2 at 0x401703: file C:/src/repos/binutils-gdb.git/g
db/testsuite/gdb.base/break.c, line 63.
(gdb) r
The program being debugged has been started already.
Start it from the beginning? (y or n) y
Starting program: C:\gdb\build64\gdb-git\gdb\testsuite\outputs\gdb.base\hbreak2\
hbreak2.exe
720
[Inferior 1 (process 7836) exited normally]

But if you stopped just once before reaching the hardware breakpoint, it
works fine:

(gdb) start
Temporary breakpoint 3 at 0x401650: file C:/src/repos/binutils-gdb.git/gdb/tests
uite/gdb.base/break.c, line 43.
Starting program: C:\gdb\build64\gdb-git\gdb\testsuite\outputs\gdb.base\hbreak2\
hbreak2.exe

Temporary breakpoint 3, main (argc=1, argv=0x322120, envp=0x322900)
    at C:/src/repos/binutils-gdb.git/gdb/testsuite/gdb.base/break.c:43
43          if (argc == 12345) {  /* an unlikely value < 2^16, in case uninited
*/ /* set breakpoint 6 here */
(gdb) c
Continuing.

Breakpoint 2, factorial (value=6)
    at C:/src/repos/binutils-gdb.git/gdb/testsuite/gdb.base/break.c:63
63        if (value > 1) {  /* set breakpoint 7 here */

I found out that cdb writes this error when trying to do the same:

Unable to set breakpoint error
The system resets thread contexts after the process
breakpoint so hardware breakpoints cannot be set.
Go to the executable's entry point and set it then.

So all the hardware breakpoints that were set before (or rather, their
debug register information) are practically lost when the process entry
point is reached.

This patch creates an internal breakpoint on the process entry point, which
when it is reached, resets all active hardware breakpoints, and continues
execution.
---
 gdb/windows-tdep.c | 130 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 130 insertions(+)

diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c
index aa0adeba99..90e4794fc5 100644
--- a/gdb/windows-tdep.c
+++ b/gdb/windows-tdep.c
@@ -37,6 +37,7 @@
 #include "coff/internal.h"
 #include "libcoff.h"
 #include "solist.h"
+#include "observable.h"
 
 #define CYGWIN_DLL_NAME "cygwin1.dll"
 
@@ -870,6 +871,99 @@ windows_get_siginfo_type (struct gdbarch *gdbarch)
   return siginfo_type;
 }
 
+/* Windows-specific cached data.  This is used by GDB for caching
+   purposes for each inferior.  This helps reduce the overhead of
+   transfering data from a remote target to the local host.  */
+struct windows_info
+{
+  CORE_ADDR entry_point = 0;
+};
+
+/* Per-inferior data key.  */
+static const struct inferior_key<windows_info> windows_inferior_data;
+
+/* Frees whatever allocated space there is to be freed and sets INF's
+   Windows cache data pointer to NULL.  */
+
+static void
+invalidate_windows_cache_inf (struct inferior *inf)
+{
+  windows_inferior_data.clear (inf);
+}
+
+/* Fetch the Windows cache info for INF.  This function always returns a
+   valid INFO pointer.  */
+
+static struct windows_info *
+get_windows_inferior_data (void)
+{
+  struct windows_info *info;
+  struct inferior *inf = current_inferior ();
+
+  info = windows_inferior_data.get (inf);
+  if (info == NULL)
+    info = windows_inferior_data.emplace (inf);
+
+  return info;
+}
+
+/* Breakpoint on entry point where any active hardware breakpoints will
+   be reset.  */
+static struct breakpoint_ops entry_point_breakpoint_ops;
+
+/* Reset active hardware breakpoints.  */
+
+static bool
+reset_hardware_breakpoints (struct breakpoint *b)
+{
+  if (b->type != bp_hardware_breakpoint
+      && b->type != bp_hardware_watchpoint
+      && b->type != bp_read_watchpoint
+      && b->type != bp_access_watchpoint)
+    return false;
+
+  struct bp_location *loc;
+  for (loc = b->loc; loc; loc = loc->next)
+    if (loc->enabled && loc->pspace == current_program_space
+	&& b->ops->remove_location (loc, REMOVE_BREAKPOINT) == 0)
+      b->ops->insert_location (loc);
+
+  return false;
+}
+
+/* This breakpoint type should never stop, but when reached, reset
+   the active hardware breakpoints.  */
+
+static void
+startup_breakpoint_check_status (bpstat bs)
+{
+  /* Never stop.  */
+  bs->stop = 0;
+
+  iterate_over_breakpoints (reset_hardware_breakpoints);
+}
+
+/* Update the breakpoint location to the current entry point.  */
+
+static void
+startup_breakpoint_re_set (struct breakpoint *b)
+{
+  struct windows_info *info = get_windows_inferior_data ();
+  CORE_ADDR entry_point = info->entry_point;
+
+  /* Do nothing if the entry point didn't change.  */
+  struct bp_location *loc;
+  for (loc = b->loc; loc; loc = loc->next)
+    if (loc->pspace == current_program_space && loc->address == entry_point)
+      return;
+
+  event_location_up location
+    = new_address_location (entry_point, nullptr, 0);
+  std::vector<symtab_and_line> sals;
+  sals = b->ops->decode_location (b, location.get (), current_program_space);
+  update_breakpoint_locations (b, current_program_space, sals, {});
+}
+
 /* Implement the "solib_create_inferior_hook" target_so_ops method.  */
 
 static void
@@ -914,6 +1008,30 @@ windows_solib_create_inferior_hook (int from_tty)
       if (vmaddr != exec_base)
 	objfile_rebase (symfile_objfile, exec_base - vmaddr);
     }
+
+  /* Create the entry point breakpoint if it doesn't exist already.  */
+  if (target_has_execution && exec_base != 0)
+    {
+      struct windows_info *info = get_windows_inferior_data ();
+      CORE_ADDR entry_point = exec_base
+	+ pe_data (exec_bfd)->pe_opthdr.AddressOfEntryPoint;
+      info->entry_point = entry_point;
+
+      breakpoint *startup_breakpoint
+	= iterate_over_breakpoints ([] (breakpoint *bp)
+	  {
+	    return bp->ops == &entry_point_breakpoint_ops;
+	  });
+      if (startup_breakpoint == nullptr)
+	{
+	  event_location_up location
+	    = new_address_location (entry_point, nullptr, 0);
+	  create_breakpoint (target_gdbarch(), location.get(), nullptr, -1,
+			     nullptr, 0, 1, bp_breakpoint, 0,
+			     AUTO_BOOLEAN_FALSE, &entry_point_breakpoint_ops,
+			     0, 1, 1, 0);
+	}
+    }
 }
 
 static struct target_so_ops windows_so_ops;
@@ -1095,6 +1213,18 @@ _initialize_windows_tdep ()
   windows_gdbarch_data_handle
     = gdbarch_data_register_post_init (init_windows_gdbarch_data);
 
+  /* Observers used to invalidate the cache when needed.  */
+  gdb::observers::inferior_exit.attach (invalidate_windows_cache_inf);
+  gdb::observers::inferior_appeared.attach (invalidate_windows_cache_inf);
+
+  initialize_breakpoint_ops ();
+  /* Entry point breakpoint.  */
+  entry_point_breakpoint_ops = bkpt_breakpoint_ops;
+  entry_point_breakpoint_ops.check_status
+    = startup_breakpoint_check_status;
+  entry_point_breakpoint_ops.re_set
+    = startup_breakpoint_re_set;
+
   init_w32_command_list ();
   add_cmd ("thread-information-block", class_info, display_tib,
 	   _("Display thread information block."),
-- 
2.26.2

Re: [PATCH 1/7] Fix function argument and return value locations

[Simon Marchi]

On 2020-05-25 2:56 p.m., Hannes Domani via Gdb-patches wrote:
> diff --git a/gdb/amd64-windows-tdep.c b/gdb/amd64-windows-tdep.c
> index 487dfd45fc..db9845203f 100644
> --- a/gdb/amd64-windows-tdep.c
> +++ b/gdb/amd64-windows-tdep.c
> @@ -77,7 +77,8 @@ static int
>  amd64_windows_passed_by_xmm_register (struct type *type)
>  {
>    return ((type->code () == TYPE_CODE_FLT
> -	   || type->code () == TYPE_CODE_DECFLOAT)
> +	   || type->code () == TYPE_CODE_DECFLOAT
> +	   || type->code () == TYPE_CODE_COMPLEX)
>            && (TYPE_LENGTH (type) == 4 || TYPE_LENGTH (type) == 8));
>  }

I don't know much about ABIs, so I tried:

$ cat hello.c
#include <complex.h>

void other(int real, int imag);
void func (complex int n)
{
  other(creal(n), cimag(n));
}
$ x86_64-w64-mingw32-gcc hello.c -g3 -O0 -c
$ x86_64-w64-mingw32-objdump -d allo.o

allo.o:     file format elf64-x86-64


Disassembly of section .text:

0000000000000000 <func>:
   0:   55                      push   %rbp
   1:   48 89 e5                mov    %rsp,%rbp
   4:   48 83 ec 10             sub    $0x10,%rsp
   8:   48 89 7d f8             mov    %rdi,-0x8(%rbp)
   c:   8b 45 fc                mov    -0x4(%rbp),%eax
   f:   66 0f ef c0             pxor   %xmm0,%xmm0
  13:   f2 0f 2a c0             cvtsi2sd %eax,%xmm0
  17:   f2 0f 2c d0             cvttsd2si %xmm0,%edx
  1b:   8b 45 f8                mov    -0x8(%rbp),%eax
  1e:   66 0f ef c0             pxor   %xmm0,%xmm0
  22:   f2 0f 2a c0             cvtsi2sd %eax,%xmm0
  26:   f2 0f 2c c0             cvttsd2si %xmm0,%eax
  2a:   89 d6                   mov    %edx,%esi
  2c:   89 c7                   mov    %eax,%edi
  2e:   e8 00 00 00 00          callq  33 <func+0x33>
  33:   90                      nop
  34:   c9                      leaveq
  35:   c3                      retq


Doesn't this show that a `complex int` argument is passed through the rdi
register?  Am I missing something here?

Simon

Re: [PATCH 1/7] Fix function argument and return value locations

[Hannes Domani]

 Am Montag, 25. Mai 2020, 23:02:37 MESZ hat Simon Marchi <simark@simark.ca> Folgendes geschrieben:

> On 2020-05-25 2:56 p.m., Hannes Domani via Gdb-patches wrote:
>
> > diff --git a/gdb/amd64-windows-tdep.c b/gdb/amd64-windows-tdep.c
> > index 487dfd45fc..db9845203f 100644
> > --- a/gdb/amd64-windows-tdep.c
> > +++ b/gdb/amd64-windows-tdep.c
> > @@ -77,7 +77,8 @@ static int
> >  amd64_windows_passed_by_xmm_register (struct type *type)
> >  {
> >    return ((type->code () == TYPE_CODE_FLT
> > -      || type->code () == TYPE_CODE_DECFLOAT)
> > +      || type->code () == TYPE_CODE_DECFLOAT
> > +      || type->code () == TYPE_CODE_COMPLEX)
> >            && (TYPE_LENGTH (type) == 4 || TYPE_LENGTH (type) == 8));
>
> >  }
>
> I don't know much about ABIs, so I tried:
>
> $ cat hello.c
> #include <complex.h>
>
> void other(int real, int imag);
> void func (complex int n)
> {
>   other(creal(n), cimag(n));
> }
> $ x86_64-w64-mingw32-gcc hello.c -g3 -O0 -c
> $ x86_64-w64-mingw32-objdump -d allo.o
>
> allo.o:    file format elf64-x86-64
>
>
> Disassembly of section .text:
>
> 0000000000000000 <func>:
>   0:  55                      push  %rbp
>   1:  48 89 e5                mov    %rsp,%rbp
>   4:  48 83 ec 10            sub    $0x10,%rsp
>   8:  48 89 7d f8            mov    %rdi,-0x8(%rbp)
>   c:  8b 45 fc                mov    -0x4(%rbp),%eax
>   f:  66 0f ef c0            pxor  %xmm0,%xmm0
>   13:  f2 0f 2a c0            cvtsi2sd %eax,%xmm0
>   17:  f2 0f 2c d0            cvttsd2si %xmm0,%edx
>   1b:  8b 45 f8                mov    -0x8(%rbp),%eax
>   1e:  66 0f ef c0            pxor  %xmm0,%xmm0
>   22:  f2 0f 2a c0            cvtsi2sd %eax,%xmm0
>   26:  f2 0f 2c c0            cvttsd2si %xmm0,%eax
>   2a:  89 d6                  mov    %edx,%esi
>   2c:  89 c7                  mov    %eax,%edi
>   2e:  e8 00 00 00 00          callq  33 <func+0x33>
>   33:  90                      nop
>   34:  c9                      leaveq
>   35:  c3                      retq
>
>
> Doesn't this show that a `complex int` argument is passed through the rdi
> register?  Am I missing something here?

You're probably right, the thing is, I was only able to test complex float
and complex double, because gdb doesn't like complex integral types:

complex int complex_int = 5 + 6i;

(gdb) p complex_int
'complex_int' has unknown type; cast it to its declared type
(gdb) pt complex_int
'complex_int' has unknown type; cast it to its declared type

So I guess it should check for target-type float as well:
       || (type->code () == TYPE_CODE_COMPLEX
           && TYPE_TARGET_TYPE (type)->code () == TYPE_CODE_FLT))

Do many people use complex int, because I personally wouldn't have expected
that this even exists.


Hannes

Re: [PATCH 1/7] Fix function argument and return value locations

[Simon Marchi]

On 2020-05-25 5:32 p.m., Hannes Domani via Gdb-patches wrote:
> You're probably right, the thing is, I was only able to test complex float
> and complex double, because gdb doesn't like complex integral types:
> 
> complex int complex_int = 5 + 6i;
> 
> (gdb) p complex_int
> 'complex_int' has unknown type; cast it to its declared type
> (gdb) pt complex_int
> 'complex_int' has unknown type; cast it to its declared type
> 
> So I guess it should check for target-type float as well:
>        || (type->code () == TYPE_CODE_COMPLEX
>            && TYPE_TARGET_TYPE (type)->code () == TYPE_CODE_FLT))
> 
> Do many people use complex int, because I personally wouldn't have expected
> that this even exists.

Err right that doesn't make sense, let's use floats instead.  I see:


$ cat hello.c
#include <complex.h>

void other(float real, float imag);
void func (complex float n)
{
  other(creal(n), cimag(n));
}
$ x86_64-w64-mingw32-gcc hello.c -g3 -O0 -c
$ x86_64-w64-mingw32-objdump -d hello.o

hello.o:     file format pe-x86-64


Disassembly of section .text:

0000000000000000 <func>:
   0:   55                      push   %rbp
   1:   48 89 e5                mov    %rsp,%rbp
   4:   48 83 ec 20             sub    $0x20,%rsp
   8:   48 89 4d 10             mov    %rcx,0x10(%rbp)
   c:   f3 0f 10 45 14          movss  0x14(%rbp),%xmm0
  11:   f3 0f 5a c0             cvtss2sd %xmm0,%xmm0
  15:   f2 0f 5a c8             cvtsd2ss %xmm0,%xmm1
  19:   f3 0f 10 45 10          movss  0x10(%rbp),%xmm0
  1e:   f3 0f 5a c0             cvtss2sd %xmm0,%xmm0
  22:   f2 0f 5a c0             cvtsd2ss %xmm0,%xmm0
  26:   e8 00 00 00 00          callq  2b <func+0x2b>
  2b:   90                      nop
  2c:   48 83 c4 20             add    $0x20,%rsp
  30:   5d                      pop    %rbp
  31:   c3                      retq

Doesn't this suggest that the parameter gets passed through rcx?  I'm not saying you
are wrong, I'm just trying to understand how things work :).

Simon

Re: [PATCH 1/7] Fix function argument and return value locations

[Hannes Domani]

 Am Dienstag, 26. Mai 2020, 00:14:36 MESZ hat Simon Marchi <simark@simark.ca> Folgendes geschrieben:

> On 2020-05-25 5:32 p.m., Hannes Domani via Gdb-patches wrote:
> > You're probably right, the thing is, I was only able to test complex float
> > and complex double, because gdb doesn't like complex integral types:
> >
> > complex int complex_int = 5 + 6i;
> >
> > (gdb) p complex_int
> > 'complex_int' has unknown type; cast it to its declared type
> > (gdb) pt complex_int
> > 'complex_int' has unknown type; cast it to its declared type
> >
> > So I guess it should check for target-type float as well:
> >        || (type->code () == TYPE_CODE_COMPLEX
> >            && TYPE_TARGET_TYPE (type)->code () == TYPE_CODE_FLT))
> >
> > Do many people use complex int, because I personally wouldn't have expected
> > that this even exists.
>
> Err right that doesn't make sense, let's use floats instead.  I see:
>
>
> $ cat hello.c
> #include <complex.h>
>
> void other(float real, float imag);
> void func (complex float n)
> {
>   other(creal(n), cimag(n));
> }
> $ x86_64-w64-mingw32-gcc hello.c -g3 -O0 -c
> $ x86_64-w64-mingw32-objdump -d hello.o
>
> hello.o:    file format pe-x86-64
>
>
> Disassembly of section .text:
>
> 0000000000000000 <func>:
>   0:  55                      push  %rbp
>   1:  48 89 e5                mov    %rsp,%rbp
>   4:  48 83 ec 20            sub    $0x20,%rsp
>   8:  48 89 4d 10            mov    %rcx,0x10(%rbp)
>   c:  f3 0f 10 45 14          movss  0x14(%rbp),%xmm0
>   11:  f3 0f 5a c0            cvtss2sd %xmm0,%xmm0
>   15:  f2 0f 5a c8            cvtsd2ss %xmm0,%xmm1
>   19:  f3 0f 10 45 10          movss  0x10(%rbp),%xmm0
>   1e:  f3 0f 5a c0            cvtss2sd %xmm0,%xmm0
>   22:  f2 0f 5a c0            cvtsd2ss %xmm0,%xmm0
>   26:  e8 00 00 00 00          callq  2b <func+0x2b>
>   2b:  90                      nop
>   2c:  48 83 c4 20            add    $0x20,%rsp
>   30:  5d                      pop    %rbp
>   31:  c3                      retq
>
> Doesn't this suggest that the parameter gets passed through rcx?  I'm not saying you
> are wrong, I'm just trying to understand how things work :).

You're absolutely right again.
The complex arguments didn't work before, so I just tried it with
amd64_windows_passed_by_xmm_register (because that seemed the obvious choice
for me), and then it worked.

But it only worked because in case of XMM register, the argument is also
passed via the integer register:

      else if (amd64_windows_passed_by_xmm_register (type))
        {
          amd64_windows_store_arg_in_reg
            (regcache, args[i], AMD64_XMM0_REGNUM + reg_idx);
          /* In case of varargs, these parameters must also be
         passed via the integer registers.  */
          amd64_windows_store_arg_in_reg
        (regcache, args[i],
         amd64_windows_dummy_call_integer_regs[reg_idx]);
          on_stack_p = 0;
          reg_idx++;
        }

So it actually should be added to amd64_windows_passed_by_integer_register,
and a quick test just now confirms that this also works.

I'm very sorry about that, I should have checked the asm code.


Hannes

Re: [PATCH 2/7] Handle Windows drives in auto-load script paths

[Jon Turney]

On 25/05/2020 19:56, Hannes Domani via Gdb-patches wrote:
> Fixes this testsuite fail on Windows:
> FAIL: gdb.base/auto-load.exp: print $script_loaded
> 
> Converts the debugfile path from c:/dir/file to /c/dir/file, so it can be
> appended to the auto-load path.
> 
> gdb/ChangeLog:
> 
> 2020-05-25  Hannes Domani  <ssbssa@yahoo.de>
> 
> 	* auto-load.c (auto_load_objfile_script_1): Convert drive part
> 	of debugfile path on Windows.
> ---
>   gdb/auto-load.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/gdb/auto-load.c b/gdb/auto-load.c
> index 99bd96b971..88221d9f3d 100644
> --- a/gdb/auto-load.c
> +++ b/gdb/auto-load.c
> @@ -784,6 +784,13 @@ auto_load_objfile_script_1 (struct objfile *objfile, const char *realname,
>   					  "scripts-directory' path \"%s\".\n"),
>   			    auto_load_dir);
>   
> +      /* Convert Windows debugfile path from c:/dir/file to /c/dir/file.  */
> +      if (HAS_DRIVE_SPEC (debugfile))
> +	{
> +	  debugfile_holder = STRIP_DRIVE_SPEC (debugfile);
> +	  filename = std::string("/") + debugfile[0] + debugfile_holder;


Is this kind of path transformation msys(2) specific?

> +	}
> +
>         for (const gdb::unique_xmalloc_ptr<char> &dir : vec)
>   	{
>   	  /* FILENAME is absolute, so we don't need a "/" here.  */
> 

Re: [PATCH 2/7] Handle Windows drives in auto-load script paths

[Christian Biesinger]

On Mon, May 25, 2020 at 1:57 PM Hannes Domani via Gdb-patches
<gdb-patches@sourceware.org> wrote:
>
> Fixes this testsuite fail on Windows:
> FAIL: gdb.base/auto-load.exp: print $script_loaded
>
> Converts the debugfile path from c:/dir/file to /c/dir/file, so it can be
> appended to the auto-load path.

How does this work? I thought /c/foo was not supported by either
cygwin or mingw?

Christian

> gdb/ChangeLog:
>
> 2020-05-25  Hannes Domani  <ssbssa@yahoo.de>
>
>         * auto-load.c (auto_load_objfile_script_1): Convert drive part
>         of debugfile path on Windows.
> ---
>  gdb/auto-load.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/gdb/auto-load.c b/gdb/auto-load.c
> index 99bd96b971..88221d9f3d 100644
> --- a/gdb/auto-load.c
> +++ b/gdb/auto-load.c
> @@ -784,6 +784,13 @@ auto_load_objfile_script_1 (struct objfile *objfile, const char *realname,
>                                           "scripts-directory' path \"%s\".\n"),
>                             auto_load_dir);
>
> +      /* Convert Windows debugfile path from c:/dir/file to /c/dir/file.  */
> +      if (HAS_DRIVE_SPEC (debugfile))
> +       {
> +         debugfile_holder = STRIP_DRIVE_SPEC (debugfile);
> +         filename = std::string("/") + debugfile[0] + debugfile_holder;
> +       }
> +
>        for (const gdb::unique_xmalloc_ptr<char> &dir : vec)
>         {
>           /* FILENAME is absolute, so we don't need a "/" here.  */
> --
> 2.26.2
>

Re: [PATCH 1/7] Fix function argument and return value locations

[Simon Marchi]

On 2020-05-25 7:03 p.m., Hannes Domani via Gdb-patches wrote:
> You're absolutely right again.
> The complex arguments didn't work before, so I just tried it with
> amd64_windows_passed_by_xmm_register (because that seemed the obvious choice
> for me), and then it worked.
> 
> But it only worked because in case of XMM register, the argument is also
> passed via the integer register:
> 
>       else if (amd64_windows_passed_by_xmm_register (type))
>         {
>           amd64_windows_store_arg_in_reg
>             (regcache, args[i], AMD64_XMM0_REGNUM + reg_idx);
>           /* In case of varargs, these parameters must also be
>          passed via the integer registers.  */
>           amd64_windows_store_arg_in_reg
>         (regcache, args[i],
>          amd64_windows_dummy_call_integer_regs[reg_idx]);
>           on_stack_p = 0;
>           reg_idx++;
>         }
> 
> So it actually should be added to amd64_windows_passed_by_integer_register,
> and a quick test just now confirms that this also works.
> 
> I'm very sorry about that, I should have checked the asm code.

I don't deserve much credit, I just randomly poked at it :)

Simon

Re: [PATCH 2/7] Handle Windows drives in auto-load script paths

[Hannes Domani]

 Am Dienstag, 26. Mai 2020, 18:05:57 MESZ hat Christian Biesinger <cbiesinger@google.com> Folgendes geschrieben:

> On Mon, May 25, 2020 at 1:57 PM Hannes Domani via Gdb-patches
> <gdb-patches@sourceware.org> wrote:
> >
> > Fixes this testsuite fail on Windows:
> > FAIL: gdb.base/auto-load.exp: print $script_loaded
> >
> > Converts the debugfile path from c:/dir/file to /c/dir/file, so it can be
> > appended to the auto-load path.
>
> How does this work? I thought /c/foo was not supported by either
> cygwin or mingw?

Yes, but in this case we need a path that can be appended to the various
entries of 'set auto-load scripts-directory'.
c:/foo can't be appended, but /c/foo can.
So you would get something like c:/script/directory/c/foo.


Hannes

Re: [PATCH 2/7] Handle Windows drives in auto-load script paths

[Hannes Domani]

 Am Dienstag, 26. Mai 2020, 18:04:13 MESZ hat Jon Turney <jon.turney@dronecode.org.uk> Folgendes geschrieben:

> On 25/05/2020 19:56, Hannes Domani via Gdb-patches wrote:
> > Fixes this testsuite fail on Windows:
> > FAIL: gdb.base/auto-load.exp: print $script_loaded
> >
> > Converts the debugfile path from c:/dir/file to /c/dir/file, so it can be
> > appended to the auto-load path.
> >
> > gdb/ChangeLog:
> >
> > 2020-05-25  Hannes Domani  <ssbssa@yahoo.de>
> >
> >     * auto-load.c (auto_load_objfile_script_1): Convert drive part
> >     of debugfile path on Windows.
> > ---
> >  gdb/auto-load.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/gdb/auto-load.c b/gdb/auto-load.c
> > index 99bd96b971..88221d9f3d 100644
> > --- a/gdb/auto-load.c
> > +++ b/gdb/auto-load.c
> > @@ -784,6 +784,13 @@ auto_load_objfile_script_1 (struct objfile *objfile, const char *realname,
> >                        "scripts-directory' path \"%s\".\n"),
> >                  auto_load_dir);
> >
> > +      /* Convert Windows debugfile path from c:/dir/file to /c/dir/file.  */
> > +      if (HAS_DRIVE_SPEC (debugfile))
> > +    {
> > +      debugfile_holder = STRIP_DRIVE_SPEC (debugfile);
> > +      filename = std::string("/") + debugfile[0] + debugfile_holder;
>
>
> Is this kind of path transformation msys(2) specific?

Yes, MSYS2 uses /c/foo paths, but that's not what this is used for.

I think this is basically the same question of Christian I just answered:

> > How does this work? I thought /c/foo was not supported by either
> > cygwin or mingw?
>
> Yes, but in this case we need a path that can be appended to the various
> entries of 'set auto-load scripts-directory'.
> c:/foo can't be appended, but /c/foo can.
> So you would get something like c:/script/directory/c/foo.


Hannes

Re: [PATCH 2/7] Handle Windows drives in auto-load script paths

[Christian Biesinger]

On Tue, May 26, 2020 at 11:25 AM Hannes Domani via Gdb-patches
<gdb-patches@sourceware.org> wrote:
>
>  Am Dienstag, 26. Mai 2020, 18:05:57 MESZ hat Christian Biesinger <cbiesinger@google.com> Folgendes geschrieben:
>
> > On Mon, May 25, 2020 at 1:57 PM Hannes Domani via Gdb-patches
> > <gdb-patches@sourceware.org> wrote:
> > >
> > > Fixes this testsuite fail on Windows:
> > > FAIL: gdb.base/auto-load.exp: print $script_loaded
> > >
> > > Converts the debugfile path from c:/dir/file to /c/dir/file, so it can be
> > > appended to the auto-load path.
> >
> > How does this work? I thought /c/foo was not supported by either
> > cygwin or mingw?
>
> Yes, but in this case we need a path that can be appended to the various
> entries of 'set auto-load scripts-directory'.
> c:/foo can't be appended, but /c/foo can.
> So you would get something like c:/script/directory/c/foo.

OK, so I *think* what you're saying is that if you have an objfile
c:\foo\bar.dll, and an auto-load directory c:\load, then with this
patch we will look for an auto-load script in
c:\load\c\foo\bar.dll-gdb.py? Is that right?

If so, shouldn't there be a documentation update about how this works
on Windows, and possibly a NEWS entry?

Christian

Re: [PATCH 5/7] Close file handle of empty history file

[Christian Biesinger]

On Mon, May 25, 2020 at 1:58 PM Hannes Domani via Gdb-patches
<gdb-patches@sourceware.org> wrote:
>
> Happened while trying to reproduce a gdb.base/gdbinit-history.exp failure:
> warning: Could not rename C:/gdb/build64/gdb-git/gdb/testsuite/outputs/gdb.base/gdbinit-history/gdbinit-history.gdb_history to C:/gdb/build64/gdb-git/gdb/testsuite/outputs/gdb.base/gdbinit-history/gdbinit-history.gdb_history-gdb5228~: Permission denied
>
> I had an empty gdbinit-history.gdb_history-gdb5228~ file, and the file
> handle was not closed on startup, so it couldn't rename it at the end
> when trying to write a new one.

Readline has an upstream repository, please send the patch to them as
well at bug-readline@gnu.org.
https://tiswww.case.edu/php/chet/readline/rltop.html

Also add the change to readline/README per the instructions in there.
Not sure if ChangeLog needs/should be updated.

Christian

>
> readline/readline/ChangeLog:
>
> 2020-05-25  Hannes Domani  <ssbssa@yahoo.de>
>
>         * histfile.c (read_history_range): Close file handle.
> ---
>  readline/readline/histfile.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/readline/readline/histfile.c b/readline/readline/histfile.c
> index dc64bde1c5..a8a92aa360 100644
> --- a/readline/readline/histfile.c
> +++ b/readline/readline/histfile.c
> @@ -305,6 +305,7 @@ read_history_range (const char *filename, int from, int to)
>    if (file_size == 0)
>      {
>        free (input);
> +      close (file);
>        return 0;        /* don't waste time if we don't have to */
>      }
>
> --
> 2.26.2
>

Re: [PATCH 2/7] Handle Windows drives in auto-load script paths

[Hannes Domani]

 Am Dienstag, 26. Mai 2020, 18:32:38 MESZ hat Christian Biesinger <cbiesinger@google.com> Folgendes geschrieben:

> On Tue, May 26, 2020 at 11:25 AM Hannes Domani via Gdb-patches
> <gdb-patches@sourceware.org> wrote:
> >
> >  Am Dienstag, 26. Mai 2020, 18:05:57 MESZ hat Christian Biesinger <cbiesinger@google.com> Folgendes geschrieben:
> >
> > > On Mon, May 25, 2020 at 1:57 PM Hannes Domani via Gdb-patches
> > > <gdb-patches@sourceware.org> wrote:
> > > >
> > > > Fixes this testsuite fail on Windows:
> > > > FAIL: gdb.base/auto-load.exp: print $script_loaded
> > > >
> > > > Converts the debugfile path from c:/dir/file to /c/dir/file, so it can be
> > > > appended to the auto-load path.
> > >
> > > How does this work? I thought /c/foo was not supported by either
> > > cygwin or mingw?
> >
> > Yes, but in this case we need a path that can be appended to the various
> > entries of 'set auto-load scripts-directory'.
> > c:/foo can't be appended, but /c/foo can.
> > So you would get something like c:/script/directory/c/foo.
>
> OK, so I *think* what you're saying is that if you have an objfile
> c:\foo\bar.dll, and an auto-load directory c:\load, then with this
> patch we will look for an auto-load script in
> c:\load\c\foo\bar.dll-gdb.py? Is that right?

Basically, yes.


> If so, shouldn't there be a documentation update about how this works
> on Windows, and possibly a NEWS entry?

A news entry that the bug was fixed?
From how I understand gdb.base/auto-load.exp, this is the way it was always
supposed to work (did I understand this wrong?).


Hannes

Re: [PATCH 2/7] Handle Windows drives in auto-load script paths

[Christian Biesinger]

On Tue, May 26, 2020 at 11:40 AM Hannes Domani via Gdb-patches
<gdb-patches@sourceware.org> wrote:
>
>  Am Dienstag, 26. Mai 2020, 18:32:38 MESZ hat Christian Biesinger <cbiesinger@google.com> Folgendes geschrieben:
>
> > On Tue, May 26, 2020 at 11:25 AM Hannes Domani via Gdb-patches
> > <gdb-patches@sourceware.org> wrote:
> > >
> > >  Am Dienstag, 26. Mai 2020, 18:05:57 MESZ hat Christian Biesinger <cbiesinger@google.com> Folgendes geschrieben:
> > >
> > > > On Mon, May 25, 2020 at 1:57 PM Hannes Domani via Gdb-patches
> > > > <gdb-patches@sourceware.org> wrote:
> > > > >
> > > > > Fixes this testsuite fail on Windows:
> > > > > FAIL: gdb.base/auto-load.exp: print $script_loaded
> > > > >
> > > > > Converts the debugfile path from c:/dir/file to /c/dir/file, so it can be
> > > > > appended to the auto-load path.
> > > >
> > > > How does this work? I thought /c/foo was not supported by either
> > > > cygwin or mingw?
> > >
> > > Yes, but in this case we need a path that can be appended to the various
> > > entries of 'set auto-load scripts-directory'.
> > > c:/foo can't be appended, but /c/foo can.
> > > So you would get something like c:/script/directory/c/foo.
> >
> > OK, so I *think* what you're saying is that if you have an objfile
> > c:\foo\bar.dll, and an auto-load directory c:\load, then with this
> > patch we will look for an auto-load script in
> > c:\load\c\foo\bar.dll-gdb.py? Is that right?
>
> Basically, yes.
>
>
> > If so, shouldn't there be a documentation update about how this works
> > on Windows, and possibly a NEWS entry?
>
> A news entry that the bug was fixed?
> From how I understand gdb.base/auto-load.exp, this is the way it was always
> supposed to work (did I understand this wrong?).

I don't feel so strongly about NEWS, but I specifically meant the
translation of c:\ to c/ on Windows -- shouldn't that be documented?

Christian

Re: [PATCH 2/7] Handle Windows drives in auto-load script paths

[Hannes Domani]

 Am Dienstag, 26. Mai 2020, 18:43:15 MESZ hat Christian Biesinger <cbiesinger@google.com> Folgendes geschrieben:

> On Tue, May 26, 2020 at 11:40 AM Hannes Domani via Gdb-patches
> <gdb-patches@sourceware.org> wrote:
> >
> >  Am Dienstag, 26. Mai 2020, 18:32:38 MESZ hat Christian Biesinger <cbiesinger@google.com> Folgendes geschrieben:
> >
> > > On Tue, May 26, 2020 at 11:25 AM Hannes Domani via Gdb-patches
> > > <gdb-patches@sourceware.org> wrote:
> > > >
> > > >  Am Dienstag, 26. Mai 2020, 18:05:57 MESZ hat Christian Biesinger <cbiesinger@google.com> Folgendes geschrieben:
> > > >
> > > > > On Mon, May 25, 2020 at 1:57 PM Hannes Domani via Gdb-patches
> > > > > <gdb-patches@sourceware.org> wrote:
> > > > > >
> > > > > > Fixes this testsuite fail on Windows:
> > > > > > FAIL: gdb.base/auto-load.exp: print $script_loaded
> > > > > >
> > > > > > Converts the debugfile path from c:/dir/file to /c/dir/file, so it can be
> > > > > > appended to the auto-load path.
> > > > >
> > > > > How does this work? I thought /c/foo was not supported by either
> > > > > cygwin or mingw?
> > > >
> > > > Yes, but in this case we need a path that can be appended to the various
> > > > entries of 'set auto-load scripts-directory'.
> > > > c:/foo can't be appended, but /c/foo can.
> > > > So you would get something like c:/script/directory/c/foo.
> > >
> > > OK, so I *think* what you're saying is that if you have an objfile
> > > c:\foo\bar.dll, and an auto-load directory c:\load, then with this
> > > patch we will look for an auto-load script in
> > > c:\load\c\foo\bar.dll-gdb.py? Is that right?
> >
> > Basically, yes.
> >
> >
> > > If so, shouldn't there be a documentation update about how this works
> > > on Windows, and possibly a NEWS entry?
> >
> > A news entry that the bug was fixed?
> > From how I understand gdb.base/auto-load.exp, this is the way it was always
> > supposed to work (did I understand this wrong?).
>
> I don't feel so strongly about NEWS, but I specifically meant the
> translation of c:\ to c/ on Windows -- shouldn't that be documented?

That's probably a good idea, debug-file-directory works the same way, and
there it is documented like this:

(On MS-Windows/MS-DOS, the drive letter of the executable’s leading
directories is converted to a one-letter subdirectory, i.e. d:/usr/bin/ is
converted to /d/usr/bin/, because Windows filesystems disallow colons in file
names.)

I will add something similar for scripts-directory.


Hannes

Re: [PATCH 5/7] Close file handle of empty history file

[Hannes Domani]

 > Am Dienstag, 26. Mai 2020, 18:38:07 MESZ hat Christian Biesinger <cbiesinger@google.com> Folgendes geschrieben:
>
> On Mon, May 25, 2020 at 1:58 PM Hannes Domani via Gdb-patches
> <gdb-patches@sourceware.org> wrote:
> >
> > Happened while trying to reproduce a gdb.base/gdbinit-history.exp failure:
> > warning: Could not rename C:/gdb/build64/gdb-git/gdb/testsuite/outputs/gdb.base/gdbinit-history/gdbinit-history.gdb_history to C:/gdb/build64/gdb-git/gdb/testsuite/outputs/gdb.base/gdbinit-history/gdbinit-history.gdb_history-gdb5228~: Permission denied
> >
> > I had an empty gdbinit-history.gdb_history-gdb5228~ file, and the file
> > handle was not closed on startup, so it couldn't rename it at the end
> > when trying to write a new one.
>
> Readline has an upstream repository, please send the patch to them as
> well at bug-readline@gnu.org.
> https://tiswww.case.edu/php/chet/readline/rltop.html
>
> Also add the change to readline/README per the instructions in there.
> Not sure if ChangeLog needs/should be updated.

Apparently, this was already fixed in readline master:
http://git.savannah.gnu.org/cgit/readline.git/commit/?id=f585708e822e021e15e5bece1de482b63ba581df

In that case I don't mind just leaving it as is, since it will be fixed
eventually anyways with the next update.


Hannes

Re: [PATCH 4/7] Use errno value of first openp failure

[Tom Tromey]

>>>>> "Hannes" == Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> writes:

Hannes> 2020-05-25  Hannes Domani  <ssbssa@yahoo.de>

Hannes> 	* exec.c (exec_file_attach): Use errno value of first openp failure.

This patch looks good to me.  Thank you.

Tom

Re: [PATCH 1/7] Fix function argument and return value locations

[Tom Tromey]

>>>>> "Hannes" == Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> writes:

Hannes> You're probably right, the thing is, I was only able to test complex float
Hannes> and complex double, because gdb doesn't like complex integral types:

Hannes> complex int complex_int = 5 + 6i;

I fixed up gdb's support for complex numbers relatively recently.
The particular thing you're seeing here is a gcc bug:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93988

Hannes> Do many people use complex int, because I personally wouldn't have expected
Hannes> that this even exists.

Seems doubtful that it is used.

Tom

Re: [PATCH 6/7] Move exit_status_set_internal_vars out of GLOBAL_CURDIR

[Tom Tromey]

>>>>> "Hannes" == Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> writes:

Hannes> The convenience variables $_shell_exitcode and $_shell_exitsignal don't
Hannes> depend on the GLOBAL_CURDIR define.

Hannes> gdb/ChangeLog:

Hannes> 2020-05-25  Hannes Domani  <ssbssa@yahoo.de>

Hannes> 	* cli/cli-cmds.c (shell_escape): Move exit_status_set_internal_vars.

Looks good.  Thanks.

Tom

Re: [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point

[Pedro Alves]

On 5/25/20 7:56 PM, Hannes Domani via Gdb-patches wrote:
> Fixes these testsuite fails on Windows (actually more, but the others are
> cascading failures):
> FAIL: gdb.base/hbreak2.exp: hardware breakpoint insertion (the program exited)
> FAIL: gdb.base/hbreak2.exp: run until function breakpoint (the program exited)
> FAIL: gdb.base/hbreak2.exp: run to factorial(6) (the program exited)
> FAIL: gdb.base/hbreak2.exp: run until hardware function breakpoint, optimized file (the program exited)
> 
> The problem happens if you only have hardware breakpoints active when
> (re-)starting the program:
> 
> (gdb) start
> Temporary breakpoint 1 at 0x401650: file C:/src/repos/binutils-gdb.git/gdb/tests
> uite/gdb.base/break.c, line 43.
> Starting program: C:\gdb\build64\gdb-git\gdb\testsuite\outputs\gdb.base\hbreak2\
> hbreak2.exe
> 
> Temporary breakpoint 1, main (argc=1, argv=0x7e2120, envp=0x7e2900)
>     at C:/src/repos/binutils-gdb.git/gdb/testsuite/gdb.base/break.c:43
> 43          if (argc == 12345) {  /* an unlikely value < 2^16, in case uninited
> */ /* set breakpoint 6 here */
> (gdb) hb factorial
> Hardware assisted breakpoint 2 at 0x401703: file C:/src/repos/binutils-gdb.git/g
> db/testsuite/gdb.base/break.c, line 63.
> (gdb) r
> The program being debugged has been started already.
> Start it from the beginning? (y or n) y
> Starting program: C:\gdb\build64\gdb-git\gdb\testsuite\outputs\gdb.base\hbreak2\
> hbreak2.exe
> 720
> [Inferior 1 (process 7836) exited normally]
> 
> But if you stopped just once before reaching the hardware breakpoint, it
> works fine:
> 
> (gdb) start
> Temporary breakpoint 3 at 0x401650: file C:/src/repos/binutils-gdb.git/gdb/tests
> uite/gdb.base/break.c, line 43.
> Starting program: C:\gdb\build64\gdb-git\gdb\testsuite\outputs\gdb.base\hbreak2\
> hbreak2.exe
> 
> Temporary breakpoint 3, main (argc=1, argv=0x322120, envp=0x322900)
>     at C:/src/repos/binutils-gdb.git/gdb/testsuite/gdb.base/break.c:43
> 43          if (argc == 12345) {  /* an unlikely value < 2^16, in case uninited
> */ /* set breakpoint 6 here */
> (gdb) c
> Continuing.
> 
> Breakpoint 2, factorial (value=6)
>     at C:/src/repos/binutils-gdb.git/gdb/testsuite/gdb.base/break.c:63
> 63        if (value > 1) {  /* set breakpoint 7 here */
> 
> I found out that cdb writes this error when trying to do the same:
> 
> Unable to set breakpoint error
> The system resets thread contexts after the process
> breakpoint so hardware breakpoints cannot be set.
> Go to the executable's entry point and set it then.
> 
> So all the hardware breakpoints that were set before (or rather, their
> debug register information) are practically lost when the process entry
> point is reached.
> 
> This patch creates an internal breakpoint on the process entry point, which
> when it is reached, resets all active hardware breakpoints, and continues
> execution.

Eh, I always assumed that the initial breakpoint the Windows debug API emits
during process initialization _was_ the entry point.  Where does the PC
point at when the initial breakpoint is reached?

You can use "starti" to check that.  "starti" spawns the process with
target_create_inferior, and then just does not run any other instruction,
It presents the stop where target_create_inferior left the process, which
is supposedly the process's entry point.

If Windows indeed resets the thread context after the initial
process breakpoint, then it sounds like we also lose any change to
the registers you make after "starti".

E.g., if you do "starti" + "p $rax = 0xabc" + "stepi", or something like
that.

It it sounding to me like we should be running to the entry point
in the initialization loop, from within do_initial_windows_stuff?

Thanks,
Pedro Alves

Re: [PATCH 5/7] Close file handle of empty history file

[Tom Tromey]

>>>>> "Hannes" == Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> writes:

Hannes> Apparently, this was already fixed in readline master:
Hannes> http://git.savannah.gnu.org/cgit/readline.git/commit/?id=f585708e822e021e15e5bece1de482b63ba581df

Hannes> In that case I don't mind just leaving it as is, since it will be fixed
Hannes> eventually anyways with the next update.

If it's upstream, it is fine if you want to commit it to gdb as well.

Imports of readline are done sporadically, not on any real schedule.
If there's been official releases, I guess we should do one though :)

Tom

Re: [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point

[Pedro Alves]

On 5/27/20 1:07 PM, Pedro Alves via Gdb-patches wrote:
> On 5/25/20 7:56 PM, Hannes Domani via Gdb-patches wrote:
>> Fixes these testsuite fails on Windows (actually more, but the others are
>> cascading failures):
>> FAIL: gdb.base/hbreak2.exp: hardware breakpoint insertion (the program exited)
>> FAIL: gdb.base/hbreak2.exp: run until function breakpoint (the program exited)
>> FAIL: gdb.base/hbreak2.exp: run to factorial(6) (the program exited)
>> FAIL: gdb.base/hbreak2.exp: run until hardware function breakpoint, optimized file (the program exited)
>>
>> The problem happens if you only have hardware breakpoints active when
>> (re-)starting the program:
>>
>> (gdb) start
>> Temporary breakpoint 1 at 0x401650: file C:/src/repos/binutils-gdb.git/gdb/tests
>> uite/gdb.base/break.c, line 43.
>> Starting program: C:\gdb\build64\gdb-git\gdb\testsuite\outputs\gdb.base\hbreak2\
>> hbreak2.exe
>>
>> Temporary breakpoint 1, main (argc=1, argv=0x7e2120, envp=0x7e2900)
>>     at C:/src/repos/binutils-gdb.git/gdb/testsuite/gdb.base/break.c:43
>> 43          if (argc == 12345) {  /* an unlikely value < 2^16, in case uninited
>> */ /* set breakpoint 6 here */
>> (gdb) hb factorial
>> Hardware assisted breakpoint 2 at 0x401703: file C:/src/repos/binutils-gdb.git/g
>> db/testsuite/gdb.base/break.c, line 63.
>> (gdb) r
>> The program being debugged has been started already.
>> Start it from the beginning? (y or n) y
>> Starting program: C:\gdb\build64\gdb-git\gdb\testsuite\outputs\gdb.base\hbreak2\
>> hbreak2.exe
>> 720
>> [Inferior 1 (process 7836) exited normally]
>>
>> But if you stopped just once before reaching the hardware breakpoint, it
>> works fine:
>>
>> (gdb) start
>> Temporary breakpoint 3 at 0x401650: file C:/src/repos/binutils-gdb.git/gdb/tests
>> uite/gdb.base/break.c, line 43.
>> Starting program: C:\gdb\build64\gdb-git\gdb\testsuite\outputs\gdb.base\hbreak2\
>> hbreak2.exe
>>
>> Temporary breakpoint 3, main (argc=1, argv=0x322120, envp=0x322900)
>>     at C:/src/repos/binutils-gdb.git/gdb/testsuite/gdb.base/break.c:43
>> 43          if (argc == 12345) {  /* an unlikely value < 2^16, in case uninited
>> */ /* set breakpoint 6 here */
>> (gdb) c
>> Continuing.
>>
>> Breakpoint 2, factorial (value=6)
>>     at C:/src/repos/binutils-gdb.git/gdb/testsuite/gdb.base/break.c:63
>> 63        if (value > 1) {  /* set breakpoint 7 here */
>>
>> I found out that cdb writes this error when trying to do the same:
>>
>> Unable to set breakpoint error
>> The system resets thread contexts after the process
>> breakpoint so hardware breakpoints cannot be set.
>> Go to the executable's entry point and set it then.
>>
>> So all the hardware breakpoints that were set before (or rather, their
>> debug register information) are practically lost when the process entry
>> point is reached.
>>
>> This patch creates an internal breakpoint on the process entry point, which
>> when it is reached, resets all active hardware breakpoints, and continues
>> execution.
> 
> Eh, I always assumed that the initial breakpoint the Windows debug API emits
> during process initialization _was_ the entry point.  Where does the PC
> point at when the initial breakpoint is reached?
> 
> You can use "starti" to check that.  "starti" spawns the process with
> target_create_inferior, and then just does not run any other instruction,
> It presents the stop where target_create_inferior left the process, which
> is supposedly the process's entry point.
> 
> If Windows indeed resets the thread context after the initial
> process breakpoint, then it sounds like we also lose any change to
> the registers you make after "starti".
> 
> E.g., if you do "starti" + "p $rax = 0xabc" + "stepi", or something like
> that.
> 
> It it sounding to me like we should be running to the entry point
> in the initialization loop, from within do_initial_windows_stuff?

The answer at <https://stackoverflow.com/questions/5925542/how-can-i-set-break-point-on-my-function-and-list-its-source>,
seems to confirm it:

   "If you debugger sets some registry values right now, those changes would not
    matter, because OS will override all register values while starting executing
    process. "

Interestingly, it also says:

   "Workaround is simple -- first make a single step using t instruction, then you
    can use hardware breakpoints as you like."

Which makes it sounds like a single-step is all it takes to get to the entry point.
That may make it easier to implement this inside do_initial_windows_stuff,
including doing the same thing in gdbserver.

Thanks,
Pedro Alves

Re: [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point

[Hannes Domani]

 Am Mittwoch, 27. Mai 2020, 16:48:16 MESZ hat Pedro Alves <palves@redhat.com> Folgendes geschrieben:

> On 5/27/20 1:07 PM, Pedro Alves via Gdb-patches wrote:
> > On 5/25/20 7:56 PM, Hannes Domani via Gdb-patches wrote:
> >> Fixes these testsuite fails on Windows (actually more, but the others are
> >> cascading failures):
> >> FAIL: gdb.base/hbreak2.exp: hardware breakpoint insertion (the program exited)
> >> FAIL: gdb.base/hbreak2.exp: run until function breakpoint (the program exited)
> >> FAIL: gdb.base/hbreak2.exp: run to factorial(6) (the program exited)
> >> FAIL: gdb.base/hbreak2.exp: run until hardware function breakpoint, optimized file (the program exited)
> >>
> >> The problem happens if you only have hardware breakpoints active when
> >> (re-)starting the program:
> >>
> >> (gdb) start
> >> Temporary breakpoint 1 at 0x401650: file C:/src/repos/binutils-gdb.git/gdb/tests
> >> uite/gdb.base/break.c, line 43.
> >> Starting program: C:\gdb\build64\gdb-git\gdb\testsuite\outputs\gdb.base\hbreak2\
> >> hbreak2.exe
> >>
> >> Temporary breakpoint 1, main (argc=1, argv=0x7e2120, envp=0x7e2900)
> >>    at C:/src/repos/binutils-gdb.git/gdb/testsuite/gdb.base/break.c:43
> >> 43          if (argc == 12345) {  /* an unlikely value < 2^16, in case uninited
> >> */ /* set breakpoint 6 here */
> >> (gdb) hb factorial
> >> Hardware assisted breakpoint 2 at 0x401703: file C:/src/repos/binutils-gdb.git/g
> >> db/testsuite/gdb.base/break.c, line 63.
> >> (gdb) r
> >> The program being debugged has been started already.
> >> Start it from the beginning? (y or n) y
> >> Starting program: C:\gdb\build64\gdb-git\gdb\testsuite\outputs\gdb.base\hbreak2\
> >> hbreak2.exe
> >> 720
> >> [Inferior 1 (process 7836) exited normally]
> >>
> >> But if you stopped just once before reaching the hardware breakpoint, it
> >> works fine:
> >>
> >> (gdb) start
> >> Temporary breakpoint 3 at 0x401650: file C:/src/repos/binutils-gdb.git/gdb/tests
> >> uite/gdb.base/break.c, line 43.
> >> Starting program: C:\gdb\build64\gdb-git\gdb\testsuite\outputs\gdb.base\hbreak2\
> >> hbreak2.exe
> >>
> >> Temporary breakpoint 3, main (argc=1, argv=0x322120, envp=0x322900)
> >>    at C:/src/repos/binutils-gdb.git/gdb/testsuite/gdb.base/break.c:43
> >> 43          if (argc == 12345) {  /* an unlikely value < 2^16, in case uninited
> >> */ /* set breakpoint 6 here */
> >> (gdb) c
> >> Continuing.
> >>
> >> Breakpoint 2, factorial (value=6)
> >>    at C:/src/repos/binutils-gdb.git/gdb/testsuite/gdb.base/break.c:63
> >> 63        if (value > 1) {  /* set breakpoint 7 here */
> >>
> >> I found out that cdb writes this error when trying to do the same:
> >>
> >> Unable to set breakpoint error
> >> The system resets thread contexts after the process
> >> breakpoint so hardware breakpoints cannot be set.
> >> Go to the executable's entry point and set it then.
> >>
> >> So all the hardware breakpoints that were set before (or rather, their
> >> debug register information) are practically lost when the process entry
> >> point is reached.
> >>
> >> This patch creates an internal breakpoint on the process entry point, which
> >> when it is reached, resets all active hardware breakpoints, and continues
> >> execution.
> >
> > Eh, I always assumed that the initial breakpoint the Windows debug API emits
> > during process initialization _was_ the entry point.  Where does the PC
> > point at when the initial breakpoint is reached?
> >
> > You can use "starti" to check that.  "starti" spawns the process with
> > target_create_inferior, and then just does not run any other instruction,
> > It presents the stop where target_create_inferior left the process, which
> > is supposedly the process's entry point.

Somewhere in ntdll.dll (the function names are probably wrong):

(gdb) starti
Starting program: C:\src\tests\ret-types.exe

Program stopped.
0x0000000076d5cb71 in ntdll!CsrSetPriorityClass () from C:\Windows\SYSTEM32\ntdll.dll
(gdb) bt
#0  0x0000000076d5cb71 in ntdll!CsrSetPriorityClass () from C:\Windows\SYSTEM32\ntdll.dll
#1  0x0000000076d12e85 in ntdll!RtlIsDosDeviceName_U () from C:\Windows\SYSTEM32\ntdll.dll
#2  0x0000000076cf1937 in ntdll!RtlCreateTagHeap () from C:\Windows\SYSTEM32\ntdll.dll
#3  0x0000000076cdc34e in ntdll!LdrInitializeThunk () from C:\Windows\SYSTEM32\ntdll.dll
#4  0x0000000000000000 in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)

I think there is a lot happening before reaching the process entry point,
like calling the dll entry points.


> > If Windows indeed resets the thread context after the initial
> > process breakpoint, then it sounds like we also lose any change to
> > the registers you make after "starti".
> >
> > E.g., if you do "starti" + "p $rax = 0xabc" + "stepi", or something like
> > that.
> >
> > It it sounding to me like we should be running to the entry point
> > in the initialization loop, from within do_initial_windows_stuff?

I'm not sure that's a good idea, I would expect starti to stop where it is
now, before the dll initialization.


> The answer at <https://stackoverflow.com/questions/5925542/how-can-i-set-break-point-on-my-function-and-list-its-source>,
> seems to confirm it:
>
>   "If you debugger sets some registry values right now, those changes would not
>     matter, because OS will override all register values while starting executing
>     process. "
>
> Interestingly, it also says:
>
>   "Workaround is simple -- first make a single step using t instruction, then you
>     can use hardware breakpoints as you like."
>
> Which makes it sounds like a single-step is all it takes to get to the entry point.
> That may make it easier to implement this inside do_initial_windows_stuff,
> including doing the same thing in gdbserver.

I just tried:

(gdb) start
Temporary breakpoint 1 at 0x401a73: file ret-types.c, line 131.
Starting program: C:\src\tests\ret-types.exe

Temporary breakpoint 1, main () at ret-types.c:131
131       return func (cf);
(gdb) hb main
Hardware assisted breakpoint 2 at 0x401a73: file ret-types.c, line 131.
(gdb) c
Continuing.
[Inferior 1 (process 6228) exited with code 0317]
(gdb) starti
Starting program: C:\src\tests\ret-types.exe

Program stopped.
0x0000000076d5cb71 in ntdll!CsrSetPriorityClass () from C:\Windows\SYSTEM32\ntdll.dll
(gdb) nexti
0x0000000076d5cb73 in ntdll!CsrSetPriorityClass () from C:\Windows\SYSTEM32\ntdll.dll
(gdb) c
Continuing.
[Inferior 1 (process 2480) exited with code 0317]

So after a nexti the hardware breakpoint still didn't work, because at this
point it's far before the overwriting of the context.

Or did you mean this different?

Anyways, since there is a simple workaround to make the hardware breakpoints
work (just use "start"), it doesn't matter much to me if this gets fixed.


Hannes

Re: [PATCH 5/7] Close file handle of empty history file

[Hannes Domani]

 Am Mittwoch, 27. Mai 2020, 16:33:40 MESZ hat Tom Tromey <tom@tromey.com> Folgendes geschrieben:

> >>>>> "Hannes" == Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Hannes> Apparently, this was already fixed in readline master:
> Hannes> http://git.savannah.gnu.org/cgit/readline.git/commit/?id=f585708e822e021e15e5bece1de482b63ba581df
>
>
> Hannes> In that case I don't mind just leaving it as is, since it will be fixed
> Hannes> eventually anyways with the next update.
>
> If it's upstream, it is fine if you want to commit it to gdb as well.

According to readline/README it has to be documented in there somehow.

Also there are 3 ChangeLog files (readline/ChangeLog,
readline/readline/CHANGELOG, and readline/readline/ChangeLog.gdb), all of them
seem to have gdb-specific entries.

I don't really know what to do with all that.


> Imports of readline are done sporadically, not on any real schedule.
> If there's been official releases, I guess we should do one though :)

There has not been a newer official release then the one currently in gdb.


Hannes

Re: [PATCH 6/7] Move exit_status_set_internal_vars out of GLOBAL_CURDIR

[Hannes Domani]

 Am Dienstag, 26. Mai 2020, 22:45:08 MESZ hat Tom Tromey <tom@tromey.com> Folgendes geschrieben:

> >>>>> "Hannes" == Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Hannes> The convenience variables $_shell_exitcode and $_shell_exitsignal don't
> Hannes> depend on the GLOBAL_CURDIR define.
>
> Hannes> gdb/ChangeLog:
>
> Hannes> 2020-05-25  Hannes Domani  <ssbssa@yahoo.de>
>
> Hannes>     * cli/cli-cmds.c (shell_escape): Move exit_status_set_internal_vars.
>
> Looks good.  Thanks.

Pushed this and the errno patch, thanks.


Hannes

Re: [PATCH 5/7] Close file handle of empty history file

[Christian Biesinger]

On Wed, May 27, 2020 at 12:38 PM Hannes Domani via Gdb-patches
<gdb-patches@sourceware.org> wrote:
>  Am Mittwoch, 27. Mai 2020, 16:33:40 MESZ hat Tom Tromey <tom@tromey.com> Folgendes geschrieben:
> > Imports of readline are done sporadically, not on any real schedule.
> > If there's been official releases, I guess we should do one though :)
>
> There has not been a newer official release then the one currently in gdb.

Well... readline's release process is a bit weird. There has been an
"official patch":
https://lists.gnu.org/archive/html/bug-readline/2019-08/msg00004.html

(actually several,
https://lists.gnu.org/archive/html/bug-readline/2020-02/threads.html)

Christian

Re: Windows testsuite failures

[Christian Biesinger]

On Mon, May 25, 2020 at 1:58 PM Hannes Domani via Gdb-patches
<gdb-patches@sourceware.org> wrote:
> I went through all testsuite failures in gdb.base, and fixed everything
> I could figure out.

Out of curiosity, which environment did you them in? cygwin/msys/msys2?

> PS: It takes 40 minutes to run all tests of gdb.base, how does this
> compare to Linux?

I usually run with -j, but without it:
$  time make -C obj/gdb/testsuite/ check TESTS="gdb.base/*.exp"
[...]
real 33m41.442s
user 25m31.795s
sys 6m36.257s

So pretty much the same. This is on a Intel(R) Xeon(R) Gold 6154 CPU @
3.00GHz running Debian.

Christian

Re: Windows testsuite failures

[Hannes Domani]

 Am Donnerstag, 28. Mai 2020, 20:15:58 MESZ hat Christian Biesinger <cbiesinger@google.com> Folgendes geschrieben:

> On Mon, May 25, 2020 at 1:58 PM Hannes Domani via Gdb-patches
> <gdb-patches@sourceware.org> wrote:
> > I went through all testsuite failures in gdb.base, and fixed everything
> > I could figure out.
>
> Out of curiosity, which environment did you them in? cygwin/msys/msys2?

msys2.


> > PS: It takes 40 minutes to run all tests of gdb.base, how does this
> > compare to Linux?
>
>
> I usually run with -j, but without it:
> $  time make -C obj/gdb/testsuite/ check TESTS="gdb.base/*.exp"
> [...]
> real 33m41.442s
> user 25m31.795s
> sys 6m36.257s
>
> So pretty much the same. This is on a Intel(R) Xeon(R) Gold 6154 CPU @
> 3.00GHz running Debian.

I see, I was sure that, like compiling, this would be a lot faster on Linux.
Looks like I was wrong.


Hannes

Re: [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point

[Pedro Alves]

On 5/27/20 4:39 PM, Hannes Domani via Gdb-patches wrote:
>  Am Mittwoch, 27. Mai 2020, 16:48:16 MESZ hat Pedro Alves <palves@redhat.com> Folgendes geschrieben:

>> The answer at <https://stackoverflow.com/questions/5925542/how-can-i-set-break-point-on-my-function-and-list-its-source>,
>> seems to confirm it:
>>
>>    "If you debugger sets some registry values right now, those changes would not
>>      matter, because OS will override all register values while starting executing
>>      process. "
>>
>> Interestingly, it also says:
>>
>>    "Workaround is simple -- first make a single step using t instruction, then you
>>      can use hardware breakpoints as you like."
>>
>> Which makes it sounds like a single-step is all it takes to get to the entry point.
>> That may make it easier to implement this inside do_initial_windows_stuff,
>> including doing the same thing in gdbserver.
> 
> I just tried:

...

> So after a nexti the hardware breakpoint still didn't work, because at this
> point it's far before the overwriting of the context.
> 

OK, thanks for trying it out.

> Or did you mean this different?

No, I was misled by the comments in stackoverflow, which are clearly incorrect.

Meanwhile I found this, which explains this mechanism in more detail:

 http://www.nynaeve.net/?p=81

> Anyways, since there is a simple workaround to make the hardware breakpoints
> work (just use "start"), it doesn't matter much to me if this gets fixed.
I think that it's nice to make this work transparently; I was just
exploring the best way to do so.  Let me take another look at the patch.

Thanks,
Pedro Alves

Re: [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point

[Pedro Alves]

On 5/25/20 7:56 PM, Hannes Domani via Gdb-patches wrote:

> This patch creates an internal breakpoint on the process entry point, which
> when it is reached, resets all active hardware breakpoints, and continues
> execution.

Missing ChangeLog entry.

> ---
>  gdb/windows-tdep.c | 130 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 130 insertions(+)
> 
> diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c
> index aa0adeba99..90e4794fc5 100644
> --- a/gdb/windows-tdep.c
> +++ b/gdb/windows-tdep.c
> @@ -37,6 +37,7 @@
>  #include "coff/internal.h"
>  #include "libcoff.h"
>  #include "solist.h"
> +#include "observable.h"
>  
>  #define CYGWIN_DLL_NAME "cygwin1.dll"
>  
> @@ -870,6 +871,99 @@ windows_get_siginfo_type (struct gdbarch *gdbarch)
>    return siginfo_type;
>  }
>  
> +/* Windows-specific cached data.  This is used by GDB for caching
> +   purposes for each inferior.  This helps reduce the overhead of
> +   transfering data from a remote target to the local host.  */
> +struct windows_info
> +{
> +  CORE_ADDR entry_point = 0;
> +};
> +
> +/* Per-inferior data key.  */
> +static const struct inferior_key<windows_info> windows_inferior_data;

This should be program_space_key / per-program_space data, instead of
per-inferior data.

You may want to take a look at the jit.c code, which is doing similar
things.

> +
> +/* Frees whatever allocated space there is to be freed and sets INF's
> +   Windows cache data pointer to NULL.  */
> +
> +static void
> +invalidate_windows_cache_inf (struct inferior *inf)
> +{
> +  windows_inferior_data.clear (inf);
> +}
> +
> +/* Fetch the Windows cache info for INF.  This function always returns a
> +   valid INFO pointer.  */
> +
> +static struct windows_info *
> +get_windows_inferior_data (void)

Drop the (void), only old pre-C++ code has it.  You can also drop
redundant "struct" throughout if you like.

> +{
> +  struct windows_info *info;
> +  struct inferior *inf = current_inferior ();
> +
> +  info = windows_inferior_data.get (inf);
> +  if (info == NULL)
> +    info = windows_inferior_data.emplace (inf);
> +
> +  return info;
> +}
> +
> +/* Breakpoint on entry point where any active hardware breakpoints will
> +   be reset.  */

Please expand the comments, explaining why this is necessary
in the first place.

> +static struct breakpoint_ops entry_point_breakpoint_ops;
> +
> +/* Reset active hardware breakpoints.  */
> +
> +static bool
> +reset_hardware_breakpoints (struct breakpoint *b)
> +{
> +  if (b->type != bp_hardware_breakpoint
> +      && b->type != bp_hardware_watchpoint
> +      && b->type != bp_read_watchpoint
> +      && b->type != bp_access_watchpoint)
> +    return false;

This should instead look at locations and their bp_loc_type,
looking for bp_loc_hardware_breakpoint / bp_loc_hardware_watchpoint.
There are situations where the breakpoint is a software breakpoint,
but GDB still inserts a hardware breakpoint, like e.g., due
to "set breakpoint auto-hw".

> +
> +  struct bp_location *loc;
> +  for (loc = b->loc; loc; loc = loc->next)
> +    if (loc->enabled && loc->pspace == current_program_space
> +	&& b->ops->remove_location (loc, REMOVE_BREAKPOINT) == 0)
> +      b->ops->insert_location (loc);

This is incorrect for not considering whether a
breakpoint location was enabled but not inserted (e.g., the overall
breakpoint was disabled), or whether a breakpoint location was
a duplicate.

You should instead look at loc->inserted.

> +
> +  return false;
> +}
> +
> +/* This breakpoint type should never stop, but when reached, reset
> +   the active hardware breakpoints.  */

hardware breakpoints and watchpoints.

> +
> +static void
> +startup_breakpoint_check_status (bpstat bs)
> +{
> +  /* Never stop.  */
> +  bs->stop = 0;
> +
> +  iterate_over_breakpoints (reset_hardware_breakpoints);
> +}
> +
> +/* Update the breakpoint location to the current entry point.  */
> +
> +static void
> +startup_breakpoint_re_set (struct breakpoint *b)
> +{

This is called if/when the loaded executable changes, even
without re-starting an inferior.  E.g., if you use the
"file" command after starting the inferior.  So this
should re-fetch the new entry point from the executable.
Again, take a look at the jit.c code.

> +  struct windows_info *info = get_windows_inferior_data ();
> +  CORE_ADDR entry_point = info->entry_point;
> +
> +  /* Do nothing if the entry point didn't change.  */
> +  struct bp_location *loc;
> +  for (loc = b->loc; loc; loc = loc->next)
> +    if (loc->pspace == current_program_space && loc->address == entry_point)
> +      return;
> +
> +  event_location_up location
> +    = new_address_location (entry_point, nullptr, 0);
> +  std::vector<symtab_and_line> sals;
> +  sals = b->ops->decode_location (b, location.get (), current_program_space);

Merge the two statements, so that you end up copy initialization, instead of
initialization, and then assignment:

  std::vector<symtab_and_line> sals
    = b->ops->decode_location (b, location.get (), current_program_space);

> +  update_breakpoint_locations (b, current_program_space, sals, {});
> +}
> +
>  /* Implement the "solib_create_inferior_hook" target_so_ops method.  */
>  
>  static void
> @@ -914,6 +1008,30 @@ windows_solib_create_inferior_hook (int from_tty)
>        if (vmaddr != exec_base)
>  	objfile_rebase (symfile_objfile, exec_base - vmaddr);
>      }
> +
> +  /* Create the entry point breakpoint if it doesn't exist already.  */
> +  if (target_has_execution && exec_base != 0)
> +    {
> +      struct windows_info *info = get_windows_inferior_data ();
> +      CORE_ADDR entry_point = exec_base
> +	+ pe_data (exec_bfd)->pe_opthdr.AddressOfEntryPoint;
> +      info->entry_point = entry_point;
> +
> +      breakpoint *startup_breakpoint
> +	= iterate_over_breakpoints ([] (breakpoint *bp)
> +	  {
> +	    return bp->ops == &entry_point_breakpoint_ops;
> +	  });
> +      if (startup_breakpoint == nullptr)
> +	{
> +	  event_location_up location
> +	    = new_address_location (entry_point, nullptr, 0);
> +	  create_breakpoint (target_gdbarch(), location.get(), nullptr, -1,

Space before parens.

This looking up for the pre-existing breakpoint doesn't work
correctly when you consider multiple inferiors, where each will
need a location for its own entry pointer.  The Windows backend
doesn't support multi-process, but OTOH, if you do it like jit.c
does, which just basically always create a breakpoint and
stores the pointer in the per-pspace data, you're practically
good to go, and you'll make it easier for whomever comes next
and decides to all multi-process support.

> +			     nullptr, 0, 1, bp_breakpoint, 0,
> +			     AUTO_BOOLEAN_FALSE, &entry_point_breakpoint_ops,
> +			     0, 1, 1, 0);

Thanks,
Pedro Alves

Re: [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point

[Hannes Domani]

 Am Sonntag, 31. Mai 2020, 18:38:06 MESZ hat Pedro Alves <palves@redhat.com> Folgendes geschrieben:

> On 5/25/20 7:56 PM, Hannes Domani via Gdb-patches wrote:
>
> > This patch creates an internal breakpoint on the process entry point, which
> > when it is reached, resets all active hardware breakpoints, and continues
> > execution.
>
> Missing ChangeLog entry.
>
> > ---
> >  gdb/windows-tdep.c | 130 +++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 130 insertions(+)
> >
> > diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c
> > index aa0adeba99..90e4794fc5 100644
> > --- a/gdb/windows-tdep.c
> > +++ b/gdb/windows-tdep.c
> > @@ -37,6 +37,7 @@
> >  #include "coff/internal.h"
> >  #include "libcoff.h"
> >  #include "solist.h"
> > +#include "observable.h"
> >
> >  #define CYGWIN_DLL_NAME "cygwin1.dll"
> >
> > @@ -870,6 +871,99 @@ windows_get_siginfo_type (struct gdbarch *gdbarch)
> >    return siginfo_type;
> >  }
> >
> > +/* Windows-specific cached data.  This is used by GDB for caching
> > +  purposes for each inferior.  This helps reduce the overhead of
> > +  transfering data from a remote target to the local host.  */
> > +struct windows_info
> > +{
> > +  CORE_ADDR entry_point = 0;
> > +};
> > +
> > +/* Per-inferior data key.  */
> > +static const struct inferior_key<windows_info> windows_inferior_data;
>
> This should be program_space_key / per-program_space data, instead of
> per-inferior data.
>
> You may want to take a look at the jit.c code, which is doing similar
> things.

OK. I will change it to program_space_key, but why are there no observers
to invalidate the a program-space, like these for inferiors?:

  /* Observers used to invalidate the cache when needed.  */
  gdb::observers::inferior_exit.attach (invalidate_windows_cache_inf);
  gdb::observers::inferior_appeared.attach (invalidate_windows_cache_inf);

Are they not needed for some reason?


> > +
> > +/* Frees whatever allocated space there is to be freed and sets INF's
> > +  Windows cache data pointer to NULL.  */
> > +
> > +static void
> > +invalidate_windows_cache_inf (struct inferior *inf)
> > +{
> > +  windows_inferior_data.clear (inf);
> > +}
> > +
> > +/* Fetch the Windows cache info for INF.  This function always returns a
> > +  valid INFO pointer.  */
> > +
> > +static struct windows_info *
> > +get_windows_inferior_data (void)
>
> Drop the (void), only old pre-C++ code has it.  You can also drop
> redundant "struct" throughout if you like.
>
> > +{
> > +  struct windows_info *info;
> > +  struct inferior *inf = current_inferior ();
> > +
> > +  info = windows_inferior_data.get (inf);
> > +  if (info == NULL)
> > +    info = windows_inferior_data.emplace (inf);
> > +
> > +  return info;
> > +}
> > +
> > +/* Breakpoint on entry point where any active hardware breakpoints will
> > +  be reset.  */
>
> Please expand the comments, explaining why this is necessary
> in the first place.
>
> > +static struct breakpoint_ops entry_point_breakpoint_ops;
> > +
> > +/* Reset active hardware breakpoints.  */
> > +
> > +static bool
> > +reset_hardware_breakpoints (struct breakpoint *b)
> > +{
> > +  if (b->type != bp_hardware_breakpoint
> > +      && b->type != bp_hardware_watchpoint
> > +      && b->type != bp_read_watchpoint
> > +      && b->type != bp_access_watchpoint)
> > +    return false;
>
> This should instead look at locations and their bp_loc_type,
> looking for bp_loc_hardware_breakpoint / bp_loc_hardware_watchpoint.
> There are situations where the breakpoint is a software breakpoint,
> but GDB still inserts a hardware breakpoint, like e.g., due
> to "set breakpoint auto-hw".
>
> > +
> > +  struct bp_location *loc;
> > +  for (loc = b->loc; loc; loc = loc->next)
> > +    if (loc->enabled && loc->pspace == current_program_space
> > +    && b->ops->remove_location (loc, REMOVE_BREAKPOINT) == 0)
> > +      b->ops->insert_location (loc);
>
> This is incorrect for not considering whether a
> breakpoint location was enabled but not inserted (e.g., the overall
> breakpoint was disabled), or whether a breakpoint location was
> a duplicate.
>
> You should instead look at loc->inserted.
>
> > +
> > +  return false;
> > +}
> > +
> > +/* This breakpoint type should never stop, but when reached, reset
> > +  the active hardware breakpoints.  */
>
> hardware breakpoints and watchpoints.
>
> > +
> > +static void
> > +startup_breakpoint_check_status (bpstat bs)
> > +{
> > +  /* Never stop.  */
> > +  bs->stop = 0;
> > +
> > +  iterate_over_breakpoints (reset_hardware_breakpoints);
> > +}
> > +
> > +/* Update the breakpoint location to the current entry point.  */
> > +
> > +static void
> > +startup_breakpoint_re_set (struct breakpoint *b)
> > +{
>
> This is called if/when the loaded executable changes, even
> without re-starting an inferior.  E.g., if you use the
> "file" command after starting the inferior.  So this
> should re-fetch the new entry point from the executable.
> Again, take a look at the jit.c code.

If I do "file" after "start", then windows_solib_create_inferior_hook is
called before startup_breakpoint_re_set, so the new entry point was
fetched already.


> > +  struct windows_info *info = get_windows_inferior_data ();
> > +  CORE_ADDR entry_point = info->entry_point;
> > +
> > +  /* Do nothing if the entry point didn't change.  */
> > +  struct bp_location *loc;
> > +  for (loc = b->loc; loc; loc = loc->next)
> > +    if (loc->pspace == current_program_space && loc->address == entry_point)
> > +      return;
> > +
> > +  event_location_up location
> > +    = new_address_location (entry_point, nullptr, 0);
> > +  std::vector<symtab_and_line> sals;
> > +  sals = b->ops->decode_location (b, location.get (), current_program_space);
>
> Merge the two statements, so that you end up copy initialization, instead of
> initialization, and then assignment:
>
>   std::vector<symtab_and_line> sals
>     = b->ops->decode_location (b, location.get (), current_program_space);
>
> > +  update_breakpoint_locations (b, current_program_space, sals, {});
> > +}
> > +
> >  /* Implement the "solib_create_inferior_hook" target_so_ops method.  */
> >
> >  static void
> > @@ -914,6 +1008,30 @@ windows_solib_create_inferior_hook (int from_tty)
> >        if (vmaddr != exec_base)
> >      objfile_rebase (symfile_objfile, exec_base - vmaddr);
> >      }
> > +
> > +  /* Create the entry point breakpoint if it doesn't exist already.  */
> > +  if (target_has_execution && exec_base != 0)
> > +    {
> > +      struct windows_info *info = get_windows_inferior_data ();
> > +      CORE_ADDR entry_point = exec_base
> > +    + pe_data (exec_bfd)->pe_opthdr.AddressOfEntryPoint;
> > +      info->entry_point = entry_point;
> > +
> > +      breakpoint *startup_breakpoint
> > +    = iterate_over_breakpoints ([] (breakpoint *bp)
> > +      {
> > +        return bp->ops == &entry_point_breakpoint_ops;
> > +      });
> > +      if (startup_breakpoint == nullptr)
> > +    {
> > +      event_location_up location
> > +        = new_address_location (entry_point, nullptr, 0);
> > +      create_breakpoint (target_gdbarch(), location.get(), nullptr, -1,
>
> Space before parens.
>
> This looking up for the pre-existing breakpoint doesn't work
> correctly when you consider multiple inferiors, where each will
> need a location for its own entry pointer.  The Windows backend
> doesn't support multi-process, but OTOH, if you do it like jit.c
> does, which just basically always create a breakpoint and
> stores the pointer in the per-pspace data, you're practically
> good to go, and you'll make it easier for whomever comes next
> and decides to all multi-process support.

I'm not sure what part here will not work.
I actually tested with multiple inferiors (not running at the same time),
and update_breakpoint_locations made a breakpoint location for each:

(gdb) maint info br
Num     Type           Disp Enb Address            What
-1      breakpoint     del  y   <MULTIPLE>
-1.1                        y   0x00000000004015b0 in mainCRTStartup at C:/gcc/src/mingw-w64-v7.0.0/mingw-w64-crt/crt/crtexe.c:218:1 inf 2
-1.2                        y   0x000000000117ee20 in mainCRTStartup at heob.c:7094:1 inf 1


Hannes

Re: [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point

[Hannes Domani]

 Ping.

Am Sonntag, 7. Juni 2020, 14:56:29 MESZ hat Hannes Domani via Gdb-patches <gdb-patches@sourceware.org> Folgendes geschrieben:

> Am Sonntag, 31. Mai 2020, 18:38:06 MESZ hat Pedro Alves <palves@redhat.com> Folgendes geschrieben:
>
> > On 5/25/20 7:56 PM, Hannes Domani via Gdb-patches wrote:
> >
> > > This patch creates an internal breakpoint on the process entry point, which
> > > when it is reached, resets all active hardware breakpoints, and continues
> > > execution.
> >
> > Missing ChangeLog entry.
> >
> > > ---
> > >  gdb/windows-tdep.c | 130 +++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 130 insertions(+)
> > >
> > > diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c
> > > index aa0adeba99..90e4794fc5 100644
> > > --- a/gdb/windows-tdep.c
> > > +++ b/gdb/windows-tdep.c
> > > @@ -37,6 +37,7 @@
> > >  #include "coff/internal.h"
> > >  #include "libcoff.h"
> > >  #include "solist.h"
> > > +#include "observable.h"
> > >
> > >  #define CYGWIN_DLL_NAME "cygwin1.dll"
> > >
> > > @@ -870,6 +871,99 @@ windows_get_siginfo_type (struct gdbarch *gdbarch)
> > >    return siginfo_type;
> > >  }
> > >
> > > +/* Windows-specific cached data.  This is used by GDB for caching
> > > +  purposes for each inferior.  This helps reduce the overhead of
> > > +  transfering data from a remote target to the local host.  */
> > > +struct windows_info
> > > +{
> > > +  CORE_ADDR entry_point = 0;
> > > +};
> > > +
> > > +/* Per-inferior data key.  */
> > > +static const struct inferior_key<windows_info> windows_inferior_data;
> >
> > This should be program_space_key / per-program_space data, instead of
> > per-inferior data.
> >
> > You may want to take a look at the jit.c code, which is doing similar
> > things.
>
> OK. I will change it to program_space_key, but why are there no observers
> to invalidate the a program-space, like these for inferiors?:
>
>   /* Observers used to invalidate the cache when needed.  */
>   gdb::observers::inferior_exit.attach (invalidate_windows_cache_inf);
>   gdb::observers::inferior_appeared.attach (invalidate_windows_cache_inf);
>
> Are they not needed for some reason?
>
>
> > > +
> > > +/* Frees whatever allocated space there is to be freed and sets INF's
> > > +  Windows cache data pointer to NULL.  */
> > > +
> > > +static void
> > > +invalidate_windows_cache_inf (struct inferior *inf)
> > > +{
> > > +  windows_inferior_data.clear (inf);
> > > +}
> > > +
> > > +/* Fetch the Windows cache info for INF.  This function always returns a
> > > +  valid INFO pointer.  */
> > > +
> > > +static struct windows_info *
> > > +get_windows_inferior_data (void)
> >
> > Drop the (void), only old pre-C++ code has it.  You can also drop
> > redundant "struct" throughout if you like.
> >
> > > +{
> > > +  struct windows_info *info;
> > > +  struct inferior *inf = current_inferior ();
> > > +
> > > +  info = windows_inferior_data.get (inf);
> > > +  if (info == NULL)
> > > +    info = windows_inferior_data.emplace (inf);
> > > +
> > > +  return info;
> > > +}
> > > +
> > > +/* Breakpoint on entry point where any active hardware breakpoints will
> > > +  be reset.  */
> >
> > Please expand the comments, explaining why this is necessary
> > in the first place.
> >
> > > +static struct breakpoint_ops entry_point_breakpoint_ops;
> > > +
> > > +/* Reset active hardware breakpoints.  */
> > > +
> > > +static bool
> > > +reset_hardware_breakpoints (struct breakpoint *b)
> > > +{
> > > +  if (b->type != bp_hardware_breakpoint
> > > +      && b->type != bp_hardware_watchpoint
> > > +      && b->type != bp_read_watchpoint
> > > +      && b->type != bp_access_watchpoint)
> > > +    return false;
> >
> > This should instead look at locations and their bp_loc_type,
> > looking for bp_loc_hardware_breakpoint / bp_loc_hardware_watchpoint.
> > There are situations where the breakpoint is a software breakpoint,
> > but GDB still inserts a hardware breakpoint, like e.g., due
> > to "set breakpoint auto-hw".
> >
> > > +
> > > +  struct bp_location *loc;
> > > +  for (loc = b->loc; loc; loc = loc->next)
> > > +    if (loc->enabled && loc->pspace == current_program_space
> > > +    && b->ops->remove_location (loc, REMOVE_BREAKPOINT) == 0)
> > > +      b->ops->insert_location (loc);
> >
> > This is incorrect for not considering whether a
> > breakpoint location was enabled but not inserted (e.g., the overall
> > breakpoint was disabled), or whether a breakpoint location was
> > a duplicate.
> >
> > You should instead look at loc->inserted.
> >
> > > +
> > > +  return false;
> > > +}
> > > +
> > > +/* This breakpoint type should never stop, but when reached, reset
> > > +  the active hardware breakpoints.  */
> >
> > hardware breakpoints and watchpoints.
> >
> > > +
> > > +static void
> > > +startup_breakpoint_check_status (bpstat bs)
> > > +{
> > > +  /* Never stop.  */
> > > +  bs->stop = 0;
> > > +
> > > +  iterate_over_breakpoints (reset_hardware_breakpoints);
> > > +}
> > > +
> > > +/* Update the breakpoint location to the current entry point.  */
> > > +
> > > +static void
> > > +startup_breakpoint_re_set (struct breakpoint *b)
> > > +{
> >
> > This is called if/when the loaded executable changes, even
> > without re-starting an inferior.  E.g., if you use the
> > "file" command after starting the inferior.  So this
> > should re-fetch the new entry point from the executable.
> > Again, take a look at the jit.c code.
>
> If I do "file" after "start", then windows_solib_create_inferior_hook is
> called before startup_breakpoint_re_set, so the new entry point was
> fetched already.
>
>
> > > +  struct windows_info *info = get_windows_inferior_data ();
> > > +  CORE_ADDR entry_point = info->entry_point;
> > > +
> > > +  /* Do nothing if the entry point didn't change.  */
> > > +  struct bp_location *loc;
> > > +  for (loc = b->loc; loc; loc = loc->next)
> > > +    if (loc->pspace == current_program_space && loc->address == entry_point)
> > > +      return;
> > > +
> > > +  event_location_up location
> > > +    = new_address_location (entry_point, nullptr, 0);
> > > +  std::vector<symtab_and_line> sals;
> > > +  sals = b->ops->decode_location (b, location.get (), current_program_space);
> >
> > Merge the two statements, so that you end up copy initialization, instead of
> > initialization, and then assignment:
> >
> >   std::vector<symtab_and_line> sals
> >     = b->ops->decode_location (b, location.get (), current_program_space);
> >
> > > +  update_breakpoint_locations (b, current_program_space, sals, {});
> > > +}
> > > +
> > >  /* Implement the "solib_create_inferior_hook" target_so_ops method.  */
> > >
> > >  static void
> > > @@ -914,6 +1008,30 @@ windows_solib_create_inferior_hook (int from_tty)
> > >        if (vmaddr != exec_base)
> > >      objfile_rebase (symfile_objfile, exec_base - vmaddr);
> > >      }
> > > +
> > > +  /* Create the entry point breakpoint if it doesn't exist already.  */
> > > +  if (target_has_execution && exec_base != 0)
> > > +    {
> > > +      struct windows_info *info = get_windows_inferior_data ();
> > > +      CORE_ADDR entry_point = exec_base
> > > +    + pe_data (exec_bfd)->pe_opthdr.AddressOfEntryPoint;
> > > +      info->entry_point = entry_point;
> > > +
> > > +      breakpoint *startup_breakpoint
> > > +    = iterate_over_breakpoints ([] (breakpoint *bp)
> > > +      {
> > > +        return bp->ops == &entry_point_breakpoint_ops;
> > > +      });
> > > +      if (startup_breakpoint == nullptr)
> > > +    {
> > > +      event_location_up location
> > > +        = new_address_location (entry_point, nullptr, 0);
> > > +      create_breakpoint (target_gdbarch(), location.get(), nullptr, -1,
> >
> > Space before parens.
> >
> > This looking up for the pre-existing breakpoint doesn't work
> > correctly when you consider multiple inferiors, where each will
> > need a location for its own entry pointer.  The Windows backend
> > doesn't support multi-process, but OTOH, if you do it like jit.c
> > does, which just basically always create a breakpoint and
> > stores the pointer in the per-pspace data, you're practically
> > good to go, and you'll make it easier for whomever comes next
> > and decides to all multi-process support.
>
> I'm not sure what part here will not work.
> I actually tested with multiple inferiors (not running at the same time),
> and update_breakpoint_locations made a breakpoint location for each:
>
> (gdb) maint info br
> Num     Type           Disp Enb Address            What
> -1      breakpoint     del  y   <MULTIPLE>
> -1.1                        y   0x00000000004015b0 in mainCRTStartup at C:/gcc/src/mingw-w64-v7.0.0/mingw-w64-crt/crt/crtexe.c:218:1 inf 2
> -1.2                        y   0x000000000117ee20 in mainCRTStartup at heob.c:7094:1 inf 1

Re: [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point

[Pedro Alves]

Hi Hannes,

On 6/7/20 1:56 PM, Hannes Domani via Gdb-patches wrote:
>  Am Sonntag, 31. Mai 2020, 18:38:06 MESZ hat Pedro Alves <palves@redhat.com> Folgendes geschrieben:
> 
>> On 5/25/20 7:56 PM, Hannes Domani via Gdb-patches wrote:
>>
>>> This patch creates an internal breakpoint on the process entry point, which
>>> when it is reached, resets all active hardware breakpoints, and continues
>>> execution.
>>
>> Missing ChangeLog entry.
>>
>>> ---
>>>   gdb/windows-tdep.c | 130 +++++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 130 insertions(+)
>>>
>>> diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c
>>> index aa0adeba99..90e4794fc5 100644
>>> --- a/gdb/windows-tdep.c
>>> +++ b/gdb/windows-tdep.c
>>> @@ -37,6 +37,7 @@
>>>   #include "coff/internal.h"
>>>   #include "libcoff.h"
>>>   #include "solist.h"
>>> +#include "observable.h"
>>>
>>>   #define CYGWIN_DLL_NAME "cygwin1.dll"
>>>
>>> @@ -870,6 +871,99 @@ windows_get_siginfo_type (struct gdbarch *gdbarch)
>>>     return siginfo_type;
>>>   }
>>>
>>> +/* Windows-specific cached data.  This is used by GDB for caching
>>> +  purposes for each inferior.  This helps reduce the overhead of
>>> +  transfering data from a remote target to the local host.  */
>>> +struct windows_info
>>> +{
>>> +  CORE_ADDR entry_point = 0;
>>> +};
>>> +
>>> +/* Per-inferior data key.  */
>>> +static const struct inferior_key<windows_info> windows_inferior_data;
>>
>> This should be program_space_key / per-program_space data, instead of
>> per-inferior data.
>>
>> You may want to take a look at the jit.c code, which is doing similar
>> things.
> 
> OK. I will change it to program_space_key, but why are there no observers
> to invalidate the a program-space, like these for inferiors?:
> 
>   /* Observers used to invalidate the cache when needed.  */
>   gdb::observers::inferior_exit.attach (invalidate_windows_cache_inf);
>   gdb::observers::inferior_appeared.attach (invalidate_windows_cache_inf);
> 
> Are they not needed for some reason?

These notifications are more about the event that happened than about
invalidating the inferior.  Some observers may use the notification
for that, others for other things.

Probably nobody found a need for matching program space notifications
because so far the existing ones have covered all needs.

> 
> 
>>> +
>>> +/* Frees whatever allocated space there is to be freed and sets INF's
>>> +  Windows cache data pointer to NULL.  */
>>> +
>>> +static void
>>> +invalidate_windows_cache_inf (struct inferior *inf)
>>> +{
>>> +  windows_inferior_data.clear (inf);
>>> +}
>>> +
>>> +/* Fetch the Windows cache info for INF.  This function always returns a
>>> +  valid INFO pointer.  */
>>> +
>>> +static struct windows_info *
>>> +get_windows_inferior_data (void)
>>
>> Drop the (void), only old pre-C++ code has it.  You can also drop
>> redundant "struct" throughout if you like.
>>
>>> +{
>>> +  struct windows_info *info;
>>> +  struct inferior *inf = current_inferior ();
>>> +
>>> +  info = windows_inferior_data.get (inf);
>>> +  if (info == NULL)
>>> +    info = windows_inferior_data.emplace (inf);
>>> +
>>> +  return info;
>>> +}
>>> +
>>> +/* Breakpoint on entry point where any active hardware breakpoints will
>>> +  be reset.  */
>>
>> Please expand the comments, explaining why this is necessary
>> in the first place.
>>
>>> +static struct breakpoint_ops entry_point_breakpoint_ops;
>>> +
>>> +/* Reset active hardware breakpoints.  */
>>> +
>>> +static bool
>>> +reset_hardware_breakpoints (struct breakpoint *b)
>>> +{
>>> +  if (b->type != bp_hardware_breakpoint
>>> +      && b->type != bp_hardware_watchpoint
>>> +      && b->type != bp_read_watchpoint
>>> +      && b->type != bp_access_watchpoint)
>>> +    return false;
>>
>> This should instead look at locations and their bp_loc_type,
>> looking for bp_loc_hardware_breakpoint / bp_loc_hardware_watchpoint.
>> There are situations where the breakpoint is a software breakpoint,
>> but GDB still inserts a hardware breakpoint, like e.g., due
>> to "set breakpoint auto-hw".
>>
>>> +
>>> +  struct bp_location *loc;
>>> +  for (loc = b->loc; loc; loc = loc->next)
>>> +    if (loc->enabled && loc->pspace == current_program_space
>>> +    && b->ops->remove_location (loc, REMOVE_BREAKPOINT) == 0)
>>> +      b->ops->insert_location (loc);
>>
>> This is incorrect for not considering whether a
>> breakpoint location was enabled but not inserted (e.g., the overall
>> breakpoint was disabled), or whether a breakpoint location was
>> a duplicate.
>>
>> You should instead look at loc->inserted.
>>
>>> +
>>> +  return false;
>>> +}
>>> +
>>> +/* This breakpoint type should never stop, but when reached, reset
>>> +  the active hardware breakpoints.  */
>>
>> hardware breakpoints and watchpoints.
>>
>>> +
>>> +static void
>>> +startup_breakpoint_check_status (bpstat bs)
>>> +{
>>> +  /* Never stop.  */
>>> +  bs->stop = 0;
>>> +
>>> +  iterate_over_breakpoints (reset_hardware_breakpoints);
>>> +}
>>> +
>>> +/* Update the breakpoint location to the current entry point.  */
>>> +
>>> +static void
>>> +startup_breakpoint_re_set (struct breakpoint *b)
>>> +{
>>
>> This is called if/when the loaded executable changes, even
>> without re-starting an inferior.  E.g., if you use the
>> "file" command after starting the inferior.  So this
>> should re-fetch the new entry point from the executable.
>> Again, take a look at the jit.c code.
> 
> If I do "file" after "start", then windows_solib_create_inferior_hook is
> called before startup_breakpoint_re_set, so the new entry point was
> fetched already.
> 
> 
>>> +  struct windows_info *info = get_windows_inferior_data ();
>>> +  CORE_ADDR entry_point = info->entry_point;
>>> +
>>> +  /* Do nothing if the entry point didn't change.  */
>>> +  struct bp_location *loc;
>>> +  for (loc = b->loc; loc; loc = loc->next)
>>> +    if (loc->pspace == current_program_space && loc->address == entry_point)
>>> +      return;
>>> +
>>> +  event_location_up location
>>> +    = new_address_location (entry_point, nullptr, 0);
>>> +  std::vector<symtab_and_line> sals;
>>> +  sals = b->ops->decode_location (b, location.get (), current_program_space);
>>
>> Merge the two statements, so that you end up copy initialization, instead of
>> initialization, and then assignment:
>>
>>    std::vector<symtab_and_line> sals
>>      = b->ops->decode_location (b, location.get (), current_program_space);
>>
>>> +  update_breakpoint_locations (b, current_program_space, sals, {});
>>> +}
>>> +
>>>   /* Implement the "solib_create_inferior_hook" target_so_ops method.  */
>>>
>>>   static void
>>> @@ -914,6 +1008,30 @@ windows_solib_create_inferior_hook (int from_tty)
>>>         if (vmaddr != exec_base)
>>>       objfile_rebase (symfile_objfile, exec_base - vmaddr);
>>>       }
>>> +
>>> +  /* Create the entry point breakpoint if it doesn't exist already.  */
>>> +  if (target_has_execution && exec_base != 0)
>>> +    {
>>> +      struct windows_info *info = get_windows_inferior_data ();
>>> +      CORE_ADDR entry_point = exec_base
>>> +    + pe_data (exec_bfd)->pe_opthdr.AddressOfEntryPoint;
>>> +      info->entry_point = entry_point;
>>> +
>>> +      breakpoint *startup_breakpoint
>>> +    = iterate_over_breakpoints ([] (breakpoint *bp)
>>> +      {
>>> +        return bp->ops == &entry_point_breakpoint_ops;
>>> +      });
>>> +      if (startup_breakpoint == nullptr)
>>> +    {
>>> +      event_location_up location
>>> +        = new_address_location (entry_point, nullptr, 0);
>>> +      create_breakpoint (target_gdbarch(), location.get(), nullptr, -1,
>>
>> Space before parens.
>>
>> This looking up for the pre-existing breakpoint doesn't work
>> correctly when you consider multiple inferiors, where each will
>> need a location for its own entry pointer.  The Windows backend
>> doesn't support multi-process, but OTOH, if you do it like jit.c
>> does, which just basically always create a breakpoint and
>> stores the pointer in the per-pspace data, you're practically
>> good to go, and you'll make it easier for whomever comes next
>> and decides to all multi-process support.
> 
> I'm not sure what part here will not work.
> I actually tested with multiple inferiors (not running at the same time),
> and update_breakpoint_locations made a breakpoint location for each:
> 
> (gdb) maint info br
> Num     Type           Disp Enb Address            What
> -1      breakpoint     del  y   <MULTIPLE>
> -1.1                        y   0x00000000004015b0 in mainCRTStartup at C:/gcc/src/mingw-w64-v7.0.0/mingw-w64-crt/crt/crtexe.c:218:1 inf 2
> -1.2                        y   0x000000000117ee20 in mainCRTStartup at heob.c:7094:1 inf 1
> 

I looked better at the code, and as long as startup_breakpoint_re_set
is called whenever an inferior is added, I agree it should work.

However, does the breakpoint go away when the inferior exits, though?
I'm wondering what happens when, say:

#1 - you load a Windows binary in inferior 1.
#2 - you run the inferior, and it exits
#3 - you now load a non-Windows binary in inferior 1 (say, a GNU/Linux program)
#4 - you run inferior 1

Don't we end up in startup_breakpoint_re_set in step #3 or #4?
If so, that calls get_windows_inferior_data() which a new
windows_info object, and then creates a location at address 0,
I presume.  All while the inferior isn't a Windows inferior.

Or what if you load a Windows program in inferior 1, and a GNU/Linux
program in inferior 2?  Doesn't a similar problem happen?

Thanks,
Pedro Alves

Re: [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point

[Hannes Domani]

 Am Freitag, 9. Oktober 2020, 20:22:21 MESZ hat Pedro Alves <pedro@palves.net> Folgendes geschrieben:

> Hi Hannes,
>
> On 6/7/20 1:56 PM, Hannes Domani via Gdb-patches wrote:
> >  Am Sonntag, 31. Mai 2020, 18:38:06 MESZ hat Pedro Alves <palves@redhat.com> Folgendes geschrieben:
> >
> >> On 5/25/20 7:56 PM, Hannes Domani via Gdb-patches wrote:
> >>
> >>> This patch creates an internal breakpoint on the process entry point, which
> >>> when it is reached, resets all active hardware breakpoints, and continues
> >>> execution.
> >>
> >> Missing ChangeLog entry.
> >>
> >>> ---
> >>>   gdb/windows-tdep.c | 130 +++++++++++++++++++++++++++++++++++++++++++++
> >>>   1 file changed, 130 insertions(+)
> >>>
> >>> diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c
> >>> index aa0adeba99..90e4794fc5 100644
> >>> --- a/gdb/windows-tdep.c
> >>> +++ b/gdb/windows-tdep.c
> >>> @@ -37,6 +37,7 @@
> >>>   #include "coff/internal.h"
> >>>   #include "libcoff.h"
> >>>   #include "solist.h"
> >>> +#include "observable.h"
> >>>
> >>>   #define CYGWIN_DLL_NAME "cygwin1.dll"
> >>>
> >>> @@ -870,6 +871,99 @@ windows_get_siginfo_type (struct gdbarch *gdbarch)
> >>>     return siginfo_type;
> >>>   }
> >>>
> >>> +/* Windows-specific cached data.  This is used by GDB for caching
> >>> +  purposes for each inferior.  This helps reduce the overhead of
> >>> +  transfering data from a remote target to the local host.  */
> >>> +struct windows_info
> >>> +{
> >>> +  CORE_ADDR entry_point = 0;
> >>> +};
> >>> +
> >>> +/* Per-inferior data key.  */
> >>> +static const struct inferior_key<windows_info> windows_inferior_data;
> >>
> >> This should be program_space_key / per-program_space data, instead of
> >> per-inferior data.
> >>
> >> You may want to take a look at the jit.c code, which is doing similar
> >> things.
> >
> > OK. I will change it to program_space_key, but why are there no observers
> > to invalidate the a program-space, like these for inferiors?:
> >
> >   /* Observers used to invalidate the cache when needed.  */
> >   gdb::observers::inferior_exit.attach (invalidate_windows_cache_inf);
> >   gdb::observers::inferior_appeared.attach (invalidate_windows_cache_inf);
> >
> > Are they not needed for some reason?
>
> These notifications are more about the event that happened than about
> invalidating the inferior.  Some observers may use the notification
> for that, others for other things.
>
> Probably nobody found a need for matching program space notifications
> because so far the existing ones have covered all needs.

I see.


> >
> >>> +
> >>> +/* Frees whatever allocated space there is to be freed and sets INF's
> >>> +  Windows cache data pointer to NULL.  */
> >>> +
> >>> +static void
> >>> +invalidate_windows_cache_inf (struct inferior *inf)
> >>> +{
> >>> +  windows_inferior_data.clear (inf);
> >>> +}
> >>> +
> >>> +/* Fetch the Windows cache info for INF.  This function always returns a
> >>> +  valid INFO pointer.  */
> >>> +
> >>> +static struct windows_info *
> >>> +get_windows_inferior_data (void)
> >>
> >> Drop the (void), only old pre-C++ code has it.  You can also drop
> >> redundant "struct" throughout if you like.
> >>
> >>> +{
> >>> +  struct windows_info *info;
> >>> +  struct inferior *inf = current_inferior ();
> >>> +
> >>> +  info = windows_inferior_data.get (inf);
> >>> +  if (info == NULL)
> >>> +    info = windows_inferior_data.emplace (inf);
> >>> +
> >>> +  return info;
> >>> +}
> >>> +
> >>> +/* Breakpoint on entry point where any active hardware breakpoints will
> >>> +  be reset.  */
> >>
> >> Please expand the comments, explaining why this is necessary
> >> in the first place.
> >>
> >>> +static struct breakpoint_ops entry_point_breakpoint_ops;
> >>> +
> >>> +/* Reset active hardware breakpoints.  */
> >>> +
> >>> +static bool
> >>> +reset_hardware_breakpoints (struct breakpoint *b)
> >>> +{
> >>> +  if (b->type != bp_hardware_breakpoint
> >>> +      && b->type != bp_hardware_watchpoint
> >>> +      && b->type != bp_read_watchpoint
> >>> +      && b->type != bp_access_watchpoint)
> >>> +    return false;
> >>
> >> This should instead look at locations and their bp_loc_type,
> >> looking for bp_loc_hardware_breakpoint / bp_loc_hardware_watchpoint.
> >> There are situations where the breakpoint is a software breakpoint,
> >> but GDB still inserts a hardware breakpoint, like e.g., due
> >> to "set breakpoint auto-hw".
> >>
> >>> +
> >>> +  struct bp_location *loc;
> >>> +  for (loc = b->loc; loc; loc = loc->next)
> >>> +    if (loc->enabled && loc->pspace == current_program_space
> >>> +    && b->ops->remove_location (loc, REMOVE_BREAKPOINT) == 0)
> >>> +      b->ops->insert_location (loc);
> >>
> >> This is incorrect for not considering whether a
> >> breakpoint location was enabled but not inserted (e.g., the overall
> >> breakpoint was disabled), or whether a breakpoint location was
> >> a duplicate.
> >>
> >> You should instead look at loc->inserted.
> >>
> >>> +
> >>> +  return false;
> >>> +}
> >>> +
> >>> +/* This breakpoint type should never stop, but when reached, reset
> >>> +  the active hardware breakpoints.  */
> >>
> >> hardware breakpoints and watchpoints.
> >>
> >>> +
> >>> +static void
> >>> +startup_breakpoint_check_status (bpstat bs)
> >>> +{
> >>> +  /* Never stop.  */
> >>> +  bs->stop = 0;
> >>> +
> >>> +  iterate_over_breakpoints (reset_hardware_breakpoints);
> >>> +}
> >>> +
> >>> +/* Update the breakpoint location to the current entry point.  */
> >>> +
> >>> +static void
> >>> +startup_breakpoint_re_set (struct breakpoint *b)
> >>> +{
> >>
> >> This is called if/when the loaded executable changes, even
> >> without re-starting an inferior.  E.g., if you use the
> >> "file" command after starting the inferior.  So this
> >> should re-fetch the new entry point from the executable.
> >> Again, take a look at the jit.c code.
> >
> > If I do "file" after "start", then windows_solib_create_inferior_hook is
> > called before startup_breakpoint_re_set, so the new entry point was
> > fetched already.
> >
> >
> >>> +  struct windows_info *info = get_windows_inferior_data ();
> >>> +  CORE_ADDR entry_point = info->entry_point;
> >>> +
> >>> +  /* Do nothing if the entry point didn't change.  */
> >>> +  struct bp_location *loc;
> >>> +  for (loc = b->loc; loc; loc = loc->next)
> >>> +    if (loc->pspace == current_program_space && loc->address == entry_point)
> >>> +      return;
> >>> +
> >>> +  event_location_up location
> >>> +    = new_address_location (entry_point, nullptr, 0);
> >>> +  std::vector<symtab_and_line> sals;
> >>> +  sals = b->ops->decode_location (b, location.get (), current_program_space);
> >>
> >> Merge the two statements, so that you end up copy initialization, instead of
> >> initialization, and then assignment:
> >>
> >>    std::vector<symtab_and_line> sals
> >>      = b->ops->decode_location (b, location.get (), current_program_space);
> >>
> >>> +  update_breakpoint_locations (b, current_program_space, sals, {});
> >>> +}
> >>> +
> >>>   /* Implement the "solib_create_inferior_hook" target_so_ops method.  */
> >>>
> >>>   static void
> >>> @@ -914,6 +1008,30 @@ windows_solib_create_inferior_hook (int from_tty)
> >>>         if (vmaddr != exec_base)
> >>>       objfile_rebase (symfile_objfile, exec_base - vmaddr);
> >>>       }
> >>> +
> >>> +  /* Create the entry point breakpoint if it doesn't exist already.  */
> >>> +  if (target_has_execution && exec_base != 0)
> >>> +    {
> >>> +      struct windows_info *info = get_windows_inferior_data ();
> >>> +      CORE_ADDR entry_point = exec_base
> >>> +    + pe_data (exec_bfd)->pe_opthdr.AddressOfEntryPoint;
> >>> +      info->entry_point = entry_point;
> >>> +
> >>> +      breakpoint *startup_breakpoint
> >>> +    = iterate_over_breakpoints ([] (breakpoint *bp)
> >>> +      {
> >>> +        return bp->ops == &entry_point_breakpoint_ops;
> >>> +      });
> >>> +      if (startup_breakpoint == nullptr)
> >>> +    {
> >>> +      event_location_up location
> >>> +        = new_address_location (entry_point, nullptr, 0);
> >>> +      create_breakpoint (target_gdbarch(), location.get(), nullptr, -1,
> >>
> >> Space before parens.
> >>
> >> This looking up for the pre-existing breakpoint doesn't work
> >> correctly when you consider multiple inferiors, where each will
> >> need a location for its own entry pointer.  The Windows backend
> >> doesn't support multi-process, but OTOH, if you do it like jit.c
> >> does, which just basically always create a breakpoint and
> >> stores the pointer in the per-pspace data, you're practically
> >> good to go, and you'll make it easier for whomever comes next
> >> and decides to all multi-process support.
> >
> > I'm not sure what part here will not work.
> > I actually tested with multiple inferiors (not running at the same time),
> > and update_breakpoint_locations made a breakpoint location for each:
> >
> > (gdb) maint info br
> > Num     Type           Disp Enb Address            What
> > -1      breakpoint     del  y   <MULTIPLE>
> > -1.1                        y   0x00000000004015b0 in mainCRTStartup at C:/gcc/src/mingw-w64-v7.0.0/mingw-w64-crt/crt/crtexe.c:218:1 inf 2
> > -1.2                        y   0x000000000117ee20 in mainCRTStartup at heob.c:7094:1 inf 1
> >
>
> I looked better at the code, and as long as startup_breakpoint_re_set
> is called whenever an inferior is added, I agree it should work.
>
> However, does the breakpoint go away when the inferior exits, though?
> I'm wondering what happens when, say:
>
> #1 - you load a Windows binary in inferior 1.
> #2 - you run the inferior, and it exits
> #3 - you now load a non-Windows binary in inferior 1 (say, a GNU/Linux program)
> #4 - you run inferior 1
>
> Don't we end up in startup_breakpoint_re_set in step #3 or #4?
> If so, that calls get_windows_inferior_data() which a new
> windows_info object, and then creates a location at address 0,
> I presume.  All while the inferior isn't a Windows inferior.
>
> Or what if you load a Windows program in inferior 1, and a GNU/Linux
> program in inferior 2?  Doesn't a similar problem happen?

You're probably right.
I've never build a gdb which understands both Windows and Linux
executables (I think), so I can't test it.

It looks like it keeps the breakpoint on the same location as the last
Windows executable.

What would you suggest how to fix this?


Hannes

Re: [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point

[Pedro Alves]

On 10/9/20 7:51 PM, Hannes Domani wrote:
>  Am Freitag, 9. Oktober 2020, 20:22:21 MESZ hat Pedro Alves <pedro@palves.net> Folgendes geschrieben:
> 
>> However, does the breakpoint go away when the inferior exits, though?
>> I'm wondering what happens when, say:
>>
>> #1 - you load a Windows binary in inferior 1.
>> #2 - you run the inferior, and it exits
>> #3 - you now load a non-Windows binary in inferior 1 (say, a GNU/Linux program)
>> #4 - you run inferior 1
>>
>> Don't we end up in startup_breakpoint_re_set in step #3 or #4?
>> If so, that calls get_windows_inferior_data() which a new
>> windows_info object, and then creates a location at address 0,
>> I presume.  All while the inferior isn't a Windows inferior.
>>
>> Or what if you load a Windows program in inferior 1, and a GNU/Linux
>> program in inferior 2?  Doesn't a similar problem happen?
> 
> You're probably right.
> I've never build a gdb which understands both Windows and Linux
> executables (I think), so I can't test it.

You just need to build gdb with --enable-targets=all

> 
> It looks like it keeps the breakpoint on the same location as the last
> Windows executable.
> 
> What would you suggest how to fix this?

I think you need to add some checks for "is this a Windows inferior".

Maybe with something like:

static bool
is_windows_inferior (inferior *inf)
{
  switch (gdbarch_osabi (inf->gdbarch))
    {
    case GDB_OSABI_CYGWIN:
    case GDB_OSABI_WINDOW:
      return true;
    }
  return false;
}

In startup_breakpoint_re_set, don't create locations if the
inferior isn't a Windows inferior.

Then in your gdb::observers::inferior_exit observer, delete the
breakpoint locations for the exiting inferior.

I still suspect that having a breakpoint per inferior/pspace,
and a keeping a pointer to it in the per-inf/pspace structure (you
can get at the current entry point address from the breakpoint's
first location's address instead of a separate field for the
entry point address), instead of a single breakpoint with multiple
locations would simplify things.  But if you want to keep your
current approach, that's fine with me.

Re: [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves via Gdb-patches <gdb-patches@sourceware.org> writes:

>> +/* Per-inferior data key.  */
>> +static const struct inferior_key<windows_info> windows_inferior_data;

Pedro> This should be program_space_key / per-program_space data, instead of
Pedro> per-inferior data.

I didn't really read this patch, but couldn't this also use
inferior::priv?  That seems to be what remote.c and darwin-nat.c do for
data that is target-specific but also related to a particular inferior.

Tom

Re: [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point

[Tom Tromey]

Tom> I didn't really read this patch, but couldn't this also use
Tom> inferior::priv?  That seems to be what remote.c and darwin-nat.c do for
Tom> data that is target-specific but also related to a particular inferior.

Oh, never mind.  I see this is in windows-tdep, not windows-nat.

Tom