Re: [PATCH 2/2] Extend recognized types of SDT probe's arguments

Sergio Durigan Junior
2014-05-02T19:25:46Z

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

On Friday, May 02 2014, Pedro Alves wrote:

> On 05/01/2014 10:52 PM, Sergio Durigan Junior wrote:
>> This commit is actually an update to make the parser in
>> gdb/stap-probe.c be aware of all the possible prefixes that a probe
>> argument can have. According to the section "Argument Format" in:
>>
>> <https://sourceware.org/systemtap/wiki/UserSpaceProbeImplementation>
>>
>> The bitness of the arguments can be 8, 16, 32 or 64 bits, signed or
>> unsigned. Currently GDB recognizes only 32 and 64-bit arguments.
>
> Looks good.

Thanks.

>> This commit extends this. Since this is a straightforward extension,
>> I am not submitting a testcase; I can do that if anybody wants.
>
> I think it'd be good to have a test -- the code that triggered the other
> bug was also supposedly straightforward. :-) Can we do this in C ?
> Ideally we'd also test that we don't crash with an invalid bitness
> (the complaint path).

Hm, OK, here is the testcase then.

>>
>> gdb/
>> 2014-05-01 Sergio Durigan Junior <sergiodj@redhat.com>
>>
>> stap-probe.c (enum stap_arg_bitness): New enums to represent 8
>
> Missing '*'-

Writing ChangeLog entries while at "git commit" buffer is tricky...
Second mistake in a row! Thanks.

>> and 16-bit signed and unsigned arguments.
>> (stap_parse_probe_arguments): Extend code to handle such
>> arguments.
>
> --
> Pedro Alves

Here is what I will push if there are no other comments.

Thanks,

--
Sergio

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 111.patch --]
[-- Type: text/x-patch, Size: 7532 bytes --]

commit 4fdce9c1c58588209e26cd690e7e653b6056d458
Author: Sergio Durigan Junior <sergiodj@redhat.com>
Date: Thu May 1 18:39:28 2014 -0300

Extend recognized types of SDT probe's arguments

This commit is actually an update to make the parser in
gdb/stap-probe.c be aware of all the possible prefixes that a probe
argument can have. According to the section "Argument Format" in:

<https://sourceware.org/systemtap/wiki/UserSpaceProbeImplementation>

The bitness of the arguments can be 8, 16, 32 or 64 bits, signed or
unsigned. Currently GDB recognizes only 32 and 64-bit arguments.
This commit extends this. Since this is a straightforward extension,
I am not submitting a testcase; I can do that if anybody wants.

gdb/
2014-05-02 Sergio Durigan Junior <sergiodj@redhat.com>

* stap-probe.c (enum stap_arg_bitness): New enums to represent 8
and 16-bit signed and unsigned arguments. Update comment.
(stap_parse_probe_arguments): Extend code to handle such
arguments. Use warning instead of complaint to notify about
unrecognized bitness.

gdb/testsuite/
2014-05-02 Sergio Durigan Junior <sergiodj@redhat.com>

* gdb.arch/amd64-stap-optional-prefix.S (main): Add several
probes to test for bitness recognition.
* gdb.arch/amd64-stap-optional-prefix.exp
(test_probe_value_without_reg): New procedure.
Add code to test for different kinds of bitness.

diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
index ef45495..84714b5 100644
--- a/gdb/stap-probe.c
+++ b/gdb/stap-probe.c
@@ -60,6 +60,10 @@ static unsigned int stap_expression_debug = 0;
The relationship is:

- STAP_ARG_BITNESS_UNDEFINED: The user hasn't specified the bitness.
+ - STAP_ARG_BITNESS_8BIT_UNSIGNED: argument string starts with `1@'.
+ - STAP_ARG_BITNESS_8BIT_SIGNED: argument string starts with `-1@'.
+ - STAP_ARG_BITNESS_16BIT_UNSIGNED: argument string starts with `2@'.
+ - STAP_ARG_BITNESS_16BIT_SIGNED: argument string starts with `-2@'.
- STAP_ARG_BITNESS_32BIT_UNSIGNED: argument string starts with `4@'.
- STAP_ARG_BITNESS_32BIT_SIGNED: argument string starts with `-4@'.
- STAP_ARG_BITNESS_64BIT_UNSIGNED: argument string starts with `8@'.
@@ -68,6 +72,10 @@ static unsigned int stap_expression_debug = 0;
enum stap_arg_bitness
{
STAP_ARG_BITNESS_UNDEFINED,
+ STAP_ARG_BITNESS_8BIT_UNSIGNED,
+ STAP_ARG_BITNESS_8BIT_SIGNED,
+ STAP_ARG_BITNESS_16BIT_UNSIGNED,
+ STAP_ARG_BITNESS_16BIT_SIGNED,
STAP_ARG_BITNESS_32BIT_UNSIGNED,
STAP_ARG_BITNESS_32BIT_SIGNED,
STAP_ARG_BITNESS_64BIT_UNSIGNED,
@@ -329,6 +337,18 @@ stap_get_expected_argument_type (struct gdbarch *gdbarch,
else
return builtin_type (gdbarch)->builtin_uint64;

+ case STAP_ARG_BITNESS_8BIT_UNSIGNED:
+ return builtin_type (gdbarch)->builtin_uint8;
+
+ case STAP_ARG_BITNESS_8BIT_SIGNED:
+ return builtin_type (gdbarch)->builtin_int8;
+
+ case STAP_ARG_BITNESS_16BIT_UNSIGNED:
+ return builtin_type (gdbarch)->builtin_uint16;
+
+ case STAP_ARG_BITNESS_16BIT_SIGNED:
+ return builtin_type (gdbarch)->builtin_int16;
+
case STAP_ARG_BITNESS_32BIT_SIGNED:
return builtin_type (gdbarch)->builtin_int32;

@@ -1095,7 +1115,7 @@ stap_parse_probe_arguments (struct stap_probe *probe, struct gdbarch *gdbarch)

N@OP

- Where `N' can be [+,-][4,8]. This is not mandatory, so
+ Where `N' can be [+,-][1,2,4,8]. This is not mandatory, so
we check it here. If we don't find it, go to the next
state. */
if ((cur[0] == '-' && isdigit (cur[1]) && cur[2] == '@')
@@ -1108,20 +1128,37 @@ stap_parse_probe_arguments (struct stap_probe *probe, struct gdbarch *gdbarch)
got_minus = 1;
}

- if (*cur == '4')
- b = (got_minus ? STAP_ARG_BITNESS_32BIT_SIGNED
- : STAP_ARG_BITNESS_32BIT_UNSIGNED);
- else if (*cur == '8')
- b = (got_minus ? STAP_ARG_BITNESS_64BIT_SIGNED
- : STAP_ARG_BITNESS_64BIT_UNSIGNED);
- else
+ /* Defining the bitness. */
+ switch (*cur)
{
- /* We have an error, because we don't expect anything
- except 4 and 8. */
- complaint (&symfile_complaints,
- _("unrecognized bitness `%c' for probe `%s'"),
- *cur, probe->p.name);
- return;
+ case '1':
+ b = (got_minus ? STAP_ARG_BITNESS_8BIT_SIGNED
+ : STAP_ARG_BITNESS_8BIT_UNSIGNED);
+ break;
+
+ case '2':
+ b = (got_minus ? STAP_ARG_BITNESS_16BIT_SIGNED
+ : STAP_ARG_BITNESS_16BIT_UNSIGNED);
+ break;
+
+ case '4':
+ b = (got_minus ? STAP_ARG_BITNESS_32BIT_SIGNED
+ : STAP_ARG_BITNESS_32BIT_UNSIGNED);
+ break;
+
+ case '8':
+ b = (got_minus ? STAP_ARG_BITNESS_64BIT_SIGNED
+ : STAP_ARG_BITNESS_64BIT_UNSIGNED);
+ break;
+
+ default:
+ {
+ /* We have an error, because we don't expect anything
+ except 1, 2, 4 and 8. */
+ warning (_("unrecognized bitness %s%c' for probe `%s'"),
+ got_minus ? "`-" : "`", *cur, probe->p.name);
+ return;
+ }
}

arg.bitness = b;
diff --git a/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.S b/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.S
index 66689cb..3cdb4c2 100644
--- a/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.S
+++ b/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.S
@@ -28,5 +28,15 @@ main:
STAP_PROBE1(probe, foo_prefix, 8@(%rsp))
STAP_PROBE1(probe, bar_prefix, 8@-8(%rbp))
mov %rbp,%rsp
+ STAP_PROBE1(probe, uint8_probe, 1@$8)
+ STAP_PROBE1(probe, int8_probe, -1@$-8)
+ STAP_PROBE1(probe, uint16_probe, 2@$16)
+ STAP_PROBE1(probe, int16_probe, -2@$-16)
+ STAP_PROBE1(probe, uint32_probe, 4@$32)
+ STAP_PROBE1(probe, int32_probe, -4@$-32)
+ STAP_PROBE1(probe, uint64_probe, 8@$64)
+ STAP_PROBE1(probe, int64_probe, -8@$-64)
+ STAP_PROBE1(probe, fail_probe, -7@$16)
+ STAP_PROBE1(probe, fail2_probe, 23-@$16)
xor %rax,%rax
ret
diff --git a/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp b/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp
index cc9d6c3..b7f1505 100644
--- a/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp
+++ b/gdb/testsuite/gdb.arch/amd64-stap-optional-prefix.exp
@@ -43,6 +43,11 @@ proc test_probe_value { value reg_val } {
gdb_test "print \$_probe_arg0 == *((unsigned int *) (${reg_val}))" "= 1"
}

+proc test_probe_value_without_reg { value } {
+ gdb_test "print \$_probe_argc" "= 1"
+ gdb_test "print \$_probe_arg0" "= $value"
+}
+
if { ![runto_main] } {
return -1
}
@@ -55,3 +60,32 @@ foreach probe_name [list "foo" "bar" "foo_prefix" "bar_prefix"] \
test_probe_value $probe_val $probe_reg_val
}
}
+
+# Testing normal probes, with several prefixes.
+
+set normal_probes_names { }
+
+foreach bit [list 8 16 32 64] {
+ lappend normal_probes_names "uint${bit}_probe"
+ lappend normal_probes_names "int${bit}_probe"
+}
+
+foreach probe_name $normal_probes_names \
+ probe_val [list 8 -8 16 -16 32 -32 64 -64] {
+ with_test_prefix $probe_name {
+ goto_probe $probe_name
+ test_probe_value_without_reg $probe_val
+ }
+}
+
+# Testing the fail probes.
+
+with_test_prefix "fail_probe" {
+ goto_probe "fail_probe"
+ gdb_test "print \$_probe_arg0" "warning: unrecognized bitness `-7' for probe `fail_probe'\r\nInvalid probe argument 0 -- probe has 0 arguments available"
+}
+
+with_test_prefix "fail2_probe" {
+ goto_probe "fail2_probe"
+ gdb_test "print \$_probe_arg0" "Unknown numeric token on expression `23-@\\\$16'."
+}