trace: incorporate events #4456
Conversation
|
Ah, I forgot I rebased this on top of #4435. Just the last commit is relevant for now |
nia-e
force-pushed
the
trace-incorporate-events
branch
2 times, most recently
from
acbf7f3 to
a8694a4
Compare
RalfJung
added
the
S-blocked
nia-e
force-pushed
the
trace-incorporate-events
branch
5 times, most recently
from
4326ab5 to
180efe5
Compare
|
What this PR definitely needs is some tests. :) Remember that tracing mode is off-by-default; none of the tests hit the tracing infra. So this needs some new native-mode tests that specifically check that we detect the ranges properly. For instance, sharing a 2-field uninit struct with C, having C initialize the first field, and then reading the 2nd field -- that should be UB. |
|
I actually wrote some! Just didn't know if they belong here. I'll add them, though |
nia-e
force-pushed
the
trace-incorporate-events
branch
5 times, most recently
from
6d91006 to
3d16495
Compare
interpret/allocation: expose init + write_wildcards on a range Part of rust-lang/miri#4456, so that we can mark down when a foreign access to our memory happened. Should this also move `prepare_for_native_access()` itself into Miri, given that everything there can be implemented on Miri's side? r? `@RalfJung`
interpret/allocation: expose init + write_wildcards on a range Part of rust-lang/miri#4456, so that we can mark down when a foreign access to our memory happened. Should this also move `prepare_for_native_access()` itself into Miri, given that everything there can be implemented on Miri's side? r? `@RalfJung`
interpret/allocation: expose init + write_wildcards on a range Part of rust-lang/miri#4456, so that we can mark down when a foreign access to our memory happened. Should this also move `prepare_for_native_access()` itself into Miri, given that everything there can be implemented on Miri's side? r? ``@RalfJung``
interpret/allocation: expose init + write_wildcards on a range Part of rust-lang/miri#4456, so that we can mark down when a foreign access to our memory happened. Should this also move `prepare_for_native_access()` itself into Miri, given that everything there can be implemented on Miri's side? r? ```@RalfJung```
interpret/allocation: expose init + write_wildcards on a range Part of rust-lang/miri#4456, so that we can mark down when a foreign access to our memory happened. Should this also move `prepare_for_native_access()` itself into Miri, given that everything there can be implemented on Miri's side? r? ````@RalfJung````
interpret/allocation: expose init + write_wildcards on a range Part of rust-lang/miri#4456, so that we can mark down when a foreign access to our memory happened. Should this also move `prepare_for_native_access()` itself into Miri, given that everything there can be implemented on Miri's side? r? `````@RalfJung`````
Rollup merge of #143634 - nia-e:init-and-wildcards, r=RalfJung interpret/allocation: expose init + write_wildcards on a range Part of rust-lang/miri#4456, so that we can mark down when a foreign access to our memory happened. Should this also move `prepare_for_native_access()` itself into Miri, given that everything there can be implemented on Miri's side? r? `````@RalfJung`````
interpret/allocation: expose init + write_wildcards on a range Part of #4456, so that we can mark down when a foreign access to our memory happened. Should this also move `prepare_for_native_access()` itself into Miri, given that everything there can be implemented on Miri's side? r? `````@RalfJung`````
nia-e
force-pushed
the
trace-incorporate-events
branch
from
5283a29 to
6454be8
Compare
|
I did readd logic (not much code) for whether an access spands multiple allocations since from what I understand this is theoretically allowed by LLVM at least and I'm a little scared it could happen in the wild; but I can replace that with just an assertion that accesses span a single allocation and ICE/error otherwise. @rustbot ready |
rustbot
added
the
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
nia-e
force-pushed
the
trace-incorporate-events
branch
from
8bfa229 to
8ed4303
Compare
|
@rustbot ready |
rustbot
added
S-waiting-on-review
src/shims/native_lib/mod.rs
Outdated
| // The start of the overlap range will be the greater of the alloc base address and the | ||
| // lowest remaining address in the access' range; the end will be the lesser of the end | ||
| // of the allocation and the end of the access' range. Both are then shifted by `alloc_addr` | ||
| // The start of the overlap range will be `curr`; the end will be the lesser of the end of |
There was a problem hiding this comment.
Please make it clear that this range is in terms of offsets inside the allocation, i.e., relative to alloc_addr.
|
This looks great, thanks! After resolving the last nits, please squash the commits using @rustbot author |
rustbot
added
S-waiting-on-author
nia-e
force-pushed
the
trace-incorporate-events
branch
from
8ed4303 to
377c515
Compare
|
@rustbot ready |
rustbot
added
S-waiting-on-review
|
Lol that commit message is something.^^ When you squash and it asks you for the commit message, please take care to write a reasonable commit message. I've cleaned it up for you in this PR, and also did some minor tweaks. |
RalfJung
force-pushed
the
trace-incorporate-events
branch
from
377c515 to
2fd5605
Compare
|
oops! normally I just mark everything as fixups and leave the top commit message, didn't realise |
Head branch was pushed to by a user without write access
|
Good thing that unwrap was added, the logic was off by 1 >< this should fix it, I'll --fixup if it passes CI |
|
That's exactly why Result should not be ignored. ;)
Please update the comment as well.
|
nia-e
force-pushed
the
trace-incorporate-events
branch
from
2f7343f to
b2adefb
Compare
|
should be good; ci seemed to pass, just squashed and updated the comment |
If tracing is enabled, use the info from the trace to actually update Miri's state. Pending a small change to rustc to expose some necessary interfaces to handle writes, this should be good!