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

[clang-repl] [test] Make an XFAIL more precise #70991

Merged
merged 1 commit into from
Nov 2, 2023

Conversation

mstorsjo
Copy link
Member

@mstorsjo mstorsjo commented Nov 1, 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

The const.cpp testcase fails when running in MSVC mode, while they
do 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
@mstorsjo mstorsjo requested review from hahnjo and vgvassilev November 1, 2023 21:42
@llvmbot llvmbot added the clang Clang issues not falling into any other category label Nov 1, 2023
@llvmbot
Copy link
Member

llvmbot commented Nov 1, 2023

@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

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

1 Files Affected:

  • (modified) clang/test/Interpreter/const.cpp (+1-1)
diff --git a/clang/test/Interpreter/const.cpp b/clang/test/Interpreter/const.cpp
index 4b6ce65e3643e64..1412a1d85d6f58f 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: target={{.*}}-windows-msvc
 
 // RUN: cat %s | clang-repl | FileCheck %s
 // RUN: cat %s | clang-repl -Xcc -O2 | FileCheck %s

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.

Very interesting... See also #68092, now I understand even less what the problem is...

Copy link
Contributor

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

Thank you. Lgtm!

@mstorsjo
Copy link
Member Author

mstorsjo commented Nov 1, 2023

Very interesting... See also #68092, now I understand even less what the problem is...

No idea actually, but I tested passing -Xcc --target=x86_64-w64-mingw32 to an MSVC-built clang-repl, and then it outputs the expected things.

Not sure at what level some JIT deduplication should be happening, but anyway, the MSVC C++ ABI is distinctly different from the Itanium C++ ABI, in most aspects.

@mstorsjo mstorsjo merged commit 3bc056d into llvm:main Nov 2, 2023
3 checks passed
@mstorsjo mstorsjo deleted the clang-repl-test-mingw branch November 2, 2023 07:51
@mstorsjo
Copy link
Member Author

mstorsjo commented Nov 2, 2023

This broke on PS5 bots, like https://lab.llvm.org/buildbot/#/builders/216/builds/29677; those are configured with a triple like x86_64-sie-ps5, which seems to use an MSVC like C++ ABI behaviour, so I pushed a revert.

Not sure whom to CC to pull in Sony people to discuss this matter, so trying @pogo59. Can we use something like XFAIL: target={{.*}}-windows-msvc, target={{.*}-ps4, target={{.*}-ps5 to specifically point towards the Sony PS triples that also use the MSVC C++ ABI here? The ideal would be something like XFAIL: default-target-is-msvc-cxx-abi, but I don't think we have that. I see triples x86_64-scei-ps4 and x86_64-sie-ps5 being mentioned elsewhere in Clang tests as examples of PS4/PS5 triples.

mstorsjo added a commit that referenced this pull request Nov 2, 2023
This reverts commit 3bc056d.

This broke on bots with a target triple of x86_64-sie-ps5,
which also appear to behave like the MSVC case.
@pogo59
Copy link
Collaborator

pogo59 commented Nov 2, 2023

FTR, the "Worker" tab on that buildbot page will point you to the maintainer. But tagging me is also fine in general.
I'm unable to repro the problem locally because my local build doesn't seem to include clang-repl.exe, so the whole clang/test/Interpreter directory is Unsupported. Is there some cmake parameter to enable JIT?
In the meantime, tagging @dyung

@pogo59
Copy link
Collaborator

pogo59 commented Nov 2, 2023

If you want to XFAIL specifically for the Sony targets, what you suggested would work.
I'm unclear about the "MSVC C++ ABI" aspect, but if that gets the test to work, go for it.

@mstorsjo
Copy link
Member Author

mstorsjo commented Nov 2, 2023

FTR, the "Worker" tab on that buildbot page will point you to the maintainer.

Ah, there it is, I tried looking around, but overlooked that one...

But tagging me is also fine in general.

Ok, thanks!

I'm unable to repro the problem locally because my local build doesn't seem to include clang-repl.exe, so the whole clang/test/Interpreter directory is Unsupported. Is there some cmake parameter to enable JIT?

Not sure - by doing ninja clang-repl I was able to get those tests running at least.

If you want to XFAIL specifically for the Sony targets, what you suggested would work. I'm unclear about the "MSVC C++ ABI" aspect, but if that gets the test to work, go for it.

Ok, great, thanks! I'll try that and reland the commit.

@dyung
Copy link
Collaborator

dyung commented Nov 2, 2023

If you still need help reproducing or debugging the issue on our bot, please let me know.

@mstorsjo
Copy link
Member Author

mstorsjo commented Nov 2, 2023

If you still need help reproducing or debugging the issue on our bot, please let me know.

Thanks, much appreciated. Can you test if mstorsjo@clang-repl-xfail seems to run correctly in this environment? Otherwise I'll try to push it tomorrow and see how it fares on the bot.

@dyung
Copy link
Collaborator

dyung commented Nov 3, 2023

If you still need help reproducing or debugging the issue on our bot, please let me know.

Thanks, much appreciated. Can you test if mstorsjo@clang-repl-xfail seems to run correctly in this environment? Otherwise I'll try to push it tomorrow and see how it fares on the bot.

Sure, I'll take it down this evening sometime and give it a try.

@dyung
Copy link
Collaborator

dyung commented Nov 3, 2023

If you still need help reproducing or debugging the issue on our bot, please let me know.

Thanks, much appreciated. Can you test if mstorsjo@clang-repl-xfail seems to run correctly in this environment? Otherwise I'll try to push it tomorrow and see how it fares on the bot.

It failed, but due to a typo in the XFAIL you added:

target={{.*}-ps4, target={{.*}-ps5

These should have balanced braces. If I add the missing braces, the test passes with an XFAIL on the bot.

@mstorsjo
Copy link
Member Author

mstorsjo commented Nov 3, 2023

If you still need help reproducing or debugging the issue on our bot, please let me know.

Thanks, much appreciated. Can you test if mstorsjo@clang-repl-xfail seems to run correctly in this environment? Otherwise I'll try to push it tomorrow and see how it fares on the bot.

It failed, but due to a typo in the XFAIL you added:

target={{.*}-ps4, target={{.*}-ps5

These should have balanced braces. If I add the missing braces, the test passes with an XFAIL on the bot.

Oh, oops - but thanks for checking and verifying with the typo fixed. I'll try to reland this change now then, with that fixed. Thanks again!

mstorsjo added a commit that referenced this pull request 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 with the XFAIL pattern expanded to include PS4/PS5
triples as well, which also seem to have the same behaviour
as MSVC.
mstorsjo added a commit that referenced this pull request Nov 3, 2023
This reverts commit e9db60c.
This was still failing (unexpectedly passing) on some Sony PS
buildbots.

The issue is that the clang-repl testcases don't depend on what
the default target triple is, but what the host triple is,
which is used for JIT purposes.
mstorsjo added a commit to mstorsjo/llvm-project that referenced this pull request Nov 3, 2023
…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 added a commit that referenced this pull request Nov 7, 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.
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants