This is an archive of the discontinued LLVM Phabricator instance.

[MSan] Add an isStore argument to getShadowOriginPtr(). NFC
ClosedPublic

Authored by glider on Mar 14 2018, 8:40 AM.

Details

Summary

This is a step towards the upcoming KMSAN implementation patch.
The is isStore argument is to be used by getShadowOriginPtrKernel(), it is ignored by getShadowOriginPtrUserspace().

Depending on whether a memory access is a load or a store, KMSAN instruments it with different functions, msan_metadata_ptr_for_load_X() and msan_metadata_ptr_for_store_X().
Those functions may return different values for a single address, which is necessary in the case the runtime library decides to ignore particular accesses.

Diff Detail

Repository
rL LLVM

Event Timeline

glider created this revision.Mar 14 2018, 8:40 AM
glider added a subscriber: Restricted Project.Mar 15 2018, 3:57 AM
eugenis accepted this revision.Mar 16 2018, 4:12 PM

Are you going to provide a zero page for ignores loads and a scratch page for ignored stores? Is it valid to use an isStore pointer for loading? If not, then load-modify-store operations will have to request two pointers and expect them to be different?

This revision is now accepted and ready to land.Mar 16 2018, 4:12 PM

Are you going to provide a zero page for ignores loads and a scratch page for ignored stores?

Yes, that is the intention.

Is it valid to use an isStore pointer for loading?

In general it is not.

If not, then load-modify-store operations will have to request two pointers and expect them to be different?

Good point.
Note that we don't instrument the atomic RMW operations in (K)MSan properly anyway, so this isn't a problem for atomic CAS/RMW.
For a nonatomic sequence of load/modify/store instructions we might be losing some optimization opportunities here, but I can't think of any. (E.g. in the case when we're overwriting the old shadow value, and there's no point in reading it, we shouldn't do it irrespective of whether the read and write shadow pointers match)

glider accepted this revision.Mar 26 2018, 2:07 AM

To reiterate, your point is valid, but I don't think it's going to affect us.

Yes, this sounds fine.
You may want to provide a read-only page for reads and a write-only page for write to validate the use of this interface.

glider closed this revision.Mar 28 2018, 3:25 AM

Committed r328692, thanks!