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.

Sign up for GitHub

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

Jump to bottom

Creduce with cvice and single lua source compilation #77

Merged
merged 1 commit into from
Nov 15, 2023
Merged

Creduce with cvice and single lua source compilation #77

merged 1 commit into from
Nov 15, 2023

Conversation

Pavel-Durov
Copy link
Contributor

Add cvice scripts and onelua compilation support.

ltratt
ltratt reviewed Nov 9, 2023
View reviewed changes
cvise_reduce.sh Outdated
set -eu

# UPDATE: YKLUA path
YKLUA="/home/pd/yklua-fork-cvise"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this path will work for the rest of us ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's why I added instructions in README :)
But I couldn't think of a better solution.
Alternatively, we can set some kind of global config to run cvise_reduce.sh. Environment variables set when running cvise don't seem to be available in 'cvise_reduce.sh`.

@ltratt
Copy link
Contributor

ltratt commented Nov 9, 2023

I'm not totally certain if having this in the repo is a good idea, but let's go with it and see what we think after a while. At a minimum, we should put this in a directory called something like creduce so that the changes are a) localised to that directory b) don't show up as conflicts/changes if we diff this repo against "real" Lua. Does that make sense?

@Pavel-Durov
Copy link
Contributor Author

ok, makes sense.

@Pavel-Durov
Copy link
Contributor Author

@ltratt updated 👉 47e8902

ltratt
ltratt reviewed Nov 10, 2023
View reviewed changes
README.md Outdated
@@ -48,6 +48,23 @@ LYK_VERBOSE=1 ../src/lua all.lua
LYK_VERBOSE=1 sh ./test.sh
```

## C-vise
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should put this part of the README into a new file creduce/README, and then instead of "pd" filenames we can use relative filenames e.g. "../src/lua" and similar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated 👉 0940e4b
Some paths need to be absolute (added comment with description) so no much use for "../"

ltratt
ltratt reviewed Nov 10, 2023
View reviewed changes
creduce/cvise_reduce.sh Outdated
@@ -0,0 +1,19 @@
#!/bin/bash
Copy link
Contributor

Choose a reason for hiding this comment

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

#! /bin/sh unless we have to use bash (which I don't think we do in this file).

I also wondered if this should be named `cvis.example.sh" or simukar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated 👉 0940e4b

@ltratt
Copy link
Contributor

ltratt commented Nov 12, 2023

I think we just need one of creduce/cvise.sh and creduce/cvise.example.sh?

@Pavel-Durov
Copy link
Contributor Author

We need to build onelua.c source code first, only then we can run: cvise ./cvise.example.sh onelua.c, otherwise cvise exit with error:

Traceback (most recent call last):
  File "/usr/bin/cvise", line 299, in <module>
    test_manager = testing.TestManager(pass_statistic, args.interestingness_test, args.timeout,
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/share/cvise/utils/testing.py", line 158, in __init__
    self.check_file_permissions(test_case, [os.F_OK, os.R_OK, os.W_OK], InvalidTestCaseError)
  File "/usr/share/cvise/utils/testing.py", line 233, in check_file_permissions
    raise error(path, m)
cvise.utils.error.InvalidTestCaseError: The specified test case 'onelua.c' cannot be accessed!

@ltratt
Copy link
Contributor

ltratt commented Nov 12, 2023

Ah, I see the confusion. Why don't we provide build_one_lua.sh (or a Makefile target) which just builds one.lua and then cvise.example.sh as the script that cvise itself runs?

@Pavel-Durov
Copy link
Contributor Author

ok

@Pavel-Durov
Copy link
Contributor Author

done 👉 32c6464

@ltratt
Copy link
Contributor

ltratt commented Nov 12, 2023

This looks good to me -- @nmdis1999 can you also have a look please?

@nmdis1999
Copy link
Contributor

This looks good to me -- @nmdis1999 can you also have a look please?

Sure. Added a comment.

@Pavel-Durov
Copy link
Contributor Author

@nmdis1999 Where is your comment? I can't find it 😶

nmdis1999
nmdis1999 reviewed Nov 13, 2023
View reviewed changes
creduce/one.c Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need one.c again here? Can we not use one.c from src to build onelua.c in creduce directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

one.c was originally in the lua src directory but we wanted to isolate everything related to cvice and onelua in the creduce directory so that we won't have a big diff when comparing with original source code.

see @ltratt comment 👉 #77 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

From what I can see, currently one.c in both src and creduce folder are the same file and is different from one.c in lua's src directory by a single line (for now at least). My question is in what case will one.c in yklua's src might be different from one we end up using in the creduce directory? My understanding is, for creduce we care only about onelua.c, which you'd get when you run make on one.c, and for make to be successful, it has to be run in the directory where we have all the headers (please, correct me if I missed anything). So, when would you need one.c in creduce?

I am not convinced we need a copy of one.c in creduce directory too yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, it's just a "left-over" I will remove one.c from src directory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, we can do it either way, but I think that keeping onc.c in creduce directory is cleaner cause we don't "pollute" the original lua src code (same goes for Makefile changes)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed one.c form src here 👉 8334f4b

@ltratt
Copy link
Contributor

ltratt commented Nov 13, 2023

Please squash.

@Pavel-Durov
Add c-vice scripts and single lua source file compilation.
92f4645 
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.
@Pavel-Durov
Copy link
Contributor Author

squashed 👉 92f4645

@Pavel-Durov Pavel-Durov changed the title Add cvice scripts and onelua compilation support Creduce with cvice and single lua source compilation Nov 14, 2023
@ltratt ltratt added this pull request to the merge queue Nov 15, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 15, 2023
@Pavel-Durov
Copy link
Contributor Author

CI failed :(
I think we need to merge first this PR #79

@ltratt ltratt added this pull request to the merge queue Nov 15, 2023
Merged via the queue into ykjit:main with commit e4947b9 Nov 15, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nmdis1999 nmdis1999 nmdis1999 left review comments

@ltratt ltratt ltratt left review comments

Assignees

@ltratt ltratt

@nmdis1999 nmdis1999

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Pavel-Durov @ltratt @nmdis1999