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

Message ID 20210712112738.GA2111@delia
State New
Headers show
Series
  • [gdb/testsuite] Fix gdb.btrace/tsx.exp on system with tsx disabled in microcode
Related show

Commit Message

Tom de Vries July 12, 2021, 11:27 a.m.
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(+)

Comments

Simon Marchi via Gdb-patches July 12, 2021, 11:54 a.m. | #1
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
Tom de Vries July 12, 2021, 2:05 p.m. | #2
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
[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
     }
Simon Marchi via Gdb-patches July 12, 2021, 2:35 p.m. | #3
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
Tom de Vries July 12, 2021, 3:08 p.m. | #4
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
Simon Marchi via Gdb-patches July 12, 2021, 3:19 p.m. | #5
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

Patch

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
     }