public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH][gdb/testsuite] Fix gdb.btrace/tsx.exp on system with tsx disabled in microcode
@ 2021-07-12 11:27 Tom de Vries
  2021-07-12 11:54 ` Metzger, Markus T
  0 siblings, 1 reply; 6+ messages in thread
From: Tom de Vries @ 2021-07-12 11:27 UTC (permalink / raw)
  To: gdb-patches

Hi,

Recently I started to see this fail with trunk:
...
(gdb) record instruction-history^M
1          0x00000000004004ab <main+4>: call   0x4004b7 <test>^M
2          0x00000000004004c6 <test+15>:        mov    $0x1,%eax^M
3          0x00000000004004cb <test+20>:        ret    ^M
(gdb) FAIL: gdb.btrace/tsx.exp: speculation indication
...

This is due to an intel microcode update (1) that disables Intel TSX by default.

Fix this by updating the pattern.

Tested on x86_64-linux.

[1] https://www.intel.com/content/www/us/en/support/articles/000059422/processors.html

Any comments?

Thanks,
- Tom

[gdb/testsuite] Fix gdb.btrace/tsx.exp on system with tsx disabled in microcode

gdb/testsuite/ChangeLog:

2021-07-12  Tom de Vries  <tdevries@suse.de>

	PR testsuite/28057
	* gdb.btrace/tsx.exp: Add pattern for system with tsx disabled in
	microcode.

---
 gdb/testsuite/gdb.btrace/tsx.exp | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/gdb/testsuite/gdb.btrace/tsx.exp b/gdb/testsuite/gdb.btrace/tsx.exp
index ccde1ea807e..7f96313f1b1 100644
--- a/gdb/testsuite/gdb.btrace/tsx.exp
+++ b/gdb/testsuite/gdb.btrace/tsx.exp
@@ -59,6 +59,11 @@ set abort_2 [multi_line \
     "\[0-9\]*\t   0x\[0-9a-f\]+ <test\\+\[0-9\]+>:\tmov\[^\\\r\\\n\]*" \
     "\[0-9\]*\t   0x\[0-9a-f\]+ <test\\+\[0-9\]+>:\tret\[^\\\r\\\n\]*" \
     ]
+set abort_3 \
+    [multi_line \
+	 "$decimal\t   $hex <main\\+$decimal>:\tcall\[^\\\r\\\n\]*" \
+	 "$decimal\t   $hex <test\\+$decimal>:\tmov\[^\\\r\\\n\]*" \
+	 "$decimal\t   $hex <test\\+$decimal>:\tret\[^\\\r\\\n\]*"]
 
 set test "speculation indication"
 gdb_test_multiple "record instruction-history" $test {
@@ -68,6 +73,9 @@ gdb_test_multiple "record instruction-history" $test {
     -re "$abort_2.*$gdb_prompt $" {
         pass $test
     }
+    -re -wrap "$abort_3" {
+        pass $gdb_test_name
+    }
     -re  "$begin_to_end.*$gdb_prompt $" {
         pass $test
     }

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

* RE: [PATCH][gdb/testsuite] Fix gdb.btrace/tsx.exp on system with tsx disabled in microcode
  2021-07-12 11:27 [PATCH][gdb/testsuite] Fix gdb.btrace/tsx.exp on system with tsx disabled in microcode Tom de Vries
@ 2021-07-12 11:54 ` Metzger, Markus T
  2021-07-12 14:05   ` Tom de Vries
  0 siblings, 1 reply; 6+ messages in thread
From: Metzger, Markus T @ 2021-07-12 11:54 UTC (permalink / raw)
  To: Tom de Vries; +Cc: gdb-patches

Hello Tom,


>diff --git a/gdb/testsuite/gdb.btrace/tsx.exp b/gdb/testsuite/gdb.btrace/tsx.exp
>index ccde1ea807e..7f96313f1b1 100644
>--- a/gdb/testsuite/gdb.btrace/tsx.exp
>+++ b/gdb/testsuite/gdb.btrace/tsx.exp
>@@ -59,6 +59,11 @@ set abort_2 [multi_line \
>     "\[0-9\]*\t   0x\[0-9a-f\]+ <test\\+\[0-9\]+>:\tmov\[^\\\r\\\n\]*" \
>     "\[0-9\]*\t   0x\[0-9a-f\]+ <test\\+\[0-9\]+>:\tret\[^\\\r\\\n\]*" \
>     ]
>+set abort_3 \
>+    [multi_line \
>+	 "$decimal\t   $hex <main\\+$decimal>:\tcall\[^\\\r\\\n\]*" \
>+	 "$decimal\t   $hex <test\\+$decimal>:\tmov\[^\\\r\\\n\]*" \
>+	 "$decimal\t   $hex <test\\+$decimal>:\tret\[^\\\r\\\n\]*"]

The patterns do not include the call since this is compiler-generated.  The actual
TSX test is written in assembly so we know the instructions.


> set test "speculation indication"
> gdb_test_multiple "record instruction-history" $test {
>@@ -68,6 +73,9 @@ gdb_test_multiple "record instruction-history" $test {
>     -re "$abort_2.*$gdb_prompt $" {
>         pass $test
>     }
>+    -re -wrap "$abort_3" {
>+        pass $gdb_test_name
>+    }

Does this '-wrap' add ".*$gdb_prompt $"?

Note that we need the ".*" after the pattern since this code is compiler-generated
and we don't really know when we will stop after returning from test ().

Regards,
Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928


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

* Re: [PATCH][gdb/testsuite] Fix gdb.btrace/tsx.exp on system with tsx disabled in microcode
  2021-07-12 11:54 ` Metzger, Markus T
