[PATCH] asan: do not instrument direct inbounds accesses to stack variables

[Dmitry Vyukov]

On Tue, Feb 24, 2015 at 10:03 PM, Anna Zaks <zaks.anna at gmail.com> wrote:
> How are you getting the speedup/size improvement measurements? Are these at -O1 or -O0?
>
> When comparing to my patch, the intend of this patch is to be more aggressive in removing bounds checking. My patch does not introduce any improvement at optimization levels higher than -O0 and is trying to simulate mem2reg. On the other hand, the analysis here are more aggressive in the checks it removes. For example, my patch would not remove provably in bounds array accesses with constant offset and it does not do anything for non-alloca values.
>
> On the other hand, with my patch, the allocas that are known not to have instrumented accesses do not get poisoned. Currently, this patch is only targeting removal of bounds checking. Also, I am not sure what is the compile time overhead this brings and how reliable it is since I don't think ObjectSizeOffsetEvaluator is used much.
>
> I preference is to have this on top of non-promotable allocas.
>
>
> ================
> Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:18
> @@ -16,1 +17,3 @@
> +#include "llvm/Analysis/TargetLibraryInfo.h"
> +#include "llvm/Analysis/ValueTracking.h"
>  #include "llvm/Transforms/Instrumentation.h"
> ----------------
> These seem out of place - should be moved after ADT.

Done

> ================
> Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:189
> @@ -184,1 +188,3 @@
> +       cl::desc("Don't instrument scalar stack variables"), cl::Hidden,
> +       cl::init(true));
>
> ----------------
> This makes me nervous. I don't think ObjectSizeOffsetEvaluator is used much. This should probably go through more testing, though I am not sure how to catch issues here since we are removing checking.

Turned off by default.


> ================
> Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:408
> @@ -398,2 +407,3 @@
>    bool GlobalIsLinkerInitialized(GlobalVariable *G);
> +  bool isInboundsAccess(ObjectSizeOffsetEvaluator &ObjSizeEval, Value *Addr);
>
> ----------------
> const?

Done

> ================
> Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:1502
> @@ +1501,3 @@
> +      getTLI();
> +  ObjectSizeOffsetEvaluator ObjSizeEval(DL, TLI, F.getContext(), true);
> +
> ----------------
> /*RoundToAlign=*/true

Done

> ================
> Comment at: lib/Transforms/Instrumentation/AddressSanitizer.cpp:2058
> @@ +2057,3 @@
> +  int64_t Offset = dyn_cast<ConstantInt>(SizeOffset.second)->getSExtValue();
> +  return Offset >= 0 && Offset + TypeSize / 8 <= Size;
> +}
> ----------------
> Why not use getTypeStoreSize()?
>   uint64_t getTypeStoreSizeInBits(Type *Ty) const {
>     return 8 * getTypeStoreSize(Ty);
>   }

Done

> Also, is overflow possible in these calculations?


These are int64's. I don't see how overflow can happen.