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

Modified basic-syscall and syscall-wrappers infrastructure #78

Merged
merged 1 commit into from
Oct 6, 2024
Merged

Modified basic-syscall and syscall-wrappers infrastructure #78

merged 1 commit into from
Oct 6, 2024

Conversation

SorinAlexB
Copy link

Added checkers and modified files

Prerequisite Checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Updated relevant documentation (if needed).

Description of changes

@github-actions github-actions bot added area/tasks Update to tasks topic/software-stack Related to "Software Stack" chapter kind/new New content / item labels Sep 25, 2024
teodutu
teodutu requested changes Sep 29, 2024
View reviewed changes
Copy link

@teodutu teodutu left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Having the tests only in the solution/ folder doesn't seem to be the way to go. Students will have a hard time working in support/src, but running the checker in ../../solution/tests/. Add a command to the makefiles so that make skels also copies the tests to the support/ folder.

In addition, fix the linter failures.

chapters/software-stack/system-calls/drills/tasks/basic-syscall/solution/src/.gitignore Outdated Show resolved Hide resolved
chapters/software-stack/system-calls/drills/tasks/basic-syscall/solution/src/Makefile Outdated Show resolved Hide resolved
chapters/software-stack/system-calls/drills/tasks/basic-syscall/solution/src/arm/Makefile Outdated Show resolved Hide resolved
chapters/software-stack/system-calls/drills/tasks/basic-syscall/solution/src/arm/.gitignore Outdated Show resolved Hide resolved
chapters/software-stack/system-calls/drills/tasks/basic-syscall/solution/tests/checker.sh Outdated Show resolved Hide resolved
chapters/software-stack/system-calls/drills/tasks/syscall-wrapper/solution/tests/checker.sh Outdated Show resolved Hide resolved
chapters/software-stack/system-calls/drills/tasks/syscall-wrapper/solution/tests/checker.sh Outdated
Comment on lines 5 to 7
touch out/out1.out
touch out/out2.out
touch out/out3.out
Copy link

Choose a reason for hiding this comment

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

Why?

chapters/software-stack/system-calls/drills/tasks/syscall-wrapper/solution/tests/checker.sh
out="out/out.out"
ref="ref/out.ref"

head -n 3 "$out" > /tmp/out.txt
Copy link

Choose a reason for hiding this comment

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

Why? Just use out.out.

Copy link
Author

Choose a reason for hiding this comment

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

Students should implement the getpid syscall wrapper. I assumed it should not be tested, so the checker evaluates just the first three lines of output(read and write syscalls).

Copy link

Choose a reason for hiding this comment

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

It should be tested. Students need a way to be sure they're using the right syscall number.

chapters/software-stack/system-calls/drills/tasks/syscall-wrapper/solution/tests/checker.sh Outdated
Comment on lines 16 to 18
if [ $? -eq 0 ]; then
echo "main ...................... passed ... 10"
else
echo "main ...................... failed ... 0"
Copy link

Choose a reason for hiding this comment

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

This only tests read() and write(). To test itoa() and reverse_string(), change the former's name to os_itoa() or something and compare their outputs to itoa() and strrev(). You can test getpid() by changing its name in syscall.asm to os_getpid() and comparing its output to the real getpid().

chapters/software-stack/system-calls/drills/tasks/syscall-wrapper/solution/src/main.c Outdated
Copy link

Choose a reason for hiding this comment

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

Change the main() function to test itoa() and reverse_string() separately before testing getpid() using itoa().

@teodutu
Copy link

teodutu commented Oct 6, 2024 •
edited
Loading

Fuck me. TIL that strrev() isn't supported by GCC 🙃 [1]. So you can skip testing this separately. And you can use snprintf() as an implementation for itoa() [2].

[1] https://stackoverflow.com/a/58898320
[2] https://stackoverflow.com/a/12970536

teodutu
teodutu requested changes Oct 6, 2024
View reviewed changes
Copy link

@teodutu teodutu left a comment

Choose a reason for hiding this comment

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

Just add the make commands to the checkers and update the paths to the binaries. Regarding testing syscall-wrapper granularly, let's leave it as it is for now and I'll convert this into an issue for a later time so we can merge this now.

chapters/software-stack/system-calls/drills/tasks/basic-syscall/solution/src/hello.asm Outdated Show resolved Hide resolved
chapters/software-stack/system-calls/drills/tasks/basic-syscall/solution/src/hello.asm Outdated Show resolved Hide resolved
chapters/software-stack/system-calls/drills/tasks/basic-syscall/solution/src/hello.asm Show resolved Hide resolved
chapters/software-stack/system-calls/drills/tasks/basic-syscall/solution/tests/checker.sh Show resolved Hide resolved
chapters/software-stack/system-calls/drills/tasks/basic-syscall/solution/tests/checker.sh Outdated Show resolved Hide resolved
chapters/software-stack/system-calls/drills/tasks/syscall-wrapper/solution/tests/checker.sh Show resolved Hide resolved
chapters/software-stack/system-calls/drills/tasks/syscall-wrapper/solution/tests/checker.sh
Comment on lines +13 to +19
if cmp -s "$ref" /tmp/out.txt; then
echo "main ...................... passed ... 10"
else
echo "main ...................... failed ... 0"
fi
Copy link

Choose a reason for hiding this comment

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

You should make this test more granular: test each functionality individually, not all of them at once.

@teodutu
chapters/software-stack: Add checker infrastructure for `basic-syscal…
ebb41b9 
…l` and `syscall-wrappers`

In addition, the skeleton files are generated from the solutions by
using `make skels`.

Signed-off-by: Sorin Birchi <[email protected]>
Signed-off-by: Teodor Dutu <[email protected]>
@teodutu teodutu added the needs-rendering The PR makes changes to the website that need to be rendered label Oct 6, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 6, 2024

Published at https://cs-pub-ro.github.io/operating-systems/78/

@teodutu teodutu merged commit d16a252 into cs-pub-ro:main Oct 6, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@teodutu teodutu teodutu approved these changes

Assignees
No one assigned
Labels
area/tasks Update to tasks kind/new New content / item needs-rendering The PR makes changes to the website that need to be rendered topic/software-stack Related to "Software Stack" chapter
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@SorinAlexB @teodutu