diff mbox

[v2] Explicitly use language_c when evaluating a SDT probe argument

Message ID 1413052811-18167-1-git-send-email-sergiodj@redhat.com
State New
Headers show

Commit Message

Sergio Durigan Junior Oct. 11, 2014, 6:40 p.m. UTC
This is the second version of the patch, which includes a testcase as
requested by Pedro.

Joel contacted me offlist with a question about a warning that one of
his customers was seeing.  The message came from the new
linker-debugger interface, which uses SDT probes internally.  The
warning said:

    (gdb) run
    [...]
    warning: Probes-based dynamic linker interface failed.
    Reverting to original interface.

    Argument to arithmetic operation not a number or boolean.

This should not have happened in the environment the customer was
using (RHEL-6.x), so I found it strange.  Another thing caught my
attention: the last message, saying "Argument to arithmetic operation
not a number or boolean.".

Joel kindly investigated the issue further, and found the answer for
this.  To quote him:

	(gdb) set lang c
	(gdb) p 48+$ebp
	$4 = (void *) 0xffffd0f8

    So far so good. But...

	(gdb) set lang ada
	(gdb) p 48+$ebp
	Argument to arithmetic operation not a number or boolean.

    Ooops! Interestingly, if you revert the order of the operands...

	(gdb) p $ebp+48
	$5 = (access void) 0xffffd0f8

So the problem is doing pointer arithmetics when the language is set
to Ada.

I remembered that, during the parsing and the evaluation of SDT probe
arguments, the code sets the language as current_language, because, at
that time, I thought it was not necessary to worry about the language
given that the code implements its own parser.  I was wrong.  So here
is a patch to fix that, by setting the language as C, which should
guarantee that the maths are done in the right way (TM).

It was somewhat hard to find a reproducer for this issue.  In the end,
what I had to do was to create a testcase that used the %ebp register
on some displacement (e.g., "-4(%ebp)"), which finally triggered the
bug.  I am not sure why I could not trigger it when using other
registers, but I did not want to spend too much time investigating
this issue, which seemed like an Ada issue.  Also, because of this
peculiar way to trigger the problem, the testcase only covers x86-like
targets (i.e., i*86 and x86_64 with -m32).

Joel kindly tested this for me, and it worked.  I also ran a full
regression test here on my Fedora 20 x86_64, and everything is fine.

I will push this patch in a few days if there are no comments.

gdb/ChangeLog:
2014-10-11  Sergio Durigan Junior  <sergiodj@redhat.com>

	* stap-probe.c (stap_parse_argument): Initialize expout explicitly
	using language_c, instead of current_language.

gdb/testsuite/ChangeLog:
2014-10-11  Sergio Durigan Junior  <sergiodj@redhat.com>

	* gdb.arch/stap-eval-lang-ada.S: Likewise.
	* gdb.arch/stap-eval-lang-ada.c: Likewise.
	* gdb.arch/stap-eval-lang-ada.exp: New file.
---
 gdb/stap-probe.c                                   |  6 +-
 gdb/testsuite/gdb.arch/i386-stap-eval-lang-ada.S   | 81 ++++++++++++++++++++++
 gdb/testsuite/gdb.arch/i386-stap-eval-lang-ada.c   | 29 ++++++++
 gdb/testsuite/gdb.arch/i386-stap-eval-lang-ada.exp | 35 ++++++++++
 4 files changed, 148 insertions(+), 3 deletions(-)
 create mode 100644 gdb/testsuite/gdb.arch/i386-stap-eval-lang-ada.S
 create mode 100644 gdb/testsuite/gdb.arch/i386-stap-eval-lang-ada.c
 create mode 100644 gdb/testsuite/gdb.arch/i386-stap-eval-lang-ada.exp

Comments

Sergio Durigan Junior Oct. 14, 2014, 6:35 p.m. UTC | #1
On Saturday, October 11 2014, I wrote:

> This is the second version of the patch, which includes a testcase as
> requested by Pedro.

Pushed.

  <https://sourceware.org/ml/gdb-cvs/2014-10/msg00042.html>
Joel Brobecker Oct. 14, 2014, 9:07 p.m. UTC | #2
> > This is the second version of the patch, which includes a testcase as
> > requested by Pedro.
> 
> Pushed.
> 
>   <https://sourceware.org/ml/gdb-cvs/2014-10/msg00042.html>

Thanks, Pedro. And since you took the time to fix the issue on
the SDT side, I took the time to push the ada-lang.c patch which
I wrote on our side :-).

https://www.sourceware.org/ml/gdb-patches/2014-10/msg00371.html
Sergio Durigan Junior Oct. 14, 2014, 9:14 p.m. UTC | #3
On Tuesday, October 14 2014, Joel Brobecker wrote:

> Thanks, Pedro. And since you took the time to fix the issue on
> the SDT side, I took the time to push the ada-lang.c patch which
> I wrote on our side :-).

My pleasure!  (though I imagine you meant "Sergio" :-P.)

> https://www.sourceware.org/ml/gdb-patches/2014-10/msg00371.html

This is what I call a two-factor fix :-).
Pedro Alves Oct. 15, 2014, 1:32 p.m. UTC | #4
On 10/11/2014 07:40 PM, Sergio Durigan Junior wrote:
> This is the second version of the patch, which includes a testcase as
> requested by Pedro.

Thanks Sergio.
diff mbox

Patch

diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
index a51bf8b..3902997 100644
--- a/gdb/stap-probe.c
+++ b/gdb/stap-probe.c
@@ -1050,9 +1050,9 @@  stap_parse_argument (const char **arg, struct type *atype,
   struct cleanup *back_to;
 
   /* We need to initialize the expression buffer, in order to begin
-     our parsing efforts.  The language here does not matter, since we
-     are using our own parser.  */
-  initialize_expout (&p.pstate, 10, current_language, gdbarch);
+     our parsing efforts.  We use language_c here because we may need
+     to do pointer arithmetics.  */
+  initialize_expout (&p.pstate, 10, language_def (language_c), gdbarch);
   back_to = make_cleanup (free_current_contents, &p.pstate.expout);
 
   p.saved_arg = *arg;
diff --git a/gdb/testsuite/gdb.arch/i386-stap-eval-lang-ada.S b/gdb/testsuite/gdb.arch/i386-stap-eval-lang-ada.S
new file mode 100644
index 0000000..26f060a
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/i386-stap-eval-lang-ada.S
@@ -0,0 +1,81 @@ 
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2014 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/>.  */
+
+/* This file has been generated using the following command:
+
+     gcc -m32 -S i386-stap-eval-lang-ada.c -o i386-stap-eval-lang-ada.S
+
+   The important thing about this is that the probe argument (below)
+   should be exactly "-4@-4(%ebp)" in order to trigger the bug.  */
+
+	.file	"i386-stap-eval-lang-ada.c"
+	.text
+	.globl	main
+	.type	main, @function
+main:
+.LFB0:
+	.cfi_startproc
+	pushl	%ebp
+	.cfi_def_cfa_offset 8
+	.cfi_offset 5, -8
+	movl	%esp, %ebp
+	.cfi_def_cfa_register 5
+	subl	$16, %esp
+	movl	$40, -4(%ebp)
+#APP
+# 27 "i386-stap-eval-lang-ada.c" 1
+	990: nop
+.pushsection .note.stapsdt,"?","note"
+.balign 4
+.4byte 992f-991f,994f-993f,3
+991: .asciz "stapsdt"
+992: .balign 4
+993: .4byte 990b
+.4byte _.stapsdt.base
+.4byte 0
+.asciz "foo"
+.asciz "bar"
+/* The following probe argument should be "-4@-4(%ebp)", or some other
+   register displacement expression that references %ebp, otherwise the
+   bug will not trigger.  */
+.asciz "-4@-4(%ebp)"
+994: .balign 4
+.popsection
+
+# 0 "" 2
+# 27 "i386-stap-eval-lang-ada.c" 1
+	.ifndef _.stapsdt.base
+.pushsection .stapsdt.base,"aG","progbits",.stapsdt.base,comdat
+.weak _.stapsdt.base
+.hidden _.stapsdt.base
+_.stapsdt.base: .space 1
+.size _.stapsdt.base,1
+.popsection
+.endif
+
+# 0 "" 2
+#NO_APP
+	movl	$0, %eax
+	leave
+	.cfi_restore 5
+	.cfi_def_cfa 4, 4
+	ret
+	.cfi_endproc
+.LFE0:
+	.size	main, .-main
+	.ident	"GCC: (GNU) 4.8.3 20140911 (Red Hat 4.8.3-7)"
+	.section	.note.GNU-stack,"",@progbits
diff --git a/gdb/testsuite/gdb.arch/i386-stap-eval-lang-ada.c b/gdb/testsuite/gdb.arch/i386-stap-eval-lang-ada.c
new file mode 100644
index 0000000..78308cd
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/i386-stap-eval-lang-ada.c
@@ -0,0 +1,29 @@ 
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2014 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/>.  */
+
+/* This file is not used directly; instead, it has been used to
+   generate the .S file.  See comments on the .S file for more info.  */
+
+#include <sys/sdt.h>
+
+int main ()
+{
+  int a = 40;
+
+  STAP_PROBE1 (foo, bar, a);
+  return 0; 
+}
diff --git a/gdb/testsuite/gdb.arch/i386-stap-eval-lang-ada.exp b/gdb/testsuite/gdb.arch/i386-stap-eval-lang-ada.exp
new file mode 100644
index 0000000..6afe758
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/i386-stap-eval-lang-ada.exp
@@ -0,0 +1,35 @@ 
+# Copyright 2014 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/>.
+
+standard_testfile ".S"
+
+# We can only test this if the target is i686 or x86_64 with -m32
+if { ![is_x86_like_target] } {
+    verbose "Skipping $testfile.exp"
+    return
+}
+
+if { [prepare_for_testing "failed to prepare" $testfile $srcfile] } {
+    return -1
+}
+
+gdb_test_no_output "set language ada"
+
+if { ![runto "-pstap bar"] } {
+    return -1
+}
+
+gdb_test "print \$_probe_arg0" " = 40" \
+    "printing \$_probe_arg0"