public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 0/2] Add testcase for libc memory operations
@ 2024-04-20 21:33 Thiago Jung Bauermann
  2024-04-20 21:33 ` [PATCH 1/2] gdb/testsuite: Add libc_has_debug_info require helper Thiago Jung Bauermann
  2024-04-20 21:33 ` [PATCH 2/2] gdb/testsuite: Add gdb.base/memops-watchpoint.exp Thiago Jung Bauermann
  0 siblings, 2 replies; 11+ messages in thread
From: Thiago Jung Bauermann @ 2024-04-20 21:33 UTC (permalink / raw)
  To: gdb-patches

Hello,

In PR testsuite/31484, Tom de Vries suggests adding testcases that
excercise memset/memcpy/memmove and set watchpoints.

Patch 2 adds such testcase.  See its description for a bit more on the
motivation.  Patch 1 factors out code to check whether libc debug info is
available, which is needed for the new testcase.


Thiago Jung Bauermann (2):
  gdb/testsuite: Add libc_has_debug_info require helper
  gdb/testsuite: Add gdb.base/memops-watchpoint.exp

 gdb/testsuite/gdb.base/memops-watchpoint.c   | 40 ++++++++++
 gdb/testsuite/gdb.base/memops-watchpoint.exp | 83 ++++++++++++++++++++
 gdb/testsuite/gdb.base/relativedebug.exp     | 13 +--
 gdb/testsuite/lib/gdb.exp                    | 54 +++++++++++++
 4 files changed, 178 insertions(+), 12 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/memops-watchpoint.c
 create mode 100644 gdb/testsuite/gdb.base/memops-watchpoint.exp


base-commit: 20eee7540b9f2615f7661393756fec0bb62a1495

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/2] gdb/testsuite: Add libc_has_debug_info require helper
  2024-04-20 21:33 [PATCH 0/2] Add testcase for libc memory operations Thiago Jung Bauermann
@ 2024-04-20 21:33 ` Thiago Jung Bauermann
  2024-04-21 20:44   ` Kevin Buettner
  2024-04-20 21:33 ` [PATCH 2/2] gdb/testsuite: Add gdb.base/memops-watchpoint.exp Thiago Jung Bauermann
  1 sibling, 1 reply; 11+ messages in thread
From: Thiago Jung Bauermann @ 2024-04-20 21:33 UTC (permalink / raw)
  To: gdb-patches

Factor the test for libc debug info out of gdb.base/relativedebug.exp to
a new procedure.

Also, change the "info sharedlibrary" test to explicitly detect when
libc has debug info.
---
 gdb/testsuite/gdb.base/relativedebug.exp | 13 +-----
 gdb/testsuite/lib/gdb.exp                | 54 ++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 12 deletions(-)

diff --git a/gdb/testsuite/gdb.base/relativedebug.exp b/gdb/testsuite/gdb.base/relativedebug.exp
index bf8d76887122..f882a5cf1676 100644
--- a/gdb/testsuite/gdb.base/relativedebug.exp
+++ b/gdb/testsuite/gdb.base/relativedebug.exp
@@ -13,7 +13,7 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-require {!target_info exists gdb,nosignals}
+require {!target_info exists gdb,nosignals} libc_has_debug_info
 
 standard_testfile .c
 
@@ -28,17 +28,6 @@ clean_restart ${binfile}
 
 runto_main
 
-set test "info sharedlibrary"
-gdb_test_multiple $test $test {
-    -re ".*\(\\*\)\[^\r\n\]*/libc\.so.*$gdb_prompt $" {
-	# Skip the test below if libc doesn't have debug info.
-	unsupported "libc doesn't have debug info"
-	return -1
-    }
-    -re ".*$gdb_prompt $" {
-    }
-}
-
 # pause () -> SIGALRM -> handler () -> abort ()
 gdb_test "continue" "Program received signal SIGABRT.*"
 
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index ddee928d5104..9af73bef8f09 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -3699,6 +3699,60 @@ proc support_displaced_stepping {} {
     return 0
 }
 
+# Return 1 if GDB can find the libc debug info, or 0 and a reason string if it
+# can't.  This procedure is meant to be called by the require procedure.
+gdb_caching_proc libc_has_debug_info {} {
+    global srcdir subdir gdb_prompt inferior_exited_re
+
+    set me "libc_has_debug_info"
+
+    # Compile a test program.
+    set src {
+	int main (void) {
+	    printf ("Hello, world!\n");
+	    return 0;
+	}
+    }
+    if {![gdb_simple_compile $me $src executable {debug}]} {
+	return [list 0 "failed to compile test program"]
+    }
+
+    # No error message, compilation succeeded so now run it via gdb.
+
+    gdb_exit
+    gdb_start
+    gdb_reinitialize_dir $srcdir/$subdir
+    gdb_load "$obj"
+    runto_main
+    set test "info sharedlibrary libc.so"
+    gdb_test_multiple $test $test {
+	-re ".*\(\\*\)\[^\r\n\]*/libc\.so.*$gdb_prompt $" {
+	    # Matched the "(*)" in the "Syms Read" columns which means:
+	    # "(*): Shared library is missing debugging information."
+	    verbose -log "$me: libc doesn't have debug info"
+	    set libc_has_debug_info 0
+	    set message "libc doesn't have debug info"
+	}
+	-re ".*Yes\[ \t\]+\[^\r\n\]*/libc\.so.*$gdb_prompt $" {
+	    verbose -log "$me: libc has debug info"
+	    set libc_has_debug_info 1
+	}
+	default {
+	    set libc_has_debug_info 0
+	    set message "libc not found in the inferior"
+	}
+    }
+    gdb_exit
+    remote_file build delete $obj
+
+    verbose "$me: returning $libc_has_debug_info" 2
+    if { $libc_has_debug_info } {
+	return $libc_has_debug_info
+    } else {
+	return [list $libc_has_debug_info $message]
+    }
+}
+
 # Run a test on the target to see if it supports vmx hardware.  Return 1 if so, 
 # 0 if it does not.  Based on 'check_vmx_hw_available' from the GCC testsuite.
 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 2/2] gdb/testsuite: Add gdb.base/memops-watchpoint.exp
  2024-04-20 21:33 [PATCH 0/2] Add testcase for libc memory operations Thiago Jung Bauermann
  2024-04-20 21:33 ` [PATCH 1/2] gdb/testsuite: Add libc_has_debug_info require helper Thiago Jung Bauermann
