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

Skip tests which rely on Mips backend. #733

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

veselypeta
Copy link

Currently, the Mips target is required to be built for many cheri tests to pass even if only targeting RISCV. This PR annotates all tests which rely on the Mips backend with the REQUIRES keyword in order to successfully skip over them without reporting as a failure. Additionally I've added a --arch option to regenerate-all.py in order to allow you to regenerate these tests for only the specific architecture you're targetting.

@veselypeta veselypeta force-pushed the petr/break-mips-riscv-dependancy branch from f6cafcf to c578e69 Compare April 26, 2024 16:45
@veselypeta
Copy link
Author

@arichardson Does this seem sensible to you?

Copy link
Member

@jrtc27 jrtc27 left a comment

Choose a reason for hiding this comment

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

I'm a bit concerned this means you're not building with the MIPS backend and are thus losing out on a lot of test coverage?

@@ -42,7 +43,8 @@ void test_onstack_int_overaligned(struct Foo *s) {

void test__stack_array(void) {
char buf[333];
do_stuff_with_buf(buf); // CSV-NEXT: 0,333,?,"{{.+}}/csetbounds-stats-all.cpp:45:21","Add subobject bounds","array decay for char[333]"
do_stuff_with_buf(buf);
Copy link
Member

Choose a reason for hiding this comment

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

Leave these alone

Copy link
Author

Choose a reason for hiding this comment

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

The thing here is that the line number 45 is hard coded, by adding the // REQUIRES: cause causes this test to fail. All the other assertions use the [[@LINE]] directive to get the line making this clause an outlier. Is there a specific reason why this assertion needs to be this way?

@veselypeta veselypeta force-pushed the petr/break-mips-riscv-dependancy branch from c578e69 to fd7059e Compare May 20, 2024 08:43
@veselypeta
Copy link
Author

We are still building the Mips backend in order to leverage the Mips testing for testing any CHERI changes. However, I still think this change is useful as you can still build a RISCV target without CHERI and still have all the tests passing.

@@ -81,6 +81,9 @@ def __init__(self):
parser = argparse.ArgumentParser()
parser.add_argument("--llvm-bindir", type=Path, required=True)
parser.add_argument("--verbose", action="store_true")
parser.add_argument("--arch", default='all',
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@arichardson arichardson left a comment

Choose a reason for hiding this comment

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

I think it makes sense to add these annotations even though I'd strongly recommend to keep running the MIPS tests - RISC-V tests don't cover everything.

@@ -1,3 +1,4 @@
// REQUIRES: mips-registered-target
Copy link
Member

Choose a reason for hiding this comment

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

Should not be needed for Driver tests. I wonder which line fails?

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

That shouldn't matter...

Copy link
Member

Choose a reason for hiding this comment

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

Ah and if that file is not built then the flag is not accepted. Should probably just modify the test to not actually consume the flag but just check the generated command line.

clang/test/Driver/cheri/cheri-cc1as.S Show resolved Hide resolved
@@ -1,3 +1,4 @@
// REQUIRES: mips-registered-target
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't need a registered target?

Copy link
Author

Choose a reason for hiding this comment

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

Same issue as below with -mxcaptable

Copy link
Member

Choose a reason for hiding this comment

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

Hmm this may be slightly annoying to fix properly, so just adding this requires seems fine to me.

Copy link
Member

Choose a reason for hiding this comment

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

Well, this test isn't really a Driver test, as it's using -emit-llvm

Copy link
Member

Choose a reason for hiding this comment

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

That's fair, ideally -emit-llvm should be removed. Maybe the flag validation only happens in the backend but it's been a long time since I looked at that code. I wonder if -E also verifies it?

Copy link
Author

Choose a reason for hiding this comment

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

I could be wrong here. But the tests here invoke clang directly, which invokes the driver too. If we're just testing frontend code I think these tests should be modified to only invoke clang -cc1

clang/test/CodeGenCXX/cheri/frame_layout_hardfloat.cpp Outdated Show resolved Hide resolved
For the script to succeed it requires that MIPS & RISCV targets
are registered. The --arch option allows the script to run only
against the targets which you're building for.
@veselypeta veselypeta force-pushed the petr/break-mips-riscv-dependancy branch from 2975066 to cbc5ba1 Compare June 5, 2024 17:15
Copy link
Member

@arichardson arichardson left a comment

Choose a reason for hiding this comment

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

Looks good to me with a cherry-pick of the Morello update script instead of the --arch flag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants