* [PATCH 1/7] Fix function argument and return value locations
2020-05-25 18:56 ` Windows testsuite failures Hannes Domani
@ 2020-05-25 18:56 ` Hannes Domani
2020-05-25 21:02 ` Simon Marchi
2020-05-25 18:56 ` [PATCH 2/7] Handle Windows drives in auto-load script paths Hannes Domani
` (6 subsequent siblings)
7 siblings, 1 reply; 44+ messages in thread
From: Hannes Domani @ 2020-05-25 18:56 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH 1/7] Fix function argument and return value locations
2020-05-25 18:56 ` [PATCH 1/7] Fix function argument and return value locations Hannes Domani
@ 2020-05-25 21:02 ` Simon Marchi
2020-05-25 21:32 ` Hannes Domani
0 siblings, 1 reply; 44+ messages in thread
From: Simon Marchi @ 2020-05-25 21:02 UTC (permalink / raw)
To: Hannes Domani, gdb-patches
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
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH 1/7] Fix function argument and return value locations
2020-05-25 21:02 ` Simon Marchi
@ 2020-05-25 21:32 ` Hannes Domani
2020-05-25 22:14 ` Simon Marchi
2020-05-26 20:43 ` Tom Tromey
0 siblings, 2 replies; 44+ messages in thread
From: Hannes Domani @ 2020-05-25 21:32 UTC (permalink / raw)
To: Gdb-patches
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
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH 1/7] Fix function argument and return value locations
2020-05-25 21:32 ` Hannes Domani
@ 2020-05-25 22:14 ` Simon Marchi
2020-05-25 23:03 ` Hannes Domani
2020-05-26 20:43 ` Tom Tromey
1 sibling, 1 reply; 44+ messages in thread
From: Simon Marchi @ 2020-05-25 22:14 UTC (permalink / raw)
To: Hannes Domani, Gdb-patches
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
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH 1/7] Fix function argument and return value locations
2020-05-25 22:14 ` Simon Marchi
@ 2020-05-25 23:03 ` Hannes Domani
2020-05-26 16:14 ` Simon Marchi
0 siblings, 1 reply; 44+ messages in thread
From: Hannes Domani @ 2020-05-25 23:03 UTC (permalink / raw)
To: Gdb-patches
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
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH 1/7] Fix function argument and return value locations
2020-05-25 23:03 ` Hannes Domani
@ 2020-05-26 16:14 ` Simon Marchi
0 siblings, 0 replies; 44+ messages in thread
From: Simon Marchi @ 2020-05-26 16:14 UTC (permalink / raw)
To: Hannes Domani, Gdb-patches
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 1/7] Fix function argument and return value locations
2020-05-25 21:32 ` Hannes Domani
2020-05-25 22:14 ` Simon Marchi
@ 2020-05-26 20:43 ` Tom Tromey
1 sibling, 0 replies; 44+ messages in thread
From: Tom Tromey @ 2020-05-26 20:43 UTC (permalink / raw)
To: Hannes Domani via Gdb-patches
>>>>> "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
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 2/7] Handle Windows drives in auto-load script paths
2020-05-25 18:56 ` Windows testsuite failures Hannes Domani
2020-05-25 18:56 ` [PATCH 1/7] Fix function argument and return value locations Hannes Domani
@ 2020-05-25 18:56 ` Hannes Domani
2020-05-26 16:04 ` Jon Turney
2020-05-26 16:05 ` Christian Biesinger
2020-05-25 18:56 ` [PATCH 3/7] Handle Windows drives in rbreak paths Hannes Domani
` (5 subsequent siblings)
7 siblings, 2 replies; 44+ messages in thread
From: Hannes Domani @ 2020-05-25 18:56 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH 2/7] Handle Windows drives in auto-load script paths
2020-05-25 18:56 ` [PATCH 2/7] Handle Windows drives in auto-load script paths Hannes Domani
@ 2020-05-26 16:04 ` Jon Turney
2020-05-26 16:31 ` Hannes Domani
2020-05-26 16:05 ` Christian Biesinger
1 sibling, 1 reply; 44+ messages in thread
From: Jon Turney @ 2020-05-26 16:04 UTC (permalink / raw)
To: Hannes Domani; +Cc: gdb-patches
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. */
>
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH 2/7] Handle Windows drives in auto-load script paths
2020-05-26 16:04 ` Jon Turney
@ 2020-05-26 16:31 ` Hannes Domani
0 siblings, 0 replies; 44+ messages in thread
From: Hannes Domani @ 2020-05-26 16:31 UTC (permalink / raw)
To: Gdb-patches
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 2/7] Handle Windows drives in auto-load script paths
2020-05-25 18:56 ` [PATCH 2/7] Handle Windows drives in auto-load script paths Hannes Domani
2020-05-26 16:04 ` Jon Turney
@ 2020-05-26 16:05 ` Christian Biesinger
2020-05-26 16:25 ` Hannes Domani
1 sibling, 1 reply; 44+ messages in thread
From: Christian Biesinger @ 2020-05-26 16:05 UTC (permalink / raw)
To: Hannes Domani; +Cc: gdb-patches
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
>
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH 2/7] Handle Windows drives in auto-load script paths
2020-05-26 16:05 ` Christian Biesinger
@ 2020-05-26 16:25 ` Hannes Domani
2020-05-26 16:31 ` Christian Biesinger
0 siblings, 1 reply; 44+ messages in thread
From: Hannes Domani @ 2020-05-26 16:25 UTC (permalink / raw)
To: Gdb-patches
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 2/7] Handle Windows drives in auto-load script paths
2020-05-26 16:25 ` Hannes Domani
@ 2020-05-26 16:31 ` Christian Biesinger
2020-05-26 16:40 ` Hannes Domani
0 siblings, 1 reply; 44+ messages in thread
From: Christian Biesinger @ 2020-05-26 16:31 UTC (permalink / raw)
To: Hannes Domani; +Cc: Gdb-patches
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 2/7] Handle Windows drives in auto-load script paths
2020-05-26 16:31 ` Christian Biesinger
@ 2020-05-26 16:40 ` Hannes Domani
2020-05-26 16:42 ` Christian Biesinger
0 siblings, 1 reply; 44+ messages in thread
From: Hannes Domani @ 2020-05-26 16:40 UTC (permalink / raw)
To: Gdb-patches
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 2/7] Handle Windows drives in auto-load script paths
2020-05-26 16:40 ` Hannes Domani
@ 2020-05-26 16:42 ` Christian Biesinger
2020-05-26 17:14 ` Hannes Domani
0 siblings, 1 reply; 44+ messages in thread
From: Christian Biesinger @ 2020-05-26 16:42 UTC (permalink / raw)
To: Hannes Domani; +Cc: Gdb-patches
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 2/7] Handle Windows drives in auto-load script paths
2020-05-26 16:42 ` Christian Biesinger
@ 2020-05-26 17:14 ` Hannes Domani
0 siblings, 0 replies; 44+ messages in thread
From: Hannes Domani @ 2020-05-26 17:14 UTC (permalink / raw)
To: Gdb-patches
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 3/7] Handle Windows drives in rbreak paths
2020-05-25 18:56 ` Windows testsuite failures Hannes Domani
2020-05-25 18:56 ` [PATCH 1/7] Fix function argument and return value locations Hannes Domani
2020-05-25 18:56 ` [PATCH 2/7] Handle Windows drives in auto-load script paths Hannes Domani
@ 2020-05-25 18:56 ` Hannes Domani
2020-05-25 18:56 ` [PATCH 4/7] Use errno value of first openp failure Hannes Domani
` (4 subsequent siblings)
7 siblings, 0 replies; 44+ messages in thread
From: Hannes Domani @ 2020-05-25 18:56 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 44+ messages in thread* [PATCH 4/7] Use errno value of first openp failure
2020-05-25 18:56 ` Windows testsuite failures Hannes Domani
` (2 preceding siblings ...)
2020-05-25 18:56 ` [PATCH 3/7] Handle Windows drives in rbreak paths Hannes Domani
@ 2020-05-25 18:56 ` Hannes Domani
2020-05-26 20:37 ` Tom Tromey
2020-05-25 18:56 ` [PATCH 5/7] Close file handle of empty history file Hannes Domani
` (3 subsequent siblings)
7 siblings, 1 reply; 44+ messages in thread
From: Hannes Domani @ 2020-05-25 18:56 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 44+ messages in thread* [PATCH 5/7] Close file handle of empty history file
2020-05-25 18:56 ` Windows testsuite failures Hannes Domani
` (3 preceding siblings ...)
2020-05-25 18:56 ` [PATCH 4/7] Use errno value of first openp failure Hannes Domani
@ 2020-05-25 18:56 ` Hannes Domani
2020-05-26 16:37 ` Christian Biesinger
2020-05-25 18:56 ` [PATCH 6/7] Move exit_status_set_internal_vars out of GLOBAL_CURDIR Hannes Domani
` (2 subsequent siblings)
7 siblings, 1 reply; 44+ messages in thread
From: Hannes Domani @ 2020-05-25 18:56 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH 5/7] Close file handle of empty history file
2020-05-25 18:56 ` [PATCH 5/7] Close file handle of empty history file Hannes Domani
@ 2020-05-26 16:37 ` Christian Biesinger
2020-05-26 17:42 ` Hannes Domani
0 siblings, 1 reply; 44+ messages in thread
From: Christian Biesinger @ 2020-05-26 16:37 UTC (permalink / raw)
To: Hannes Domani; +Cc: gdb-patches
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
>
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH 5/7] Close file handle of empty history file
2020-05-26 16:37 ` Christian Biesinger
@ 2020-05-26 17:42 ` Hannes Domani
2020-05-27 14:33 ` Tom Tromey
0 siblings, 1 reply; 44+ messages in thread
From: Hannes Domani @ 2020-05-26 17:42 UTC (permalink / raw)
To: Gdb-patches
> 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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 5/7] Close file handle of empty history file
2020-05-26 17:42 ` Hannes Domani
@ 2020-05-27 14:33 ` Tom Tromey
2020-05-27 17:37 ` Hannes Domani
0 siblings, 1 reply; 44+ messages in thread
From: Tom Tromey @ 2020-05-27 14:33 UTC (permalink / raw)
To: Hannes Domani via Gdb-patches
>>>>> "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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 5/7] Close file handle of empty history file
2020-05-27 14:33 ` Tom Tromey
@ 2020-05-27 17:37 ` Hannes Domani
2020-05-27 18:27 ` Christian Biesinger
0 siblings, 1 reply; 44+ messages in thread
From: Hannes Domani @ 2020-05-27 17:37 UTC (permalink / raw)
To: Gdb-patches
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 6/7] Move exit_status_set_internal_vars out of GLOBAL_CURDIR
2020-05-25 18:56 ` Windows testsuite failures Hannes Domani
` (4 preceding siblings ...)
2020-05-25 18:56 ` [PATCH 5/7] Close file handle of empty history file Hannes Domani
@ 2020-05-25 18:56 ` Hannes Domani
2020-05-26 20:45 ` Tom Tromey
2020-05-25 18:56 ` [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point Hannes Domani
2020-05-28 18:15 ` Windows testsuite failures Christian Biesinger
7 siblings, 1 reply; 44+ messages in thread
From: Hannes Domani @ 2020-05-25 18:56 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH 6/7] Move exit_status_set_internal_vars out of GLOBAL_CURDIR
2020-05-25 18:56 ` [PATCH 6/7] Move exit_status_set_internal_vars out of GLOBAL_CURDIR Hannes Domani
@ 2020-05-26 20:45 ` Tom Tromey
2020-05-27 17:50 ` Hannes Domani
0 siblings, 1 reply; 44+ messages in thread
From: Tom Tromey @ 2020-05-26 20:45 UTC (permalink / raw)
To: Hannes Domani via Gdb-patches
>>>>> "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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 6/7] Move exit_status_set_internal_vars out of GLOBAL_CURDIR
2020-05-26 20:45 ` Tom Tromey
@ 2020-05-27 17:50 ` Hannes Domani
0 siblings, 0 replies; 44+ messages in thread
From: Hannes Domani @ 2020-05-27 17:50 UTC (permalink / raw)
To: Gdb-patches
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point
2020-05-25 18:56 ` Windows testsuite failures Hannes Domani
` (5 preceding siblings ...)
2020-05-25 18:56 ` [PATCH 6/7] Move exit_status_set_internal_vars out of GLOBAL_CURDIR Hannes Domani
@ 2020-05-25 18:56 ` Hannes Domani
2020-05-27 12:07 ` Pedro Alves
2020-05-31 16:37 ` Pedro Alves
2020-05-28 18:15 ` Windows testsuite failures Christian Biesinger
7 siblings, 2 replies; 44+ messages in thread
From: Hannes Domani @ 2020-05-25 18:56 UTC (permalink / raw)
To: gdb-patches
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
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point
2020-05-25 18:56 ` [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point Hannes Domani
@ 2020-05-27 12:07 ` Pedro Alves
2020-05-27 14:48 ` Pedro Alves
2020-05-31 16:37 ` Pedro Alves
1 sibling, 1 reply; 44+ messages in thread
From: Pedro Alves @ 2020-05-27 12:07 UTC (permalink / raw)
To: Hannes Domani, gdb-patches
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
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point
2020-05-27 12:07 ` Pedro Alves
@ 2020-05-27 14:48 ` Pedro Alves
2020-05-27 15:39 ` Hannes Domani
0 siblings, 1 reply; 44+ messages in thread
From: Pedro Alves @ 2020-05-27 14:48 UTC (permalink / raw)
To: Hannes Domani, gdb-patches
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
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point
2020-05-27 14:48 ` Pedro Alves
@ 2020-05-27 15:39 ` Hannes Domani
2020-05-31 15:54 ` Pedro Alves
0 siblings, 1 reply; 44+ messages in thread
From: Hannes Domani @ 2020-05-27 15:39 UTC (permalink / raw)
To: Gdb-patches
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
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point
2020-05-27 15:39 ` Hannes Domani
@ 2020-05-31 15:54 ` Pedro Alves
0 siblings, 0 replies; 44+ messages in thread
From: Pedro Alves @ 2020-05-31 15:54 UTC (permalink / raw)
To: Hannes Domani, Gdb-patches
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point
2020-05-25 18:56 ` [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point Hannes Domani
2020-05-27 12:07 ` Pedro Alves
@ 2020-05-31 16:37 ` Pedro Alves
2020-06-07 12:56 ` Hannes Domani
2020-10-12 17:21 ` Tom Tromey
1 sibling, 2 replies; 44+ messages in thread
From: Pedro Alves @ 2020-05-31 16:37 UTC (permalink / raw)
To: Hannes Domani, gdb-patches
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
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point
2020-05-31 16:37 ` Pedro Alves
@ 2020-06-07 12:56 ` Hannes Domani
2020-07-08 17:43 ` Hannes Domani
2020-10-09 18:22 ` Pedro Alves
2020-10-12 17:21 ` Tom Tromey
1 sibling, 2 replies; 44+ messages in thread
From: Hannes Domani @ 2020-06-07 12:56 UTC (permalink / raw)
To: Gdb-patches; +Cc: Pedro Alves
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
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point
2020-06-07 12:56 ` Hannes Domani
@ 2020-07-08 17:43 ` Hannes Domani
2020-10-09 18:22 ` Pedro Alves
1 sibling, 0 replies; 44+ messages in thread
From: Hannes Domani @ 2020-07-08 17:43 UTC (permalink / raw)
To: Gdb-patches; +Cc: Pedro Alves
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
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point
2020-06-07 12:56 ` Hannes Domani
2020-07-08 17:43 ` Hannes Domani
@ 2020-10-09 18:22 ` Pedro Alves
2020-10-09 18:51 ` Hannes Domani
1 sibling, 1 reply; 44+ messages in thread
From: Pedro Alves @ 2020-10-09 18:22 UTC (permalink / raw)
To: Hannes Domani, Gdb-patches
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
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point
2020-10-09 18:22 ` Pedro Alves
@ 2020-10-09 18:51 ` Hannes Domani
2020-10-12 11:13 ` Pedro Alves
0 siblings, 1 reply; 44+ messages in thread
From: Hannes Domani @ 2020-10-09 18:51 UTC (permalink / raw)
To: Gdb-patches, Pedro Alves
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
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point
2020-10-09 18:51 ` Hannes Domani
@ 2020-10-12 11:13 ` Pedro Alves
0 siblings, 0 replies; 44+ messages in thread
From: Pedro Alves @ 2020-10-12 11:13 UTC (permalink / raw)
To: Hannes Domani, Gdb-patches
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.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point
2020-05-31 16:37 ` Pedro Alves
2020-06-07 12:56 ` Hannes Domani
@ 2020-10-12 17:21 ` Tom Tromey
2020-10-12 17:22 ` Tom Tromey
1 sibling, 1 reply; 44+ messages in thread
From: Tom Tromey @ 2020-10-12 17:21 UTC (permalink / raw)
To: Pedro Alves via Gdb-patches; +Cc: Hannes Domani, Pedro Alves
>>>>> "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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point
2020-10-12 17:21 ` Tom Tromey
@ 2020-10-12 17:22 ` Tom Tromey
0 siblings, 0 replies; 44+ messages in thread
From: Tom Tromey @ 2020-10-12 17:22 UTC (permalink / raw)
To: Tom Tromey; +Cc: Pedro Alves via Gdb-patches, Pedro Alves
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
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: Windows testsuite failures
2020-05-25 18:56 ` Windows testsuite failures Hannes Domani
` (6 preceding siblings ...)
2020-05-25 18:56 ` [PATCH 7/7] Reset Windows hardware breakpoints on executable's entry point Hannes Domani
@ 2020-05-28 18:15 ` Christian Biesinger
2020-05-28 18:37 ` Hannes Domani
7 siblings, 1 reply; 44+ messages in thread
From: Christian Biesinger @ 2020-05-28 18:15 UTC (permalink / raw)
To: Hannes Domani; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: Windows testsuite failures
2020-05-28 18:15 ` Windows testsuite failures Christian Biesinger
@ 2020-05-28 18:37 ` Hannes Domani
0 siblings, 0 replies; 44+ messages in thread
From: Hannes Domani @ 2020-05-28 18:37 UTC (permalink / raw)
To: Gdb-patches
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
^ permalink raw reply [flat|nested] 44+ messages in thread