@ 2024-04-20 21:33 ` Thiago Jung Bauermann
  2024-04-21 21:20   ` Kevin Buettner
  1 sibling, 1 reply; 11+ messages in thread
From: Thiago Jung Bauermann @ 2024-04-20 21:33 UTC (permalink / raw)
  To: gdb-patches

Test behaviour of watchpoints triggered by libc's memset/memcpy/memmove.
These functions are frequently optimized with specialized instructions
that favor larger memory access operations, so make sure GDB behaves
correctly in their presence.

There's a separate watched variable for each function so that the
testcase can test whether GDB correctly identified the watchpoint that
triggered.

Also, the watchpoint is 31 bytes away from the beginning of the buffer
being modified, so that large memory accesses (if present) as well as
watching an unaligned memory address are exercised.

PR testsuite/31484
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31484
---
 gdb/testsuite/gdb.base/memops-watchpoint.c   | 40 ++++++++++
 gdb/testsuite/gdb.base/memops-watchpoint.exp | 83 ++++++++++++++++++++
 2 files changed, 123 insertions(+)
 create mode 100644 gdb/testsuite/gdb.base/memops-watchpoint.c
 create mode 100644 gdb/testsuite/gdb.base/memops-watchpoint.exp

NB: Please let me know if you think there are more interesting tests
regarding watchpoint and memory accesses that can be done.  I tried to
make it cover the interesting scenarios but the testcase is small, so
maybe I'm not very creative.

