This is an archive of the discontinued LLVM Phabricator instance.

[AddressSanitizer] Add support for (constant-)masked loads and stores.
ClosedPublic

Authored by filcab on Nov 1 2016, 7:36 PM.

Details

Summary

This patch adds support for instrumenting masked loads and stores under
ASan, if they have a constant mask.

isInterestingMemoryAccess now supports returning a mask to be applied to
the loads, and instrumentMop will use it to generate additional checks.

Added tests for v4i32 v8i32, and v4p0i32 (~v4i64) for both loads and
stores (as well as a test to verify we don't add checks to non-constant
masks).

Diff Detail

Repository
rL LLVM

Event Timeline

filcab updated this revision to Diff 76669.Nov 1 2016, 7:36 PM
filcab retitled this revision from to [AddressSanitizer] Add support for (constant-)masked loads and stores..
filcab updated this object.
filcab added reviewers: RKSimon, dvyukov, kcc, zaks.anna.
filcab added a subscriber: llvm-commits.
RKSimon added inline comments.Nov 8 2016, 8:38 AM
lib/Transforms/Instrumentation/AddressSanitizer.cpp
505 ↗(On Diff #76669)

Add MaybeMask description to comment.

996 ↗(On Diff #76669)

Add MaybeMask description to comment.

kcc edited edge metadata.Nov 8 2016, 10:05 AM

Could you please also add a test to compiler-rt/test/asan?
(you can do it as a separate Phab patch, if that's more convenient)

lib/Transforms/Instrumentation/AddressSanitizer.cpp
1183 ↗(On Diff #76669)

could you please outline this into its own function, just as you did with doInstrumentAddress?

filcab updated this revision to Diff 77487.Nov 10 2016, 7:56 AM
filcab edited edge metadata.

Update CreateGEP call to add 0 as the first parameter.

filcab planned changes to this revision.Nov 10 2016, 8:06 AM

Forgot to outline part of a function.

filcab updated this revision to Diff 77491.Nov 10 2016, 8:23 AM

Extract instrumentMaskedLoadOrStore()

RKSimon added inline comments.Nov 11 2016, 11:06 AM
lib/Transforms/Instrumentation/AddressSanitizer.cpp
1055 ↗(On Diff #77491)

Use isa<> instead as we don't use the return value from dyn_cast<> ?

1059 ↗(On Diff #77491)

I don't think we can guarantee ConstantInt type here (UndefValue etc.?)

1152 ↗(On Diff #77491)

I don't think we can guarantee ConstantInt type here (UndefValue etc.?)

filcab updated this revision to Diff 77722.Nov 12 2016, 6:46 AM
filcab marked 6 inline comments as done.

Addressed rksimon's comments:
Basically try to discard undef in a safe way.

Brought back (fixed) test.

vitalybuka accepted this revision.Nov 14 2016, 12:48 PM
vitalybuka edited edge metadata.

LGTM

lib/Transforms/Instrumentation/AddressSanitizer.cpp
1064 ↗(On Diff #77722)

You don't call this with nullptr, so for consistency with other output arguments i'd remove "if (MaybeMask)" check

This revision is now accepted and ready to land.Nov 14 2016, 12:48 PM
filcab marked an inline comment as done.Nov 15 2016, 1:47 PM
filcab added inline comments.
lib/Transforms/Instrumentation/AddressSanitizer.cpp
1064 ↗(On Diff #77722)

It's called in many places outside the function I changed, though.
In the future we will need to refactor this to support scatter/gathers, so I'd like to avoid doing too much now.

996 ↗(On Diff #76669)

Deleted this one, since there's no reason to have those comments on both the prototype and impl.

This revision was automatically updated to reflect the committed changes.
filcab marked an inline comment as done.