[patch] XFAIL gdb.cp/mb-inline.exp conditionaly

[patch] XFAIL gdb.cp/mb-inline.exp conditionaly

[Yao Qi]

[-- Attachment #1: Type: text/plain, Size: 1087 bytes --]

In mb-inline.exp, breakpoint 1.2 is disabled, re-run program, and make
sure breakpoint 1.2 is still disabled and program will not hit it.  The
pre-condition is new inferior created by re-run is loaded at the exactly
same address as previous one.  However, it is not true on some systems,
such as uclinux.

On uclinux, new inferior is created on the different address, so status
of breakpoint location (enabled or disabled) will not be kept during
breakpoint updates.  breakpoint 1.2 become enabled, instead of disabled.
 This will make this FAIL,

  FAIL: gdb.cp/mb-inline.exp: continue with disabled breakpoint 1.2

This patch is to address this issue.  After this patch, in my uclinux
port, the test result is like this,

# of expected passes            9
# of expected failures          1

but, some times, we can get 10 PASSes, because the new inferior may be
created/loaded on the same address as previous one.

Note that I also considered to use setup_xfail for uclinux target, but
we may get a XPASS, so I didn't write patch in that way.

OK for mainline?

-- 
Yao (齐尧)

[-- Attachment #2: uclinux_mb_inline.patch --]
[-- Type: text/x-patch, Size: 1651 bytes --]

2011-06-15  Yao Qi  <yao@codesourcery.com>

	gdb/testsuite/
	* gdb.cp/mb-inline.exp: Set breakpoint on function marker.
	XFAIL for uclinux.
	* gdb.cp/mb-inline1.cc (marker): New.

diff --git a/gdb/testsuite/gdb.cp/mb-inline.exp b/gdb/testsuite/gdb.cp/mb-inline.exp
index 86cb5ba..62708b0 100644
--- a/gdb/testsuite/gdb.cp/mb-inline.exp
+++ b/gdb/testsuite/gdb.cp/mb-inline.exp
@@ -101,7 +101,24 @@ gdb_expect {
     }
 }
 
-gdb_continue_to_end "disabled breakpoint 1.2"
+
+gdb_test "break marker" \
+    "Breakpoint.*at.* file .*, line.*" \
+    "set breakpoint on marker"
+
+gdb_test_multiple "continue" "continue with disabled breakpoint 1.2" {
+    -re "Breakpoint \[0-9\]+,.*marker.*$gdb_prompt $" {
+       pass "continue with disabled breakpoint 1.2"
+    }
+    -re "Breakpoint \[0-9\]+,.*foo \\(i=1\\).*$gdb_prompt $" {
+       # When inferior is restarted, breakpoint locations will be updated.
+       # On uclinux, it is not guaranteed that new inferior is located the
+       # same address as previous one, so status/state of breakpoint location
+       # will loose.
+       setup_xfail "*-*-uclinux*"
+       fail "continue with disabled breakpoint 1.2"
+    }
+}
 
 # Make sure we can set a breakpoint on a source statement that spans
 # multiple lines.
diff --git a/gdb/testsuite/gdb.cp/mb-inline1.cc b/gdb/testsuite/gdb.cp/mb-inline1.cc
index 3259002..86dc697 100644
--- a/gdb/testsuite/gdb.cp/mb-inline1.cc
+++ b/gdb/testsuite/gdb.cp/mb-inline1.cc
@@ -26,10 +26,15 @@ afn ()
   return foo (0) + multi_line_foo (0);
 }
 
+void
+marker ()
+{}
+
 int
 main ()
 {
   int a = afn ();
   int b = bfn ();
+  marker ();
   return a * b;
 }

Re: [patch] XFAIL gdb.cp/mb-inline.exp conditionaly

[Pedro Alves]

On Wednesday 15 June 2011 15:17:19, Yao Qi wrote:
> In mb-inline.exp, breakpoint 1.2 is disabled, re-run program, and make
> sure breakpoint 1.2 is still disabled and program will not hit it.  The
> pre-condition is new inferior created by re-run is loaded at the exactly
> same address as previous one.  However, it is not true on some systems,
> such as uclinux.
> 
> On uclinux, new inferior is created on the different address, so status
> of breakpoint location (enabled or disabled) will not be kept during
> breakpoint updates.  breakpoint 1.2 become enabled, instead of disabled.
>  This will make this FAIL,
> 
>   FAIL: gdb.cp/mb-inline.exp: continue with disabled breakpoint 1.2
> 

Why not do "info breakpoints", and parse the output, checking if
the breakpoint is really disabled?  If it is not, no point issuing
the continue.