diff --git a/gdb/testsuite/gdb.base/memops-watchpoint.c b/gdb/testsuite/gdb.base/memops-watchpoint.c
new file mode 100644
index 000000000000..13e923faa1e9
--- /dev/null
+++ b/gdb/testsuite/gdb.base/memops-watchpoint.c
@@ -0,0 +1,40 @@
+/* This test program is part of GDB, the GNU debugger.
+
+   Copyright 2024 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <stdio.h>
+#include <string.h>
+
+int
+main (void)
+{
+  char s[40] = "This is a relatively long string...";
+  char a[40] = "String to be overwritten with zeroes";
+  char b[40] = "Another string to be memcopied...";
+  char c[40] = "Another string to be memmoved...";
+
+  /* Break here.  */
+  memset (a, 0, sizeof (a));
+
+  memcpy (b, s, sizeof (b));
+
+  memmove (c, s, sizeof (c));
+
+  printf ("b = '%s'\n", b);
+  printf ("c = '%s'\n", c);
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/memops-watchpoint.exp b/gdb/testsuite/gdb.base/memops-watchpoint.exp
new file mode 100644
index 000000000000..6fc84eb469c4
--- /dev/null
+++ b/gdb/testsuite/gdb.base/memops-watchpoint.exp
@@ -0,0 +1,83 @@
+# Copyright 2024 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test a binary that uses standard libc memory operation functions.  They are
+# frequently optimized with specialized instructions, so make sure GDB behaves
+# correctly in their presence.
+
+# It's not possible to check in which libc function the watchpoint triggers
+# without its debug info.
+require libc_has_debug_info
+
+standard_testfile
+
+set options "-fno-builtin-memset -fno-builtin-memcpy -fno-builtin-memmove"
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
+	  [list debug additional_flags=$options]] } {
+    return -1
+}
+
+set linespec ${srcfile}:[gdb_get_line_number "Break here"]
+
+if ![runto ${linespec}] {
+    return
+}
+
+gdb_test "watch -location a\[31\]" \
+    "(Hardware w|W)atchpoint ${decimal}: -location a\\\[31\\\]" \
+    "set watch on a"
+gdb_test "watch -location b\[31\]" \
+    "(Hardware w|W)atchpoint ${decimal}: -location b\\\[31\\\]" \
+    "set watchpoint on b"
+gdb_test "watch -location c\[31\]" \
+    "(Hardware w|W)atchpoint ${decimal}: -location c\\\[31\\\]" \
+    "set watchpoint on c"
+
+gdb_test "continue" \
+    [multi_line \
+	 "Continuing\\." \
+	 "" \
+	 "(Hardware w|W)atchpoint ${decimal}: -location a\\\[31\\\]" \
+	 "" \
+	 "Old value = 101 'e'" \
+	 "New value = 0 '\\\\000'" \
+	 ".*memset.* \\(\\) at .*:$decimal" \
+	 ".*"] \
+    "continue until memset watchpoint hits"
+
+gdb_test "continue" \
+    [multi_line \
+	 "Continuing\\." \
+	 "" \
+	 "(Hardware w|W)atchpoint ${decimal}: -location b\\\[31\\\]" \
+	 "" \
+	 "Old value = 46 '\\.'" \
+	 "New value = 103 'g'" \
+	 ".*memcpy.* \\(\\) at .*:$decimal" \
+	".*"] \
+    "continue until memcpy watchpoint hits"
+
+# Note: Some architectures use memcpy for memmove.
+gdb_test "continue" \
+    [multi_line \
+	 "Continuing\\." \
+	 "" \
+	 "(Hardware w|W)atchpoint ${decimal}: -location c\\\[31\\\]" \
+	 "" \
+	 "Old value = 46 '\\.'" \
+	 "New value = 103 'g'" \
+	 ".*(memmove|memcpy).* \\(\\) at .*:$decimal" \
+	".*"] \
+    "continue until memmove watchpoint hits"

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] gdb/testsuite: Add libc_has_debug_info require helper
  2024-04-20 21:33 ` [PATCH 1/2] gdb/testsuite: Add libc_has_debug_info require helper Thiago Jung Bauermann
@ 2024-04-21 20:44   ` Kevin Buettner
  2024-04-21 21:02     ` Kevin Buettner
  0 siblings, 1 reply; 11+ messages in thread
From: Kevin Buettner @ 2024-04-21 20:44 UTC (permalink / raw)
  To: Thiago Jung Bauermann; +Cc: gdb-patches

On Sat, 20 Apr 2024 18:33:06 -0300
Thiago Jung Bauermann <thiago.bauermann@linaro.org> wrote:

> Factor the test for libc debug info out of gdb.base/relativedebug.exp to
> a new procedure.
> 
> Also, change the "info sharedlibrary" test to explicitly detect when
> libc has debug info.

LGTM.

Approved-by: Kevin Buettner <kevinb@redhat.com>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] gdb/testsuite: Add libc_has_debug_info require helper
  2024-04-21 20:44   ` Kevin Buettner
@ 2024-04-21 21:02     ` Kevin Buettner
  2024-04-22  0:05       ` Thiago Jung Bauermann
  0 siblings, 1 reply; 11+ messages in thread
From: Kevin Buettner @ 2024-04-21 21:02 UTC (permalink / raw)
  To: Thiago Jung Bauermann; +Cc: gdb-patches

On Sun, 21 Apr 2024 13:44:06 -0700
Kevin Buettner <kevinb@redhat.com> wrote:

> On Sat, 20 Apr 2024 18:33:06 -0300
> Thiago Jung Bauermann <thiago.bauermann@linaro.org> wrote:
> 
> > Factor the test for libc debug info out of gdb.base/relativedebug.exp to
> > a new procedure.
> > 
> > Also, change the "info sharedlibrary" test to explicitly detect when
> > libc has debug info.  
> 
> LGTM.
> 
> Approved-by: Kevin Buettner <kevinb@redhat.com>

Just found a problem...  While attempting to test part 2 using clang:

make check RUNTESTFLAGS="CC_FOR_TARGET=clang" TESTS="gdb.base/memops-watchpoint.exp"

Then, in the log file:

get_compiler_info: clang-17-0-6
Executing on host: clang  /mesquite2/sourceware-git/f39-review/bld/gdb/testsuite/temp/164548/libc_has_debug_info.c   -fdiagnostics-color=never -Wno-unknown-warning-option -w -g -g  -lm  -o /mesquite2/sourceware-git/f39-review/bld/gdb/testsuite/temp/164548/libc_has_debug_info.x    (timeout = 300)
builtin_spawn -ignore SIGHUP clang /mesquite2/sourceware-git/f39-review/bld/gdb/testsuite/temp/164548/libc_has_debug_info.c -fdiagnostics-color=never -Wno-unknown-warning-option -w -g -g -lm -o /mesquite2/sourceware-git/f39-review/bld/gdb/testsuite/temp/164548/libc_has_debug_info.x
/mesquite2/sourceware-git/f39-review/bld/gdb/testsuite/temp/164548/libc_has_debug_info.c:3:6: error: call to undeclared library function 'printf' with type 'int (const char *, ...)'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    3 |             printf ("Hello, world!\n");
      |             ^
/mesquite2/sourceware-git/f39-review/bld/gdb/testsuite/temp/164548/libc_has_debug_info.c:3:6: note: include the header <stdio.h> or explicitly provide a declaration for 'printf'
1 error generated.
compiler exited with status 1
UNSUPPORTED: gdb.base/memops-watchpoint.exp: require failed: libc_has_debug_info (failed to compile test program)

My testing shows that things will work if you add the suggested #include to
the "set src" text.

Kevin


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] gdb/testsuite: Add gdb.base/memops-watchpoint.exp
  2024-04-20 21:33 ` [PATCH 2/2] gdb/testsuite: Add gdb.base/memops-watchpoint.exp Thiago Jung Bauermann
@ 2024-04-21 21:20   ` Kevin Buettner
  2024-04-22  0:24     ` Thiago Jung Bauermann
  0 siblings, 1 reply; 11+ messages in thread
From: Kevin Buettner @ 2024-04-21 21:20 UTC (permalink / raw)
  To: Thiago Jung Bauermann; +Cc: gdb-patches

On Sat, 20 Apr 2024 18:33:07 -0300
Thiago Jung Bauermann <thiago.bauermann@linaro.org> wrote:

> diff --git a/gdb/testsuite/gdb.base/memops-watchpoint.exp b/gdb/testsuite/gdb.base/memops-watchpoint.exp
> new file mode 100644
> index 000000000000..6fc84eb469c4
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/memops-watchpoint.exp
> @@ -0,0 +1,83 @@
> +# Copyright 2024 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Test a binary that uses standard libc memory operation functions.  They are
> +# frequently optimized with specialized instructions, so make sure GDB behaves
> +# correctly in their presence.
> +
> +# It's not possible to check in which libc function the watchpoint triggers
> +# without its debug info.
> +require libc_has_debug_info

I'm wondering about the need for this requirement.  When I comment it
out and run it on a machine without libc debuginfo, I do see 3 FAILs,
but it seems to me that those could be turned into PASSes by changing
the regular expressions for the "continue until..." tests.

E.g. for the first one, with libc debuginfo, I see:

continue
Continuing.

Hardware watchpoint 2: -location a[31]

Old value = 101 'e'
New value = 0 '\000'
__memset_avx2_unaligned () at ../sysdeps/x86_64/multiarch/memset-vec-unaligned-erms.S:146
146		VMOVU	%VMM(0), (%rdi)
(gdb) PASS: gdb.base/memops-watchpoint.exp: continue until memset watchpoint hits

But, without libc debuginfo, the watchpoint still works:

continue
Continuing.

Hardware watchpoint 2: -location a[31]

Old value = 101 'e'
New value = 0 '\000'
0x00007ffff7e3553a in __memset_avx2_unaligned () from /lib64/libc.so.6
(gdb) FAIL: gdb.base/memops-watchpoint.exp: continue until memset watchpoint hits

As stated earlier, this could be turned into a PASS by tweaking the RE.

In both cases, we know that it's in a "memset" function.  (The presence
of minimal symbols provides GDB with this information.)

Kevin


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] gdb/testsuite: Add libc_has_debug_info require helper
  2024-04-21 21:02     ` Kevin Buettner
