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

Remove lit #305

Closed
wants to merge 5 commits into from
Closed

Remove lit #305

wants to merge 5 commits into from

Conversation

sim-immunant
Copy link
Contributor

This PR completely removes lit.

Notably, Criterion does not test certain things such as segments, linker args, or that the rewriter produces the exact output, as lit did. My hope and belief is that the fact that the test compiles and passes is evidence that these are as expected.

@@ -6,7 +6,6 @@ message(STATUS "Found LLVM: ${LLVM_DIR} (found version \"${LLVM_PACKAGE_VERSION}
find_package(Clang REQUIRED CONFIG)
message(STATUS "Found Clang: ${Clang_DIR}")

# We use the lit CMake functions from LLVM
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check-ia2 works without these lines but nginx build seems to need these

/*
RUN: cp %s %t.h
RUN: ia2-rewriter --omit-wrappers %t.c %t.h -- -I%resource_dir
RUN: sh -c "[ ! -e %t.c.args ]"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what this is doing. Is the Criterion test sufficient to test what this is testing?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry that's my bad, this test should've been removed when we switched over to the source rewriter. The header rewriter required more flags to get it working on real codebases and this was testing one of those flags.

@ayrtonm
Copy link
Contributor

ayrtonm commented Oct 25, 2023

I thought we settled on keeping lit but making it opt-in rather than opt-out. rewriter/tests/lit.cfg.py has a config.excludes with the test directories that are excluded from lit, but what we should do is exclude everything by default and only include directories that we want to test with lit. I don't know if lit.cfg provides anything for that but I think that file is just a python script so it should be simple enough.

Criterion does not test certain things such as segments, linker args, or that the rewriter produces the exact output

I don't think we can necessarily assume that because a test builds and runs that some of these things are correct. The segment tests aren't necessary anymore since our linker script (libia2/padding.ld) is much simpler than before and shouldn't accidentally change segment permissions like we noticed before. For linker args we can assume the call gates were inserted if the test coverage is sufficient. I wouldn't bother with removing the existing linker args tests if a directory needs lit tests though, it's just something to keep in mind for new tests. For rewriter changes we definitely want explicit lit tests.

@sim-immunant
Copy link
Contributor Author

Since we've decided not to remove lit but instead to reduce its scope and make it opt-in rather than automatic, I'm going to close this PR and make a new one that's freshly branched off main.

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.

2 participants