@ 2021-07-12 14:05   ` Tom de Vries
  2021-07-12 14:35     ` Metzger, Markus T
  0 siblings, 1 reply; 6+ messages in thread
From: Tom de Vries @ 2021-07-12 14:05 UTC (permalink / raw)
  To: Metzger, Markus T; +Cc: gdb-patches

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

On 7/12/21 1:54 PM, Metzger, Markus T wrote:
> Hello Tom,
> 
> 
>> diff --git a/gdb/testsuite/gdb.btrace/tsx.exp b/gdb/testsuite/gdb.btrace/tsx.exp
>> index ccde1ea807e..7f96313f1b1 100644
>> --- a/gdb/testsuite/gdb.btrace/tsx.exp
>> +++ b/gdb/testsuite/gdb.btrace/tsx.exp
>> @@ -59,6 +59,11 @@ set abort_2 [multi_line \
>>     "\[0-9\]*\t   0x\[0-9a-f\]+ <test\\+\[0-9\]+>:\tmov\[^\\\r\\\n\]*" \
>>     "\[0-9\]*\t   0x\[0-9a-f\]+ <test\\+\[0-9\]+>:\tret\[^\\\r\\\n\]*" \
>>     ]
>> +set abort_3 \
>> +    [multi_line \
>> +	 "$decimal\t   $hex <main\\+$decimal>:\tcall\[^\\\r\\\n\]*" \
>> +	 "$decimal\t   $hex <test\\+$decimal>:\tmov\[^\\\r\\\n\]*" \
>> +	 "$decimal\t   $hex <test\\+$decimal>:\tret\[^\\\r\\\n\]*"]
> 
> The patterns do not include the call since this is compiler-generated.  The actual
> TSX test is written in assembly so we know the instructions.
> 

I see.  Fixed by not requiring a specific instruction.

> 
>> set test "speculation indication"
>> gdb_test_multiple "record instruction-history" $test {
>> @@ -68,6 +73,9 @@ gdb_test_multiple "record instruction-history" $test {
>>     -re "$abort_2.*$gdb_prompt $" {
>>         pass $test
>>     }
>> +    -re -wrap "$abort_3" {
>> +        pass $gdb_test_name
>> +    }
> 
> Does this '-wrap' add ".*$gdb_prompt $"?
> 
> Note that we need the ".*" after the pattern since this code is compiler-generated
> and we don't really know when we will stop after returning from test ().


The -wrap processes the pattern in exactly the same way as gdb_test does:
...
        if { $wrap_pattern } {
            # Wrap subst_item as is done for the gdb_test PATTERN
            # argument.
            lappend $current_list \
                "\[\r\n\]*(?:$subst_item)\[\r\n\]+$gdb_prompt $"
            set wrap_pattern 0
        } else {
...

After running with clang, I ran into the FAIL you anticipated, which is
indeed fixed by adding .* in the pattern.

Updated patch attached.  OK for trunk?

Thanks,
- Tom


[-- Attachment #2: 0001-gdb-testsuite-Fix-gdb.btrace-tsx.exp-on-system-with-tsx-disabled-in-microcode.patch --]
[-- Type: text/x-patch, Size: 1925 bytes --]

[gdb/testsuite] Fix gdb.btrace/tsx.exp on system with tsx disabled in microcode

Recently I started to see this fail with trunk:
...
(gdb) record instruction-history^M
1          0x00000000004004ab <main+4>: call   0x4004b7 <test>^M
2          0x00000000004004c6 <test+15>:        mov    $0x1,%eax^M
3          0x00000000004004cb <test+20>:        ret    ^M
(gdb) FAIL: gdb.btrace/tsx.exp: speculation indication
...

This is due to an intel microcode update (1) that disables Intel TSX by default.

Fix this by updating the pattern.

Tested on x86_64-linux, with both gcc 7.5.0 and clang 12.0.1.

[1] https://www.intel.com/content/www/us/en/support/articles/000059422/processors.html

gdb/testsuite/ChangeLog:

2021-07-12  Tom de Vries  <tdevries@suse.de>

	PR testsuite/28057
	* gdb.btrace/tsx.exp: Add pattern for system with tsx disabled in
	microcode.

---
 gdb/testsuite/gdb.btrace/tsx.exp | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/gdb/testsuite/gdb.btrace/tsx.exp b/gdb/testsuite/gdb.btrace/tsx.exp
index ccde1ea807e..66f6305e50a 100644
--- a/gdb/testsuite/gdb.btrace/tsx.exp
+++ b/gdb/testsuite/gdb.btrace/tsx.exp
@@ -59,6 +59,11 @@ set abort_2 [multi_line \
     "\[0-9\]*\t   0x\[0-9a-f\]+ <test\\+\[0-9\]+>:\tmov\[^\\\r\\\n\]*" \
     "\[0-9\]*\t   0x\[0-9a-f\]+ <test\\+\[0-9\]+>:\tret\[^\\\r\\\n\]*" \
     ]
+set abort_3 \
+    [multi_line \
+	 "$decimal\t   $hex <main\\+$decimal>:\t\[^\\\r\\\n\]*" \
+	 "$decimal\t   $hex <test\\+$decimal>:\tmov\[^\\\r\\\n\]*" \
+	 "$decimal\t   $hex <test\\+$decimal>:\tret\[^\\\r\\\n\]*"]
 
 set test "speculation indication"
 gdb_test_multiple "record instruction-history" $test {
@@ -68,6 +73,9 @@ gdb_test_multiple "record instruction-history" $test {
     -re "$abort_2.*$gdb_prompt $" {
         pass $test
     }
+    -re -wrap "$abort_3.*" {
+        pass $gdb_test_name
+    }
     -re  "$begin_to_end.*$gdb_prompt $" {
         pass $test
     }

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

* RE: [PATCH][gdb/testsuite] Fix gdb.btrace/tsx.exp on system with tsx disabled in microcode
  2021-07-12 14:05   ` Tom de Vries
@ 2021-07-12 14:35     ` Metzger, Markus T
  2021-07-12 15:08       ` Tom de Vries
  0 siblings, 1 reply; 6+ messages in thread
From: Metzger, Markus T @ 2021-07-12 14:35 UTC (permalink / raw)
  To: Tom de Vries; +Cc: gdb-patches

Hello Tom,

> diff --git a/gdb/testsuite/gdb.btrace/tsx.exp b/gdb/testsuite/gdb.btrace/tsx.exp
> index ccde1ea807e..66f6305e50a 100644
> --- a/gdb/testsuite/gdb.btrace/tsx.exp
> +++ b/gdb/testsuite/gdb.btrace/tsx.exp
> @@ -59,6 +59,11 @@ set abort_2 [multi_line \
>      "\[0-9\]*\t   0x\[0-9a-f\]+ <test\\+\[0-9\]+>:\tmov\[^\\\r\\\n\]*" \
>      "\[0-9\]*\t   0x\[0-9a-f\]+ <test\\+\[0-9\]+>:\tret\[^\\\r\\\n\]*" \
>      ]
> +set abort_3 \
> +    [multi_line \
> +	 "$decimal\t   $hex <main\\+$decimal>:\t\[^\\\r\\\n\]*" \
> +	 "$decimal\t   $hex <test\\+$decimal>:\tmov\[^\\\r\\\n\]*" \
> +	 "$decimal\t   $hex <test\\+$decimal>:\tret\[^\\\r\\\n\]*"]
>  
>  set test "speculation indication"
>  gdb_test_multiple "record instruction-history" $test {
> @@ -68,6 +73,9 @@ gdb_test_multiple "record instruction-history" $test {
>      -re "$abort_2.*$gdb_prompt $" {
>          pass $test
>      }
> +    -re -wrap "$abort_3.*" {
> +        pass $gdb_test_name
> +    }
>      -re  "$begin_to_end.*$gdb_prompt $" {
>          pass $test
>      }

We allow (and require) a single instruction in main.  That works if we stopped
directly at the call.  I don't think we can guarantee that.

In the other patterns, I put .* in front to ignore any compiler-generated
code prior to the actual test.

Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

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

* Re: [PATCH][gdb/testsuite] Fix gdb.btrace/tsx.exp on system with tsx disabled in microcode
  2021-07-12 14:35     ` Metzger, Markus T
@ 2021-07-12 15:08       ` Tom de Vries
  2021-07-12 15:19         ` Metzger, Markus T
  0 siblings, 1 reply; 6+ messages in thread
From: Tom de Vries @ 2021-07-12 15:08 UTC (permalink / raw)
  To: Metzger, Markus T; +Cc: gdb-patches

On 7/12/21 4:35 PM, Metzger, Markus T wrote:
> Hello Tom,
> 
>> diff --git a/gdb/testsuite/gdb.btrace/tsx.exp b/gdb/testsuite/gdb.btrace/tsx.exp
>> index ccde1ea807e..66f6305e50a 100644
>> --- a/gdb/testsuite/gdb.btrace/tsx.exp
>> +++ b/gdb/testsuite/gdb.btrace/tsx.exp
>> @@ -59,6 +59,11 @@ set abort_2 [multi_line \
>>      "\[0-9\]*\t   0x\[0-9a-f\]+ <test\\+\[0-9\]+>:\tmov\[^\\\r\\\n\]*" \
>>      "\[0-9\]*\t   0x\[0-9a-f\]+ <test\\+\[0-9\]+>:\tret\[^\\\r\\\n\]*" \
>>      ]
>> +set abort_3 \
>> +    [multi_line \
>> +	 "$decimal\t   $hex <main\\+$decimal>:\t\[^\\\r\\\n\]*" \
>> +	 "$decimal\t   $hex <test\\+$decimal>:\tmov\[^\\\r\\\n\]*" \
>> +	 "$decimal\t   $hex <test\\+$decimal>:\tret\[^\\\r\\\n\]*"]
>>  
>>  set test "speculation indication"
>>  gdb_test_multiple "record instruction-history" $test {
>> @@ -68,6 +73,9 @@ gdb_test_multiple "record instruction-history" $test {
>>      -re "$abort_2.*$gdb_prompt $" {
>>          pass $test
>>      }
>> +    -re -wrap "$abort_3.*" {
>> +        pass $gdb_test_name
>> +    }
>>      -re  "$begin_to_end.*$gdb_prompt $" {
>>          pass $test
>>      }
> 
> We allow (and require) a single instruction in main.
> That works if we stopped
> directly at the call.  I don't think we can guarantee that.
> 

Well, we require at least one instruction.  There can be more that one.
So that should work if we stopped directly at the call, or before.

The current form is only wrong if there is no instruction from main in
the trace, which I don't expect given that we start recording in main.
Is my understanding wrong here?

> In the other patterns, I put .* in front to ignore any compiler-generated
> code prior to the actual test.

A .* at the start of a -re clause is redundant, so it's best to leave
that out, to avoid confusion.

Are you satisfied with this explanation, or do still require changes?

Thanks,
- Tom

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

* RE: [PATCH][gdb/testsuite] Fix gdb.btrace/tsx.exp on system with tsx disabled in microcode
  2021-07-12 15:08       ` Tom de Vries
@ 2021-07-12 15:19         ` Metzger, Markus T
  0 siblings, 0 replies; 6+ messages in thread
From: Metzger, Markus T @ 2021-07-12 15:19 UTC (permalink / raw)
  To: Tom de Vries; +Cc: gdb-patches

Hello Tom,

>> We allow (and require) a single instruction in main.
>> That works if we stopped
>> directly at the call.  I don't think we can guarantee that.
>>
>
>Well, we require at least one instruction.  There can be more that one.
>So that should work if we stopped directly at the call, or before.
>
>The current form is only wrong if there is no instruction from main in
>the trace, which I don't expect given that we start recording in main.
>Is my understanding wrong here?

No, that's correct.


>> In the other patterns, I put .* in front to ignore any compiler-generated
>> code prior to the actual test.
>
>A .* at the start of a -re clause is redundant, so it's best to leave
>that out, to avoid confusion.

Here's my confusion, apparently.  So the .* I put in front of those patterns
is simply redundant.


>Are you satisfied with this explanation, or do still require changes?

I'm fine in that case.  Thanks for explaining.

LGTM,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

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

end of thread, other threads:[~2021-07-12 15:19 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-12 11:27 [PATCH][gdb/testsuite] Fix gdb.btrace/tsx.exp on system with tsx disabled in microcode Tom de Vries
2021-07-12 11:54 ` Metzger, Markus T
2021-07-12 14:05   ` Tom de Vries
2021-07-12 14:35     ` Metzger, Markus T
2021-07-12 15:08       ` Tom de Vries
2021-07-12 15:19         ` Metzger, Markus T

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).