@ 2024-04-22  0:05       ` Thiago Jung Bauermann
  0 siblings, 0 replies; 11+ messages in thread
From: Thiago Jung Bauermann @ 2024-04-22  0:05 UTC (permalink / raw)
  To: Kevin Buettner; +Cc: gdb-patches

Hello Kevin,

Thank you for your review! Sorry for sending v2 without addressing your
comments. I only saw your emails after I sent the series.

Kevin Buettner <kevinb@redhat.com> writes:

> On Sun, 21 Apr 2024 13:44:06 -0700
> Kevin Buettner <kevinb@redhat.com> wrote:
>
>> On Sat, 20 Apr 2024 18:33:06 -0300
>> Thiago Jung Bauermann <thiago.bauermann@linaro.org> wrote:
>>
>> > Factor the test for libc debug info out of gdb.base/relativedebug.exp to
>> > a new procedure.
>> >
>> > Also, change the "info sharedlibrary" test to explicitly detect when
>> > libc has debug info.
>>
>> LGTM.
>>
>> Approved-by: Kevin Buettner <kevinb@redhat.com>
>
> Just found a problem...  While attempting to test part 2 using clang:
>
> make check RUNTESTFLAGS="CC_FOR_TARGET=clang" TESTS="gdb.base/memops-watchpoint.exp"
>
> Then, in the log file:
>
> get_compiler_info: clang-17-0-6
> Executing on host: clang  /mesquite2/sourceware-git/f39-review/bld/gdb/testsuite/temp/164548/libc_has_debug_info.c   -fdiagnostics-color=never -Wno-unknown-warning-option -w -g -g  -lm  -o /mesquite2/sourceware-git/f39-review/bld/gdb/testsuite/temp/164548/libc_has_debug_info.x    (timeout = 300)
> builtin_spawn -ignore SIGHUP clang /mesquite2/sourceware-git/f39-review/bld/gdb/testsuite/temp/164548/libc_has_debug_info.c -fdiagnostics-color=never -Wno-unknown-warning-option -w -g -g -lm -o /mesquite2/sourceware-git/f39-review/bld/gdb/testsuite/temp/164548/libc_has_debug_info.x
> /mesquite2/sourceware-git/f39-review/bld/gdb/testsuite/temp/164548/libc_has_debug_info.c:3:6: error: call to undeclared library function 'printf' with type 'int (const char *, ...)'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>     3 |             printf ("Hello, world!\n");
>       |             ^
> /mesquite2/sourceware-git/f39-review/bld/gdb/testsuite/temp/164548/libc_has_debug_info.c:3:6: note: include the header <stdio.h> or explicitly provide a declaration for 'printf'
> 1 error generated.
> compiler exited with status 1
> UNSUPPORTED: gdb.base/memops-watchpoint.exp: require failed: libc_has_debug_info (failed to compile test program)
>
> My testing shows that things will work if you add the suggested #include to
> the "set src" text.

Ah, of course. GCC doesn't complain about it because of the -w option:

       -w  Inhibit all warning messages.

and in GCC this is a warning not an error.

I'll fix it in the next version.

--
Thiago

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] gdb/testsuite: Add gdb.base/memops-watchpoint.exp
  2024-04-21 21:20   ` Kevin Buettner