The breakpoint was set by line number, and we're reloading
the session the same both times.  Why does breakpoint 1.2 become
enabled (and I'm guessing that breakpoint 1.1 becomes disabled)?

> This patch is to address this issue.  After this patch, in my uclinux
> port, the test result is like this,
> 
> # of expected passes            9
> # of expected failures          1
> 
> but, some times, we can get 10 PASSes, because the new inferior may be
> created/loaded on the same address as previous one.
> 
> Note that I also considered to use setup_xfail for uclinux target, but
> we may get a XPASS, so I didn't write patch in that way.
> 
> OK for mainline?
> 
> 

-- 
Pedro Alves

Re: [patch] XFAIL gdb.cp/mb-inline.exp conditionaly

[Yao Qi]

[-- Attachment #1: Type: text/plain, Size: 1478 bytes --]

On 06/22/2011 11:38 PM, Pedro Alves wrote:
> Why not do "info breakpoints", and parse the output, checking if
> the breakpoint is really disabled?  If it is not, no point issuing
> the continue.

Oh, yes.  New patch is revised in this way.

> The breakpoint was set by line number, and we're reloading
> the session the same both times.  Why does breakpoint 1.2 become
> enabled (and I'm guessing that breakpoint 1.1 becomes disabled)?
> 

No, in my case, both 1.1 and 1.2 becomes enabled.
info break^M
Num     Type           Disp Enb Address    What^M
1       breakpoint     keep y   <MULTIPLE> ^M
        breakpoint already hit 3 times^M
1.1                         y     0xe78c90a8 in foo(int) at
gdb/testsuite/gdb.cp/mb-inline.h:26^M
1.2                         y     0xe78c91a8 in foo(int) at
gdb/testsuite/gdb.cp/mb-inline.h:26^M

During breakpoint updates (when inferior is re-created), GDB will
iterate list of breakpoint locations, to look for breakpoint locations
for a certain address in new inferior, and assign found breakpoint
locations to that breakpoint.  The state of breakpoint location
(enabled/disabled) is kept, and this test will pass.

If gdb is unable to find any breakpoint location for a given address,
gdb will create new breakpoint locations, and remove old breakpoint
location.  Then, the state of breakpoint location of previous inferior
is lost, and new created breakpoint location is enabled in default.

OK for mainline?

-- 
Yao (齐尧)

[-- Attachment #2: mb-inline-v2.patch --]
[-- Type: text/x-patch, Size: 1053 bytes --]

	gdb/testsuite/
	* gdb.cp/mb-inline.exp: Parse the output of `info break' to check breakpoint
	1.2 is disabled.  XFAIL for uclinux.

diff --git a/gdb/testsuite/gdb.cp/mb-inline.exp b/gdb/testsuite/gdb.cp/mb-inline.exp
index 86cb5ba..81f7460 100644
--- a/gdb/testsuite/gdb.cp/mb-inline.exp
+++ b/gdb/testsuite/gdb.cp/mb-inline.exp
@@ -101,7 +101,19 @@ gdb_expect {
     }
 }
 
-gdb_continue_to_end "disabled breakpoint 1.2"
+gdb_test_multiple "info break" "disabled breakpoint 1.2" {
+    -re "1\.2.* n .* at .*$hdrfile:$bp_location.*$gdb_prompt $" {
+	pass "disabled breakpoint 1.2"
+    }
+    -re "1\.2.* y .* at .*$hdrfile:$bp_location.*$gdb_prompt $" {
+	# When inferior is restarted, breakpoint locations will be updated.
+	# On uclinux, it is not guaranteed that new inferior is located the
+	# same address as previous one, so status/state of breakpoint location
+	# will loose.
+	setup_xfail "*-*-uclinux*"
+	fail "disabled breakpoint 1.2"
+    }    
+}
 
 # Make sure we can set a breakpoint on a source statement that spans
 # multiple lines.

Re: [patch] XFAIL gdb.cp/mb-inline.exp conditionaly

[Pedro Alves]

On Thursday 23 June 2011 10:21:21, Yao Qi wrote:
> On 06/22/2011 11:38 PM, Pedro Alves wrote:

> > The breakpoint was set by line number, and we're reloading
> > the session the same both times.  Why does breakpoint 1.2 become
> > enabled (and I'm guessing that breakpoint 1.1 becomes disabled)?
> > 
> 
> No, in my case, both 1.1 and 1.2 becomes enabled.
> info break^M
> Num     Type           Disp Enb Address    What^M
> 1       breakpoint     keep y   <MULTIPLE> ^M
>         breakpoint already hit 3 times^M
> 1.1                         y     0xe78c90a8 in foo(int) at
> gdb/testsuite/gdb.cp/mb-inline.h:26^M
> 1.2                         y     0xe78c91a8 in foo(int) at
> gdb/testsuite/gdb.cp/mb-inline.h:26^M
> 
> During breakpoint updates (when inferior is re-created), GDB will
> iterate list of breakpoint locations, to look for breakpoint locations
> for a certain address in new inferior, and assign found breakpoint
> locations to that breakpoint.  The state of breakpoint location
> (enabled/disabled) is kept, and this test will pass.
> 
> If gdb is unable to find any breakpoint location for a given address,
> gdb will create new breakpoint locations, and remove old breakpoint
> location.  Then, the state of breakpoint location of previous inferior
> is lost, and new created breakpoint location is enabled in default.

Okay, that happens when the locations have ambiguous names, at the
end of update_breakpoint_locations.

You're just re-running the same binary, and the breakpoints are even
all set in the same objfile.  Nothing changed from the user's
perpective.  It's reasonable to expect gdb manages to not lose track
of the disable state.  I think we can and should improve the heuristic
to handle this scenario.  

Instead of comparing
absolute addresses, normalize them before comparing.  E.g.,

Before             After
0x400010           0x800010
0x401000           0x801000
0x410000           0x810000
0x400100           0x800100

For each list, find some common base and subtract it from
each entry, and _then_ compare the locations:

Before             After
0x000010           0x000010
0x001000           0x001000
0x010000           0x010000
0x000100           0x000100

The common base might be the lowest address in each list,
or something else, like the objfile's lowest address, or
some such.  If we had some sort of unique symbol hash id,
we could use that instead, and it'd be more reliable, me
thinks.

-- 
Pedro Alves

Re: [patch] XFAIL gdb.cp/mb-inline.exp conditionaly

[Yao Qi]

On 06/23/2011 06:28 PM, Pedro Alves wrote:
> Okay, that happens when the locations have ambiguous names, at the
> end of update_breakpoint_locations.
> 
> You're just re-running the same binary, and the breakpoints are even
> all set in the same objfile.  Nothing changed from the user's
> perpective.  It's reasonable to expect gdb manages to not lose track
> of the disable state.  I think we can and should improve the heuristic
> to handle this scenario.  
> 

Yes, this heuristics can be improved.  I have had a local hack similar
to what you suggest below, and this hack works for me.

> Instead of comparing
> absolute addresses, normalize them before comparing.  E.g.,
> 
> Before             After
> 0x400010           0x800010
> 0x401000           0x801000
> 0x410000           0x810000
> 0x400100           0x800100
> 
> For each list, find some common base and subtract it from
> each entry, and _then_ compare the locations:
> 
> Before             After
> 0x000010           0x000010
> 0x001000           0x001000
> 0x010000           0x010000
> 0x000100           0x000100
> 
> The common base might be the lowest address in each list,
> or something else, like the objfile's lowest address, or
> some such.  If we had some sort of unique symbol hash id,
> we could use that instead, and it'd be more reliable, me
> thinks.
> 

The key point of this approach is about identifying common base.

My hack is similar to yours.  In my hack, the offset of objfile
relocation O1 is cached.  When new inferior is created again and new
object of objfile relocation offset O2 is got, we can compute the offset
A between offset of previous inferior's relocation offset (O1) and
current inferior's (O2), and then we can "relocate" breakpoint locations
with offset A.  Again, it is still a hack, and some more work may be
needed here.

Anyway, I'll think of this problem.

-- 
Yao (齐尧)

Re: [patch] XFAIL gdb.cp/mb-inline.exp conditionaly

[Pedro Alves]

On Thursday 23 June 2011 14:40:07, Yao Qi wrote:
> On 06/23/2011 06:28 PM, Pedro Alves wrote:
> > Okay, that happens when the locations have ambiguous names, at the
> > end of update_breakpoint_locations.
> > 
> > You're just re-running the same binary, and the breakpoints are even
> > all set in the same objfile.  Nothing changed from the user's
> > perpective.  It's reasonable to expect gdb manages to not lose track
> > of the disable state.  I think we can and should improve the heuristic
> > to handle this scenario.  
> > 
> 
> Yes, this heuristics can be improved.  I have had a local hack similar
> to what you suggest below, and this hack works for me.
> 
> > Instead of comparing
> > absolute addresses, normalize them before comparing.  E.g.,
> > 
> > Before             After
> > 0x400010           0x800010
> > 0x401000           0x801000
> > 0x410000           0x810000
> > 0x400100           0x800100
> > 
> > For each list, find some common base and subtract it from
> > each entry, and _then_ compare the locations:
> > 
> > Before             After
> > 0x000010           0x000010
> > 0x001000           0x001000
> > 0x010000           0x010000
> > 0x000100           0x000100
> > 
> > The common base might be the lowest address in each list,
> > or something else, like the objfile's lowest address, or
> > some such.  If we had some sort of unique symbol hash id,
> > we could use that instead, and it'd be more reliable, me
> > thinks.
> > 
> 
> The key point of this approach is about identifying common base.
> 
> My hack is similar to yours.  In my hack, the offset of objfile
> relocation O1 is cached.  When new inferior is created again and new
> object of objfile relocation offset O2 is got, we can compute the offset
> A between offset of previous inferior's relocation offset (O1) and
> current inferior's (O2), and then we can "relocate" breakpoint locations
> with offset A.  Again, it is still a hack, and some more work may be
> needed here.

Yes, that'd be a good approach.  But you don't have a single
offset though.  You have potentialy one relocation offset per segment, or
one per section.  And its not clear locations set at addresses (b *0xf00)
rather than though symbols should be relocated.  At
<http://sourceware.org/ml/gdb-patches/2009-06/msg00668.html> I outline
"cooked" vs "raw" addresses, that I think would be an even better long
term goal, which is sort of a superset of all this in that it treats
addresses as base+offset.  Hmmm, going back to the current mechanism, I
wonder if we could use offset into section as common base (and also
compare if the sections are the same).

> 
> Anyway, I'll think of this problem.
> 
> 

-- 
Pedro Alves

Re: [patch] XFAIL gdb.cp/mb-inline.exp conditionaly

[Pedro Alves]

On Thursday 23 June 2011 10:21:21, Yao Qi wrote:
>         gdb/testsuite/
>         * gdb.cp/mb-inline.exp: Parse the output of `info break' to check breakpoint
>         1.2 is disabled.  XFAIL for uclinux.

If you don't want to fix gdb for this now, then please open a PR, and
make it a kfail instead.  Okay with that change.

-- 
Pedro Alves

Re: [patch] XFAIL gdb.cp/mb-inline.exp conditionaly

[Yao Qi]

[-- Attachment #1: Type: text/plain, Size: 572 bytes --]

On 06/23/2011 06:37 PM, Pedro Alves wrote:
> On Thursday 23 June 2011 10:21:21, Yao Qi wrote:
>>         gdb/testsuite/
>>         * gdb.cp/mb-inline.exp: Parse the output of `info break' to check breakpoint
>>         1.2 is disabled.  XFAIL for uclinux.
> 
> If you don't want to fix gdb for this now, then please open a PR, and
> make it a kfail instead.  Okay with that change.
> 

A PR is opened http://sourceware.org/bugzilla/show_bug.cgi?id=12924.

Patch attached is what I committed.

http://sourceware.org/ml/gdb-cvs/2011-06/msg00139.html

-- 
Yao (齐尧)

[-- Attachment #2: mb-inline-v2.patch --]
[-- Type: text/x-patch, Size: 1105 bytes --]

	gdb/testsuite/
	* gdb.cp/mb-inline.exp: Parse the output of `info break' to check breakpoint
	1.2 is disabled.  KFAIL for uclinux.

diff --git a/gdb/testsuite/gdb.cp/mb-inline.exp b/gdb/testsuite/gdb.cp/mb-inline.exp
index 86cb5ba..81f7460 100644
--- a/gdb/testsuite/gdb.cp/mb-inline.exp
+++ b/gdb/testsuite/gdb.cp/mb-inline.exp
@@ -101,7 +101,19 @@ gdb_expect {
     }
 }
 
-gdb_continue_to_end "disabled breakpoint 1.2"
+gdb_test_multiple "info break" "disabled breakpoint 1.2" {
+    -re "1\.2.* n .* at .*$hdrfile:$bp_location.*$gdb_prompt $" {
+	pass "disabled breakpoint 1.2"
+    }
+    -re "1\.2.* y .* at .*$hdrfile:$bp_location.*$gdb_prompt $" {
+	# When inferior is restarted, breakpoint locations will be updated.
+	# On uclinux, it is not guaranteed that new inferior is located the
+	# same address as previous one, so status/state of breakpoint location
+	# will loose.  The heuristic of GDB should be improved.
+	setup_kfail gdb/12924 "*-*-uclinux*"
+	fail "disabled breakpoint 1.2"
+    }    
+}
 
 # Make sure we can set a breakpoint on a source statement that spans
 # multiple lines.