Stream: git-wasmtime

Topic: wasmtime / PR #3793 Make VMOffset calculation more readable

Wasmtime GitHub notifications bot (Feb 11 2022 at 19:18):

bjorn3 opened PR #3793 from vmoffset_cleanup to main:

This should reduce the risk of making a mistake when changing it. Also fixes a typo and moves builtin_functions earlier in the VMContext to make it's offset known at compile time.

Wasmtime GitHub notifications bot (Feb 11 2022 at 20:12):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Feb 11 2022 at 20:12):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Feb 11 2022 at 20:12):

alexcrichton created PR review comment:

This comment I think is fine to drop because it basically references something not existent in the code after this PR.

Wasmtime GitHub notifications bot (Feb 11 2022 at 20:12):

alexcrichton created PR review comment:

Personally I find this macro pretty hard to read and the syntax feels pretty odd. Could this perhaps be improved with something like:

fields! {
    size(interrupts) = ret.ptr.size(),
    size(epoch_ptr) = ret.ptr.size(),
    // ...
}

I think it might be helpful for the macro to automatically insert u32::from everywhere to avoid having to explicitly call it as well. Furthermore I think it's best to avoid cmul and cadd aliases and instead just have the .checked_mul inline since at least for me I think that's easier to read.

Wasmtime GitHub notifications bot (Feb 11 2022 at 21:37):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Feb 11 2022 at 21:37):

bjorn3 created PR review comment:

Personally I find this macro pretty hard to read and the syntax feels pretty odd.

I guess so. I like your suggestion.

Furthermore I think it's best to avoid cmul and cadd aliases and instead just have the .checked_mul inline since at least for me I think that's easier to read.

a.checked_mul(b).unwrap() is harder to read than cmul(a, b) IMHO. Especially as the former often requires wrapping to stay below 100 characters. Also as noted in the comment above their definition it increases binary size a bit due to unique caller locations passed to .unwrap(). If you aren't convinced I will drop it.

Wasmtime GitHub notifications bot (Feb 12 2022 at 13:47):

bjorn3 updated PR #3793 from vmoffset_cleanup to main.

Wasmtime GitHub notifications bot (Feb 12 2022 at 13:48):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Feb 12 2022 at 13:48):

bjorn3 created PR review comment:

Personally I find this macro pretty hard to read and the syntax feels pretty odd. Could this perhaps be improved with something like:

How should align be handled? defined_globals needs to be aligned to 16 bytes.

Wasmtime GitHub notifications bot (Feb 12 2022 at 13:48):

bjorn3 edited PR review comment.

Wasmtime GitHub notifications bot (Feb 14 2022 at 15:47):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2022 at 15:47):

alexcrichton created PR review comment:

I'm personally not a huge fan of one-off functions with short names like "cmul" purely to save some characters on a few lines. Others may feel differently.

For alignment I would naively think that align(the_field) = align could be a macro argument which simply aligns up the current offset and implicitly the alignment is always specified after the size. (or before the size? something like that)

Wasmtime GitHub notifications bot (Feb 14 2022 at 15:59):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2022 at 15:59):

bjorn3 created PR review comment:

Maybe something like

fields! {
    size(interrupts) = ret.ptr.size(),
    size(epoch_ptr) = ret.ptr.size(),
    // ...
    align(16),
    size(defined_globals) = ...,
    // ...
}

Wasmtime GitHub notifications bot (Feb 14 2022 at 16:43):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Feb 14 2022 at 16:43):

alexcrichton created PR review comment:

That seems reasonable too, yeah.

Wasmtime GitHub notifications bot (Feb 18 2022 at 18:10):

bjorn3 updated PR #3793 from vmoffset_cleanup to main.

Wasmtime GitHub notifications bot (Feb 18 2022 at 18:11):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Feb 18 2022 at 18:11):

bjorn3 created PR review comment:

Pushed the new macro syntax.

Wasmtime GitHub notifications bot (Feb 22 2022 at 15:48):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Feb 22 2022 at 15:48):

alexcrichton merged PR #3793.

Last updated: Dec 23 2024 at 12:05 UTC