@ 2024-04-22  0:24     ` Thiago Jung Bauermann
  2024-04-22 18:40       ` Kevin Buettner
  2024-04-22 23:04       ` Thiago Jung Bauermann
  0 siblings, 2 replies; 11+ messages in thread
From: Thiago Jung Bauermann @ 2024-04-22  0:24 UTC (permalink / raw)
  To: Kevin Buettner; +Cc: gdb-patches


Thank you for your review!

Kevin Buettner <kevinb@redhat.com> writes:

> On Sat, 20 Apr 2024 18:33:07 -0300
> Thiago Jung Bauermann <thiago.bauermann@linaro.org> wrote:
>
>> diff --git a/gdb/testsuite/gdb.base/memops-watchpoint.exp
>> b/gdb/testsuite/gdb.base/memops-watchpoint.exp
>> new file mode 100644
>> index 000000000000..6fc84eb469c4
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/memops-watchpoint.exp
>> @@ -0,0 +1,83 @@
>> +# Copyright 2024 Free Software Foundation, Inc.
>> +
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 3 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +
>> +# Test a binary that uses standard libc memory operation functions.  They are
>> +# frequently optimized with specialized instructions, so make sure GDB behaves
>> +# correctly in their presence.
>> +
>> +# It's not possible to check in which libc function the watchpoint triggers
>> +# without its debug info.
>> +require libc_has_debug_info
>
> I'm wondering about the need for this requirement.  When I comment it
> out and run it on a machine without libc debuginfo, I do see 3 FAILs,
> but it seems to me that those could be turned into PASSes by changing
> the regular expressions for the "continue until..." tests.
>
> E.g. for the first one, with libc debuginfo, I see:
>
> continue
> Continuing.
>
> Hardware watchpoint 2: -location a[31]
>
> Old value = 101 'e'
> New value = 0 '\000'
> __memset_avx2_unaligned () at ../sysdeps/x86_64/multiarch/memset-vec-unaligned-erms.S:146
> 146		VMOVU	%VMM(0), (%rdi)
> (gdb) PASS: gdb.base/memops-watchpoint.exp: continue until memset watchpoint hits
>
> But, without libc debuginfo, the watchpoint still works:
>
> continue
> Continuing.
>
> Hardware watchpoint 2: -location a[31]
>
> Old value = 101 'e'
> New value = 0 '\000'
> 0x00007ffff7e3553a in __memset_avx2_unaligned () from /lib64/libc.so.6
> (gdb) FAIL: gdb.base/memops-watchpoint.exp: continue until memset watchpoint hits
>
> As stated earlier, this could be turned into a PASS by tweaking the RE.
>
> In both cases, we know that it's in a "memset" function.  (The presence
> of minimal symbols provides GDB with this information.)

I added the requirement because in my aarch64-linux system without libc6
debug info I get:

continue
Continuing.

Hardware watchpoint 2: -location a[28]

Old value = 104 'h'
New value = 0 '\000'
0x0000fffff7e90664 in ?? () from /lib/aarch64-linux-gnu/libc.so.6
(gdb) FAIL: gdb.base/memops-watchpoint.exp: continue until memset watchpoint hits

And I just tested removing libc6-dbg from my x86_64-linux laptop:

continue
Continuing.

Hardware watchpoint 2: -location a[28]

Old value = 104 'h'
New value = 0 '\000'
0x00007ffff7d8e05f in ?? () from /lib/x86_64-linux-gnu/libc.so.6
(gdb) FAIL: gdb.base/memops-watchpoint.exp: continue until memset watchpoint hits

So it depends on the system.

One alternative would be to not use the require statement and run the
test until the watchpoint hits, and have a case in gdb_test_multiple to
mark as UNRESOLVED if the function name is '??'.

--
Thiago

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] gdb/testsuite: Add gdb.base/memops-watchpoint.exp
  2024-04-22  0:24     ` Thiago Jung Bauermann
@ 2024-04-22 18:40       ` Kevin Buettner
  2024-04-23  1:20         ` Thiago Jung Bauermann
  2024-04-22 23:04       ` Thiago Jung Bauermann
  1 sibling, 1 reply; 11+ messages in thread
From: Kevin Buettner @ 2024-04-22 18:40 UTC (permalink / raw)
  To: Thiago Jung Bauermann; +Cc: gdb-patches

On Sun, 21 Apr 2024 21:24:42 -0300
Thiago Jung Bauermann <thiago.bauermann@linaro.org> wrote:

> >> +require libc_has_debug_info  
> >
> > I'm wondering about the need for this requirement.  When I comment it
> > out and run it on a machine without libc debuginfo, I do see 3 FAILs,
> > but it seems to me that those could be turned into PASSes by changing
> > the regular expressions for the "continue until..." tests.
[...]
> 
> I added the requirement because in my aarch64-linux system without libc6
> debug info I get:
> 
> continue
> Continuing.
> 
> Hardware watchpoint 2: -location a[28]
> 
> Old value = 104 'h'
> New value = 0 '\000'
> 0x0000fffff7e90664 in ?? () from /lib/aarch64-linux-gnu/libc.so.6
> (gdb) FAIL: gdb.base/memops-watchpoint.exp: continue until memset watchpoint hits
> 
> And I just tested removing libc6-dbg from my x86_64-linux laptop:
> 
> continue
> Continuing.
> 
> Hardware watchpoint 2: -location a[28]
> 
> Old value = 104 'h'
> New value = 0 '\000'
> 0x00007ffff7d8e05f in ?? () from /lib/x86_64-linux-gnu/libc.so.6
> (gdb) FAIL: gdb.base/memops-watchpoint.exp: continue until memset watchpoint hits
> 
> So it depends on the system.

What distro are you using?

> One alternative would be to not use the require statement and run the
> test until the watchpoint hits, and have a case in gdb_test_multiple to
> mark as UNRESOLVED if the function name is '??'.

I'm in favor of this approach.

If we stick with the require statement, I think that Fedora testing
will frequently show this new test as unsupported since installing
debuginfo is less common / important that it used to be.  (This is
due to debuginfod doing it for you.  But I think that debuginfod is
mostly disabled when running the GDB tests.)


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] gdb/testsuite: Add gdb.base/memops-watchpoint.exp
  2024-04-22  0:24     ` Thiago Jung Bauermann
  2024-04-22 18:40       ` Kevin Buettner
@ 2024-04-22 23:04       ` Thiago Jung Bauermann
  1 sibling, 0 replies; 11+ messages in thread
From: Thiago Jung Bauermann @ 2024-04-22 23:04 UTC (permalink / raw)
  To: Kevin Buettner; +Cc: gdb-patches

Thiago Jung Bauermann <thiago.bauermann@linaro.org> writes:

> Kevin Buettner <kevinb@redhat.com> writes:
>
>> On Sat, 20 Apr 2024 18:33:07 -0300
>> Thiago Jung Bauermann <thiago.bauermann@linaro.org> wrote:
>>
>>> +# It's not possible to check in which libc function the watchpoint triggers
>>> +# without its debug info.
>>> +require libc_has_debug_info
>>
>> I'm wondering about the need for this requirement.  When I comment it
>> out and run it on a machine without libc debuginfo, I do see 3 FAILs,
>> but it seems to me that those could be turned into PASSes by changing
>> the regular expressions for the "continue until..." tests.
>>
>> E.g. for the first one, with libc debuginfo, I see:
>>
>> continue
>> Continuing.
>>
>> Hardware watchpoint 2: -location a[31]
>>
>> Old value = 101 'e'
>> New value = 0 '\000'
>> __memset_avx2_unaligned () at
>> ../sysdeps/x86_64/multiarch/memset-vec-unaligned-erms.S:146
>> 146		VMOVU	%VMM(0), (%rdi)
>> (gdb) PASS: gdb.base/memops-watchpoint.exp: continue until memset watchpoint hits
>>
>> But, without libc debuginfo, the watchpoint still works:
>>
>> continue
>> Continuing.
>>
>> Hardware watchpoint 2: -location a[31]
>>
>> Old value = 101 'e'
>> New value = 0 '\000'
>> 0x00007ffff7e3553a in __memset_avx2_unaligned () from /lib64/libc.so.6
>> (gdb) FAIL: gdb.base/memops-watchpoint.exp: continue until memset watchpoint hits
>>
>> As stated earlier, this could be turned into a PASS by tweaking the RE.
>>
>> In both cases, we know that it's in a "memset" function.  (The presence
>> of minimal symbols provides GDB with this information.)
>
> I added the requirement because in my aarch64-linux system without libc6
> debug info I get:
>
> continue
> Continuing.
>
> Hardware watchpoint 2: -location a[28]
>
> Old value = 104 'h'
> New value = 0 '\000'
> 0x0000fffff7e90664 in ?? () from /lib/aarch64-linux-gnu/libc.so.6
> (gdb) FAIL: gdb.base/memops-watchpoint.exp: continue until memset watchpoint hits

