Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reapply #2 [clang-repl] [test] Make an XFAIL more precise #71168

Merged
merged 1 commit into from
Nov 7, 2023

Conversation

mstorsjo
Copy link
Member

@mstorsjo mstorsjo commented Nov 3, 2023

The const.cpp testcase fails when running in MSVC mode, while it does succeed in MinGW mode.

In MSVC mode, there are more constructor invocations than expected, as the printout looks like this:

A(1), this = 0000025597930000
A(1), this = 0000025597930000
f: this = 0000025597930000, val = 1
A(1), this = 0000025597930000
f: this = 0000025597930000, val = 1
~A, this = 0000025597930000, val = 1
~A, this = 0000025597930000, val = 1
~A, this = 0000025597930000, val = 1

While the expected printout looks like this:

A(1), this = 000002C903E10000
f: this = 000002C903E10000, val = 1
f: this = 000002C903E10000, val = 1
~A, this = 000002C903E10000, val = 1

Reapplying #70991 with the XFAIL changed to check the host triple, not the target triple. On an MSVC based build of Clang, but with the default target triple set to PS4/PS5, we will still see the failure. And a Linux based build of Clang that targets PS4/PS5 won't see the issue.

…0991)

The const.cpp testcase fails when running in MSVC mode, while it does
succeed in MinGW mode.

In MSVC mode, there are more constructor invocations than expected, as
the printout looks like this:

    A(1), this = 0000025597930000
    A(1), this = 0000025597930000
    f: this = 0000025597930000, val = 1
    A(1), this = 0000025597930000
    f: this = 0000025597930000, val = 1
    ~A, this = 0000025597930000, val = 1
    ~A, this = 0000025597930000, val = 1
    ~A, this = 0000025597930000, val = 1

While the expected printout looks like this:

    A(1), this = 000002C903E10000
    f: this = 000002C903E10000, val = 1
    f: this = 000002C903E10000, val = 1
    ~A, this = 000002C903E10000, val = 1

Reapplying with the XFAIL changed to check the host triple,
not the target triple. On an MSVC based build of Clang, but
with the default target triple set to PS4/PS5, we will still
see the failure. And a Linux based build of Clang that targets
PS4/PS5 won't see the issue.
@mstorsjo mstorsjo added clang Clang issues not falling into any other category platform:windows labels Nov 3, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 3, 2023

@llvm/pr-subscribers-testing-tools

@llvm/pr-subscribers-clang

Author: Martin Storsjö (mstorsjo)

Changes

The const.cpp testcase fails when running in MSVC mode, while it does succeed in MinGW mode.

In MSVC mode, there are more constructor invocations than expected, as the printout looks like this:

A(1), this = 0000025597930000
A(1), this = 0000025597930000
f: this = 0000025597930000, val = 1
A(1), this = 0000025597930000
f: this = 0000025597930000, val = 1
~A, this = 0000025597930000, val = 1
~A, this = 0000025597930000, val = 1
~A, this = 0000025597930000, val = 1

While the expected printout looks like this:

A(1), this = 000002C903E10000
f: this = 000002C903E10000, val = 1
f: this = 000002C903E10000, val = 1
~A, this = 000002C903E10000, val = 1

Reapplying with the XFAIL changed to check the host triple, not the target triple. On an MSVC based build of Clang, but with the default target triple set to PS4/PS5, we will still see the failure. And a Linux based build of Clang that targets PS4/PS5 won't see the issue.


Full diff: https://github.com/llvm/llvm-project/pull/71168.diff

3 Files Affected:

  • (modified) clang/test/Interpreter/const.cpp (+1-1)
  • (modified) llvm/test/lit.cfg.py (-4)
  • (modified) llvm/utils/lit/lit/llvm/config.py (+1)
diff --git a/clang/test/Interpreter/const.cpp b/clang/test/Interpreter/const.cpp
index 4b6ce65e3643e64..86358c1a54fbdde 100644
--- a/clang/test/Interpreter/const.cpp
+++ b/clang/test/Interpreter/const.cpp
@@ -1,6 +1,6 @@
 // UNSUPPORTED: system-aix
 // see https://github.com/llvm/llvm-project/issues/68092
-// XFAIL: system-windows
+// XFAIL: host={{.*}}-windows-msvc
 
 // RUN: cat %s | clang-repl | FileCheck %s
 // RUN: cat %s | clang-repl -Xcc -O2 | FileCheck %s
diff --git a/llvm/test/lit.cfg.py b/llvm/test/lit.cfg.py
index ab245b71cdd16a5..022d1aedbdcdbb6 100644
--- a/llvm/test/lit.cfg.py
+++ b/llvm/test/lit.cfg.py
@@ -477,10 +477,6 @@ def have_cxx_shared_library():
     if not config.target_triple.startswith(("nvptx", "xcore")):
         config.available_features.add("object-emission")
 
-# Allow checking for specific details in the host triple
-if config.host_triple:
-    config.available_features.add('host=%s' % config.host_triple)
-
 if config.have_llvm_driver:
     config.available_features.add("llvm-driver")
 
