Tidy cvise stuff #86
Closed
Tidy cvise stuff #86
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Root dir:
url: https://www.lua.org/ftp/lua-5.4.4.tar.gz
sha1: 03c27684b9d5d9783fb79a7c836ba1cdc5f309cd
`tests` dir:
url: https://www.lua.org/tests/lua-5.4.4-tests.tar.gz
sha256: 04d28355cd67a2299dfe5708b55a0ff221ccb1a3907a3113cc103ccc05ac6aad
Lua has no official public repo, so we are having to do it this way.
Although there is https://github.com/lua/lua, this repo doesn't contain
all of the source code. For example `luac` (a useful tool for dumping
Lua bytecode) is missing.
1: Import lua 5.4.4 and set up CI. r=ltratt a=vext01 Due to the way Lua development is, the git workflow is going to be a bit convoluted. In this repo there is no `main`/`master` branch. We will have a branch for each Lua release, and we will import the upstream Lua code from tarballs. This sets up the `lua-5.4.4` branch ready for our yk patches (next PR). Co-authored-by: Edd Barrett <[email protected]>
Obviously nothing really works yet, but this is a good starting point.
The JIT can be conditionally compiled in if `YK_DIR` is set (e.g. `make
YK_DIR=~/yk`). Sadly I had to make a portable Makefile into a GNU
Makefile for that to work though. I don't think it matters too much.
Currently we only make JIT locations for regular `for` loops.
The test suite currently seg faults when using the JIT, so that's
commented in CI for now.
This simple program makes it to stop-gapping and then fails a
stackmap-related assertion:
```
for i = 0, 100 do
print("hello!")
end
```
It's a start.
2: Retrofit the Yk JIT into the Lua interpreter. r=ltratt a=vext01
Obviously nothing really works yet, but this is a good starting point.
The JIT can be conditionally compiled in if `YK_DIR` is set (e.g. `make
YK_DIR=~/yk`). Sadly I had to make a portable Makefile into a GNU
Makefile for that to work though. I don't think it matters too much.
Currently we only make JIT locations for regular `for` loops.
The test suite currently seg faults when using the JIT, so that's
commented in CI for now.
This simple program makes it to stop-gapping and then fails a
stackmap-related assertion:
```
for i = 0, 100 do
print("hello!")
end
```
It's a start.
Co-authored-by: Edd Barrett <[email protected]>
This is required for accurate stack reconstruction until we implement the "build the interpreter twice" idea.
3: Add compiler flags and a couple of other bits. r=ltratt a=vext01 Co-authored-by: Edd Barrett <[email protected]>
4: Use yk-config. r=ltratt a=vext01 Co-authored-by: Edd Barrett <[email protected]>
Before you needed to set (and it wasn't well-documented): - `PATH` to include `yk-config`. - `YK_BUILD_TYPE` to `debug` or `release`. - `YK_DIR` to the `yk` clone. This change removes the need for `YK_DIR`.
5: Remove the need to set YK_DIR. r=ltratt a=vext01 Before you needed to set (and it wasn't well-documented): - `PATH` to include `yk-config`. - `YK_BUILD_TYPE` to `debug` or `release`. - `YK_DIR` to the `yk` clone. This change removes the need for `YK_DIR` (and gives an error message if `yk-config` can't be found). Co-authored-by: Edd Barrett <[email protected]>
yk_mt_new() changed.
7: Make yklua build again. r=ltratt a=vext01 yk_mt_new() changed. Co-authored-by: Edd Barrett <[email protected]>
We thought that the YkLinkage pass would solve this, but alas.
8: Use the correct test path and one hack to get us going. r=ptersilie a=vext01 As discussed. Co-authored-by: Edd Barrett <[email protected]>
9: Add dockerfile for CI. r=ltratt a=vext01 Depends on https://github.com/softdevteam/buildbot_config/pull/69 Co-authored-by: Edd Barrett <[email protected]>
10: Use dynamically linked llvm in CI. r=ltratt a=vext01 Needs ykjit/ykllvm#67 Co-authored-by: Edd Barrett <[email protected]>
11: We no longer need to build ykllvm separately. r=vext01 a=ltratt Co-authored-by: Laurence Tratt <[email protected]>
Because we were having issues with older compilers crashing.
Move to GitHub merge queues.
These scripts will help us to find bugs in large code bases such as lua. It will iteratively reduce the size of the codebase using c-vice and script oracles.
Serial compilation tests
Creduce with cvice and single lua source compilation
Set serialisation off for non-serialised compilation tests.
Remove LYK_DEBUG check
Updated documentation with yklua compilation flags and runtime environment variables.
Update readme.
Location array was reallocated for every instructions, which was very ineffective since we only need to reallocate the buffer either when yk location is set to NULL or when the number of bytecode instructions is greater than number of location in the yk location array. This was causing around 1000x slowdown in benchmark test such as verybig.lua. The fix improves verybig.lua by 300x and brings performance from 198sec to 0.650sec when JIT is off.
Reallocting location array only when bytecode size is greater than the location size.
This step only requires us to build the merged lua sources into onelua.c.
cvise puts the candidate reduced file in a temp dir and then it's up to you to test it. We were copying in the source tree, but without installing the candidate onelua.c file, so this wasn't doing what we thought it was.
|
LGTM. |
|
When I tested it, it ran for 6 hours and eventually errored out with: 05:58:48 WARNING Test timed out.
05:58:48 INFO Created extra directory cvise_extra_0000 for you to look at later
05:58:48 WARNING Test timed out.
05:58:48 INFO Created extra directory cvise_extra_0001 for you to look at later
Traceback (most recent call last):
File "/usr/bin/cvise", line 312, in <module>
reducer.reduce(pass_group, skip_initial=args.skip_initial_passes)
File "/usr/share/cvise/cvise.py", line 143, in reduce
self._run_additional_passes(pass_group['first'])
File "/usr/share/cvise/cvise.py", line 166, in _run_additional_passes
self.test_manager.run_pass(p)
File "/usr/share/cvise/utils/testing.py", line 529, in run_pass
success_env = self.run_parallel_tests()
^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/share/cvise/utils/testing.py", line 455, in run_parallel_tests
future = pool.schedule(test_env.run, timeout=self.timeout)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/lib/python3/dist-packages/pebble/pool/process.py", line 103, in schedule
self._check_pool_state()
File "/usr/lib/python3/dist-packages/pebble/pool/base_pool.py", line 95, in _check_pool_state
raise RuntimeError('Unexpected error within the Pool')
RuntimeError: Unexpected error within the PoolReducing |
|
I've seen this too. The disk was full from creating those "cvise_extra_*" directories. I don't know what they are for, but because they are created in $PWD, we end up copying them into the temp dirs of future iterations, filling the disk quite quickly. If you agree that this PR moves things in the right direction, I suggest we merge this and I can investigate what the "extra" dirs are for (and when they are created) on my return. (No squashing required if you want to merge) |
|
The log you posted also confirms that there is a default timeout (assuming you didn't specify a timeout explicitly). This is something else I was curious about. I need to experiment with shorter timeouts. Maybe 30 seconds more than build time (which IIRC is about a minute or so). |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
(For the record, if I manage to get cvise working, I will redo this stuff elsewhere, either in a separate repo, or maybe just outline the steps required in the docs) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The third commit is for correctness, the other two are just tidy ups.
I've not finished a full run yet, but having studied creduce and cvise, I think these changes should be uncontroversial.
At least it's no longer quickly reducing to an empty file.