<snip>

> So it depends on the system.
>
> One alternative would be to not use the require statement and run the
> test until the watchpoint hits, and have a case in gdb_test_multiple to
> mark as UNRESOLVED if the function name is '??'.

I was able to do this in v3. But I mark the test as UNSUPPORTED rather
than UNRESOLVED. The difference between them isn't always clear in my
mind, but I think UNSUPPORTED is better in this case.

--
Thiago

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] gdb/testsuite: Add gdb.base/memops-watchpoint.exp
  2024-04-22 18:40       ` Kevin Buettner
@ 2024-04-23  1:20         ` Thiago Jung Bauermann
  0 siblings, 0 replies; 11+ messages in thread
From: Thiago Jung Bauermann @ 2024-04-23  1:20 UTC (permalink / raw)
  To: Kevin Buettner; +Cc: gdb-patches

Kevin Buettner <kevinb@redhat.com> writes:

> On Sun, 21 Apr 2024 21:24:42 -0300
> Thiago Jung Bauermann <thiago.bauermann@linaro.org> wrote:
>
>> >> +require libc_has_debug_info
>> >
>> > I'm wondering about the need for this requirement.  When I comment it
>> > out and run it on a machine without libc debuginfo, I do see 3 FAILs,
>> > but it seems to me that those could be turned into PASSes by changing
>> > the regular expressions for the "continue until..." tests.
> [...]
>>
>> I added the requirement because in my aarch64-linux system without libc6
>> debug info I get:
>>
>> continue
>> Continuing.
>>
>> Hardware watchpoint 2: -location a[28]
>>
>> Old value = 104 'h'
>> New value = 0 '\000'
>> 0x0000fffff7e90664 in ?? () from /lib/aarch64-linux-gnu/libc.so.6
>> (gdb) FAIL: gdb.base/memops-watchpoint.exp: continue until memset watchpoint hits
>>
>> And I just tested removing libc6-dbg from my x86_64-linux laptop:
>>
>> continue
>> Continuing.
>>
>> Hardware watchpoint 2: -location a[28]
>>
>> Old value = 104 'h'
>> New value = 0 '\000'
>> 0x00007ffff7d8e05f in ?? () from /lib/x86_64-linux-gnu/libc.so.6
>> (gdb) FAIL: gdb.base/memops-watchpoint.exp: continue until memset watchpoint hits
>>
>> So it depends on the system.
>
> What distro are you using?

Ubuntu. Version 22.04 on some machines, and version 23.10 on others.

>> One alternative would be to not use the require statement and run the
>> test until the watchpoint hits, and have a case in gdb_test_multiple to
>> mark as UNRESOLVED if the function name is '??'.
>
> I'm in favor of this approach.
>
> If we stick with the require statement, I think that Fedora testing
> will frequently show this new test as unsupported since installing
> debuginfo is less common / important that it used to be.  (This is
> due to debuginfod doing it for you.  But I think that debuginfod is
> mostly disabled when running the GDB tests.)

Yes, that is a good point. I think v3 should work with the output you
pasted in a previous email. Thank you for bring this up.

--
Thiago

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2024-04-23  1:20 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-20 21:33 [PATCH 0/2] Add testcase for libc memory operations Thiago Jung Bauermann
2024-04-20 21:33 ` [PATCH 1/2] gdb/testsuite: Add libc_has_debug_info require helper Thiago Jung Bauermann
2024-04-21 20:44   ` Kevin Buettner
2024-04-21 21:02     ` Kevin Buettner
2024-04-22  0:05       ` Thiago Jung Bauermann
2024-04-20 21:33 ` [PATCH 2/2] gdb/testsuite: Add gdb.base/memops-watchpoint.exp Thiago Jung Bauermann
2024-04-21 21:20   ` Kevin Buettner
2024-04-22  0:24     ` Thiago Jung Bauermann
2024-04-22 18:40       ` Kevin Buettner
2024-04-23  1:20         ` Thiago Jung Bauermann
2024-04-22 23:04       ` Thiago Jung Bauermann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).