diff mbox series

x86: replace XFAILs in "GOTX default" tests

Message ID 6f16a2fd-4aad-4071-b710-642b9c2576e1@suse.com
State New
Headers
Series x86: replace XFAILs in "GOTX default" tests |

Commit Message

Jan Beulich Dec. 4, 2025, 2:02 p.m. UTC 
  The use of XFAIL was wrong here. XFAIL marks tests which in principle
should work, but where making them work requires extra effort.

Comments

Rainer Orth Dec. 4, 2025, 3:43 p.m. UTC | #1 
Hi Jan,

> The use of XFAIL was wrong here. XFAIL marks tests which in principle
> should work, but where making them work requires extra effort.
>
> --- a/gas/testsuite/gas/i386/gotx-default.d
> +++ b/gas/testsuite/gas/i386/gotx-default.d
> @@ -1,6 +1,6 @@
>  #source: gotx.s
>  #objdump: -dwr
> -#xfail: *-*-solaris*
> +#skip: *-*-solaris*
>  
>  .*: +file format .*
>  
> --- a/gas/testsuite/gas/i386/no-gotx-default.d
> +++ b/gas/testsuite/gas/i386/no-gotx-default.d
> @@ -1,7 +1,6 @@
>  #source: gotx.s
>  #objdump: -dwr
> -#xfail: *-*-*
> -#noxfail: *-*-solaris*
> +#noskip: *-*-solaris*
>  
>  .*: +file format .*

this is wrong: the tests are intended to verify that the
-mrelax-relocations defaut remains as desired (off on Solaris, on
elsewhere).  With xfail's, any change results in an XPASS, while with
skip's the change goes unnoticed because the tests are never run.

	Rainer
Jan Beulich Dec. 4, 2025, 3:54 p.m. UTC | #2 
On 04.12.2025 16:43, Rainer Orth wrote:
> Hi Jan,
> 
>> The use of XFAIL was wrong here. XFAIL marks tests which in principle
>> should work, but where making them work requires extra effort.
>>
>> --- a/gas/testsuite/gas/i386/gotx-default.d
>> +++ b/gas/testsuite/gas/i386/gotx-default.d
>> @@ -1,6 +1,6 @@
>>  #source: gotx.s
>>  #objdump: -dwr
>> -#xfail: *-*-solaris*
>> +#skip: *-*-solaris*
>>  
>>  .*: +file format .*
>>  
>> --- a/gas/testsuite/gas/i386/no-gotx-default.d
>> +++ b/gas/testsuite/gas/i386/no-gotx-default.d
>> @@ -1,7 +1,6 @@
>>  #source: gotx.s
>>  #objdump: -dwr
>> -#xfail: *-*-*
>> -#noxfail: *-*-solaris*
>> +#noskip: *-*-solaris*
>>  
>>  .*: +file format .*
> 
> this is wrong: the tests are intended to verify that the
> -mrelax-relocations defaut remains as desired (off on Solaris, on
> elsewhere).  With xfail's, any change results in an XPASS, while with
> skip's the change goes unnoticed because the tests are never run.

But then XFAIL is still wrong to use. Since the same file is assembled twice
(as two independent test cases), a change in default would still be notice by
the non-skipped testcase failing.

Jan
Rainer Orth Dec. 12, 2025, 3:25 p.m. UTC | #3 
Hi Jan,

> On 04.12.2025 16:43, Rainer Orth wrote:
>> Hi Jan,
>> 
>>> The use of XFAIL was wrong here. XFAIL marks tests which in principle
>>> should work, but where making them work requires extra effort.
>>>
>>> --- a/gas/testsuite/gas/i386/gotx-default.d
>>> +++ b/gas/testsuite/gas/i386/gotx-default.d
>>> @@ -1,6 +1,6 @@
>>>  #source: gotx.s
>>>  #objdump: -dwr
>>> -#xfail: *-*-solaris*
>>> +#skip: *-*-solaris*
>>>  
>>>  .*: +file format .*
>>>  
>>> --- a/gas/testsuite/gas/i386/no-gotx-default.d
>>> +++ b/gas/testsuite/gas/i386/no-gotx-default.d
>>> @@ -1,7 +1,6 @@
>>>  #source: gotx.s
>>>  #objdump: -dwr
>>> -#xfail: *-*-*
>>> -#noxfail: *-*-solaris*
>>> +#noskip: *-*-solaris*
>>>  
>>>  .*: +file format .*
>> 
>> this is wrong: the tests are intended to verify that the
>> -mrelax-relocations defaut remains as desired (off on Solaris, on
>> elsewhere).  With xfail's, any change results in an XPASS, while with
>> skip's the change goes unnoticed because the tests are never run.
>
> But then XFAIL is still wrong to use. Since the same file is assembled twice
> (as two independent test cases), a change in default would still be notice by
> the non-skipped testcase failing.

While I understand your desire to avoid running tests unnecessarily, I'm
wary about skipping tests for a couple of reasons:

* The original POSIX 1001.3 spec didn't SKIP for a reason: the intent
  was that the same set of tests should be run everywhere so test
  results could be compared without problems: every test would have a
  result, no exceptions.

* I'm well aware that this ship has long sailed, though, in particular
  in GCC and Binutils where whole groups of tests are skipped if they
  cannot be run.

* However, skipping them can lead to undesirable consequences.  One
  particular bad example in recent times has been LLVM where the tests
  for the builtins part of compiler-rt were accidentally not run any
  longer without anyone noticing at all.  Once I'd reported the issue,
  it took a year and a half to finally address this (and the patch is
  still not in, I believe).

* Another possible issue is restricting tests to a particular set of
  targets without proper analysis (like restricting them to Linux
  although they'd run perfectly fine elsewhere).  This has the
  unfortunate consequence that one either looses PASSing results that
  would confirm the feature works.  Alternatively, when there are issues
  with some feature on a skipped target, you won't get notified that
  something is wrong.

Since it took me some time to get the original version of the tests
right, I've tested this patch in a variety of configurations:

amd64 is Solaris/amd64, x86_64 is Linux/x86_64.  invert means the
-mrelax-relocations default was swapped in tc-i386.c: Linux off, Solaris
on:

				gotx-default.d	no-gotx-default.d

				amd64	x86_64	amd64	x86_64

  original tests		XFAIL	PASS	PASS	XFAIL	
  original and invert		XPASS	FAIL	FAIL	XPASS

  with patch   			SKIP	PASS	PASS	SKIP	
  with patch and invert		SKIP	FAIL	FAIL	SKIP

Long story short: the patch is good to go.

	Rainer
diff mbox series

Patch

--- a/gas/testsuite/gas/i386/gotx-default.d
+++ b/gas/testsuite/gas/i386/gotx-default.d
@@ -1,6 +1,6 @@ 
 #source: gotx.s
 #objdump: -dwr
-#xfail: *-*-solaris*
+#skip: *-*-solaris*
 
 .*: +file format .*
 
--- a/gas/testsuite/gas/i386/no-gotx-default.d
+++ b/gas/testsuite/gas/i386/no-gotx-default.d
@@ -1,7 +1,6 @@ 
 #source: gotx.s
 #objdump: -dwr
-#xfail: *-*-*
-#noxfail: *-*-solaris*
+#noskip: *-*-solaris*
 
 .*: +file format .*