diff --git a/llvm/utils/lit/lit/llvm/config.py b/llvm/utils/lit/lit/llvm/config.py
index 16cc2968034bf74..79094b839e772e7 100644
--- a/llvm/utils/lit/lit/llvm/config.py
+++ b/llvm/utils/lit/lit/llvm/config.py
@@ -97,6 +97,7 @@ def __init__(self, lit_config, config):
         # part of the standard header.  But currently they aren't)
         host_triple = getattr(config, "host_triple", None)
         target_triple = getattr(config, "target_triple", None)
+        features.add("host=%s" % host_triple)
         features.add("target=%s" % target_triple)
         if host_triple and host_triple == target_triple:
             features.add("native")

@llvmbot
Copy link
Collaborator

llvmbot commented Nov 3, 2023

@llvm/pr-subscribers-platform-windows

Author: Martin Storsjö (mstorsjo)

Changes

The const.cpp testcase fails when running in MSVC mode, while it does succeed in MinGW mode.

In MSVC mode, there are more constructor invocations than expected, as the printout looks like this:

A(1), this = 0000025597930000
A(1), this = 0000025597930000
f: this = 0000025597930000, val = 1
A(1), this = 0000025597930000
f: this = 0000025597930000, val = 1
~A, this = 0000025597930000, val = 1
~A, this = 0000025597930000, val = 1
~A, this = 0000025597930000, val = 1

While the expected printout looks like this:

A(1), this = 000002C903E10000
f: this = 000002C903E10000, val = 1
f: this = 000002C903E10000, val = 1
~A, this = 000002C903E10000, val = 1

Reapplying with the XFAIL changed to check the host triple, not the target triple. On an MSVC based build of Clang, but with the default target triple set to PS4/PS5, we will still see the failure. And a Linux based build of Clang that targets PS4/PS5 won't see the issue.


Full diff: https://github.com/llvm/llvm-project/pull/71168.diff

3 Files Affected:

  • (modified) clang/test/Interpreter/const.cpp (+1-1)
  • (modified) llvm/test/lit.cfg.py (-4)
  • (modified) llvm/utils/lit/lit/llvm/config.py (+1)
diff --git a/clang/test/Interpreter/const.cpp b/clang/test/Interpreter/const.cpp
index 4b6ce65e3643e64..86358c1a54fbdde 100644
--- a/clang/test/Interpreter/const.cpp
+++ b/clang/test/Interpreter/const.cpp
@@ -1,6 +1,6 @@
 // UNSUPPORTED: system-aix
 // see https://github.com/llvm/llvm-project/issues/68092
-// XFAIL: system-windows
+// XFAIL: host={{.*}}-windows-msvc
 
 // RUN: cat %s | clang-repl | FileCheck %s
 // RUN: cat %s | clang-repl -Xcc -O2 | FileCheck %s
diff --git a/llvm/test/lit.cfg.py b/llvm/test/lit.cfg.py
index ab245b71cdd16a5..022d1aedbdcdbb6 100644
--- a/llvm/test/lit.cfg.py
+++ b/llvm/test/lit.cfg.py
@@ -477,10 +477,6 @@ def have_cxx_shared_library():
     if not config.target_triple.startswith(("nvptx", "xcore")):
         config.available_features.add("object-emission")
 
-# Allow checking for specific details in the host triple
-if config.host_triple:
-    config.available_features.add('host=%s' % config.host_triple)
-
 if config.have_llvm_driver:
     config.available_features.add("llvm-driver")
 
diff --git a/llvm/utils/lit/lit/llvm/config.py b/llvm/utils/lit/lit/llvm/config.py
index 16cc2968034bf74..79094b839e772e7 100644
--- a/llvm/utils/lit/lit/llvm/config.py
+++ b/llvm/utils/lit/lit/llvm/config.py
@@ -97,6 +97,7 @@ def __init__(self, lit_config, config):
         # part of the standard header.  But currently they aren't)
         host_triple = getattr(config, "host_triple", None)
         target_triple = getattr(config, "target_triple", None)
+        features.add("host=%s" % host_triple)
         features.add("target=%s" % target_triple)
         if host_triple and host_triple == target_triple:
             features.add("native")

@mstorsjo
Copy link
Member Author

mstorsjo commented Nov 3, 2023

Posting for a second review instead of just relanding the patch as is; in order to check the host triple, I had to add the host=triple string; it was previously only available for tests under llvm/test, but let's move it to the common llvm test configuration just like the target=triple strings, so that it is available for tests under clang as well.

Copy link
Member

@hahnjo hahnjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable to me. I know this fixes a test error for MinGW, but if possible maybe let it sit until early next week in case somebody else has a different opinion on moving host= to lit.

@mstorsjo mstorsjo changed the title Reapply #2 [clang-repl] [test] Make an XFAIL more precise (#70991) Reapply #2 [clang-repl] [test] Make an XFAIL more precise Nov 7, 2023
@mstorsjo mstorsjo merged commit 2c4f938 into llvm:main Nov 7, 2023
6 of 7 checks passed
@mstorsjo mstorsjo deleted the clang-repl-xfail branch November 7, 2023 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang Clang issues not falling into any other category llvm-lit platform:windows testing-tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants