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

refactor(snapshot.rs): use const generics #2069

Merged
merged 7 commits into from
Apr 24, 2024

Conversation

vringar
Copy link
Contributor

@vringar vringar commented Apr 17, 2024

Just a simple reduction in the amount of code that needs to be maintained.

@rmalmain
Copy link
Collaborator

Thank you for the PR. It looks good to me.
I'll merge it when it passes CI tests.

@vringar
Copy link
Contributor Author

vringar commented Apr 17, 2024

Two questions:

  • Should I keep submitting these small PRs or should I build one "misc improvements" PR
  • Is there any documentation around hooks? Or are they a QEMU concept and I should look through their docs for any help on that topic?

@vringar
Copy link
Contributor Author

vringar commented Apr 17, 2024

I just realized this would also work for libafl_qemu/src/asan.rs but that's more than a quick drive-by PR, so I'm not going to do that as part of this PR.

@rmalmain
Copy link
Collaborator

  • Both are good, feel free to submit them in the way that is most convenient for you. If you can avoid extreme cases (spam of 2 lines PRs that could easily be condensed or 1 PR of 10K LoC with completely unrelated changes) it is ofc better, but we are always happy to receive help.
  • Hooks come from libafl_qemu; it is an abstraction above QEMU. The hook usage (and libafl_qemu stuff in general) is poorly documented for now. Any help on this side would be greatly appreciated, in case someone would like to lend a hand. If you need help to use them, I'm open to discussion.

@rmalmain
Copy link
Collaborator

I just realized this would also work for libafl_qemu/src/asan.rs but that's more than a quick drive-by PR, so I'm not going to do that as part of this PR.

Sounds good.

This PR should pass the CI after running cargo fmt with the nightly version of rustc. Do you mind running cargo fmt on your side?

@tokatoka
Copy link
Member

ci will never pass for a couple of days. there's a bug in rust compiler

@vringar
Copy link
Contributor Author

vringar commented Apr 17, 2024

This PR should pass the CI after running cargo fmt with the nightly version of rustc. Do you mind running cargo fmt on your side?

Hey, I just ran cargo fmt in the root dir as well as the libafl_qemu dir with rustfmt 1.7.0-nightly (1cec373 2024-04-16) and nothing changed. The diff is in a completely unrelated part of the codebase

Should I fix it anyway?

@rmalmain
Copy link
Collaborator

This PR should pass the CI after running cargo fmt with the nightly version of rustc. Do you mind running cargo fmt on your side?

Hey, I just ran cargo fmt in the root dir as well as the libafl_qemu dir with rustfmt 1.7.0-nightly (1cec373 2024-04-16) and nothing changed. The diff is in a completely unrelated part of the codebase

Should I fix it anyway?

It seems like the nightly compiler is broken atm, as @tokatoka said. So, I guess the best we can do for now is to wait a few days and re-run cargo fmt / the CI job.

@vringar
Copy link
Contributor Author

vringar commented Apr 17, 2024

Hooks come from libafl_qemu; it is an abstraction above QEMU. The hook usage (and libafl_qemu stuff in general) is poorly documented for now. Any help on this side would be greatly appreciated, in case someone would like to lend a hand. If you need help to use them, I'm open to discussion.

I really appreciate the offer. I will poke around a little bit more and try to write some doc comments on the functions with my current understanding and open a PR. Maybe that can then serve as a good place for that discussion.

@rmalmain rmalmain self-assigned this Apr 18, 2024
@vringar
Copy link
Contributor Author

vringar commented Apr 21, 2024

@rmalmain I also attempted to capture my current understanding of the hooks in 81f508e but I'm not sure if incomplete documentation is worse than no docs and if my understanding is correct.
Should I keep this in a separate PR?

@domenukk domenukk requested a review from rmalmain April 22, 2024 08:39
@rmalmain
Copy link
Collaborator

It looks good to me. It doesn't necessarily need to be in a separate PR; it's all fine like this. Even if you don't fully understand a function, as long as what you write is accurate, there is no problem. A (correct) partial doc is much better than nothing at all.

A few details:

  • It's better to keep the comment before the attribute (for consistency with the rest of the code)
  • Ideally, it would be very nice to have a short comment about the non-trivial parameters of the function (like gen in that case).

Thanks for helping with the documentation!

@vringar
Copy link
Contributor Author

vringar commented Apr 23, 2024

@rmalmain I commented on gen to the best of my ability, however I don't really understand what the TCGTemp struct actually does and why we are passing an id from the gen to the exec functions, so my comment focusses more on what is happening rather than the why.

Thanks for helping with the documentation!

I'm being very selfish here as I'll have to use these APIs for my thesis, so any documentation that's signed off by you is helping me do my thing :)

@vringar vringar force-pushed the refactor/const_generics branch from 28eb59a to 224a3f9 Compare April 23, 2024 16:35
@rmalmain
Copy link
Collaborator

rmalmain commented Apr 24, 2024

The comment looks good, thanks.

however I don't really understand what the TCGTemp struct actually does and why we are passing an id from the gen to the exec functions, so my comment focusses more on what is happening rather than the why.

  • TCGTemp is a structure internal to QEMU. To make it short, it's an internal variable holding the address at which the read will happen in that context. TCTemp is a "virtual" register whose exact value is (a priori) unknown at generation time. So, the only thing you can know is in which register it will be stored at runtime

  • The ID parameter is a way to get information from the gen hook (run during the translation block's generation) to the actual read hook that will be executed each time the translation block containing the read is run. Since both hooks are decorrelated, you need a way to know which gen hook generated the read hook itself. For now, the ID is used to get the PC at which the read hook is executed, but you could use other kinds of IDs. It is also a way for the gen hook to determine whether the read hook should be generated or not.

I'll merge this for now, if you want to add more documentation feel free to open a new PR

@rmalmain rmalmain merged commit 0f42efa into AFLplusplus:main Apr 24, 2024
102 checks passed
@vringar vringar deleted the refactor/const_generics branch April 24, 2024 10:00
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.

4 participants