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

feat(svm): initial sbfv1 virtual machine #478

Merged
merged 19 commits into from
Jan 15, 2025
Merged

feat(svm): initial sbfv1 virtual machine #478

merged 19 commits into from
Jan 15, 2025

Conversation

Rexicon226
Copy link
Contributor

@Rexicon226 Rexicon226 commented Jan 3, 2025

Merge TODO checklist:

  • Port over the ELF binary testing stuff
  • Port over the benchmarks (there's some V2 stuff I want to benchmark as well, excluding from this PR)
  • Go over the code again and clean it up a bit more

This does not include SBFv2 support, as it's not ready yet, and we don't need it to start work on the program runtime.

You'll also notice I'm using Zig's file-as-struct feature in some places. The debate for whether it should be used was before my time, however I do want to advocate for it again and take this time to show how great it can be in practice. If we don't like it, I'll change it back to our current style guide.

@InKryption
Copy link
Contributor

I would like to cast my vote as being in favor of files-as-structs.

@0xNineteen 0xNineteen marked this pull request as draft January 6, 2025 11:01
@dnut
Copy link
Contributor

dnut commented Jan 6, 2025

I would like to cast my vote as being in favor of files-as-structs.

Personally I am indifferent. I see pros and cons. I'm happy to use them or to banish them, I don't think it matters very much. But I'll play devil's advocate for a moment to provide some additional perspective.

The argument against is that they are hard to understand for people who are not familiar with Zig. The syntax is esoteric and specific to only this programming language. Let's say a developer who has never worked with Zig tries to understand our codebase. They would probably not understand that the file is a struct. They might totally miss the struct fields, or have no idea what they mean. This has a negative impact on the readability and accessibility of our codebase to outsiders.

I get that each language has unique features that people would not understand without specific knowledge of that language. I'm not arguing that you should avoid using all of them just for the sake of universal readability. The question is whether the benefit justifies the cost. Having no idea that a file is a struct would have a significant impact on the intelligibility of the entire file. If the only advantage of using the file as a struct is to reduce one level of indentation, that's a pretty small benefit.

Would you grant a small benefit to the majority if it causes a big problem for a minority?

Copy link
Contributor

@yewman yewman left a comment

Choose a reason for hiding this comment

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

A couple of minor comments / queries but otherwise looks good to me!

src/svm/Elf.zig Outdated Show resolved Hide resolved
src/svm/Elf.zig Outdated Show resolved Hide resolved
src/svm/Executable.zig Outdated Show resolved Hide resolved
src/svm/Executable.zig Outdated Show resolved Hide resolved
src/svm/ebpf.zig Outdated Show resolved Hide resolved
src/svm/ebpf.zig Outdated Show resolved Hide resolved
@Rexicon226 Rexicon226 marked this pull request as ready for review January 7, 2025 02:39
@dnut dnut self-requested a review January 7, 2025 15:02
@yewman yewman self-requested a review January 7, 2025 16:20
Copy link
Contributor Author

Rexicon226 commented Jan 7, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

Copy link
Contributor

@0xNineteen 0xNineteen left a comment

Choose a reason for hiding this comment

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

overall looks great - just few notes/questions

src/svm/Elf.zig Outdated Show resolved Hide resolved
src/svm/Elf.zig Outdated Show resolved Hide resolved
src/svm/Elf.zig Outdated Show resolved Hide resolved
src/svm/Elf.zig Outdated Show resolved Hide resolved
src/svm/Elf.zig Outdated Show resolved Hide resolved
src/svm/Elf.zig Outdated Show resolved Hide resolved
src/svm/Vm.zig Outdated Show resolved Hide resolved
src/svm/Executable.zig Outdated Show resolved Hide resolved
src/svm/Executable.zig Outdated Show resolved Hide resolved
src/svm/syscalls.zig Show resolved Hide resolved
0xNineteen
0xNineteen previously approved these changes Jan 7, 2025
Copy link
Contributor

@0xNineteen 0xNineteen left a comment

Choose a reason for hiding this comment

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

:shipit:

src/svm/syscalls.zig Show resolved Hide resolved
src/svm/Vm.zig Outdated Show resolved Hide resolved
src/svm/ebpf.zig Outdated Show resolved Hide resolved
src/svm/ebpf.zig Outdated Show resolved Hide resolved
src/svm/ebpf.zig Outdated Show resolved Hide resolved
src/svm/Elf.zig Outdated Show resolved Hide resolved
src/svm/Elf.zig Outdated Show resolved Hide resolved
src/svm/Elf.zig Outdated Show resolved Hide resolved
src/svm/Elf.zig Outdated Show resolved Hide resolved
src/svm/Elf.zig Outdated Show resolved Hide resolved
yewman
yewman previously requested changes Jan 14, 2025
Copy link
Contributor

@yewman yewman left a comment

Choose a reason for hiding this comment

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

Lets change the actual file names to lower case as well, otherwise looks good to me.

Copy link
Contributor

@dnut dnut 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 we can merge this once it conforms to the self pattern that was decided yesterday. Currently this pr is using a couple different patterns.

@Rexicon226
Copy link
Contributor Author

I believe I've got the last of the usages here: d334218
Please let me know if you spot any others.

@dnut dnut enabled auto-merge (squash) January 15, 2025 16:55
@dnut dnut merged commit 7b961e8 into main Jan 15, 2025
8 checks passed
@dnut dnut deleted the Rexicon226/svm branch January 15, 2025 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

6 participants