[llvm] r358919 - [InstCombine] Eliminate stores to constant memory

Sat Apr 27 03:18:41 PDT 2019

On Sat, Apr 27, 2019 at 1:33 AM Philip Reames via llvm-commits
<llvm-commits at lists.llvm.org> wrote:
>
> This doesn't feel like a case where we should change clang's behavior to be more conservative by default.  Maybe adding a "const sanitizer" or an -fallow-const-misuse might be justified, but I'm not sure I'd go any further that that.
>
> If we did want to sanitize for such inputs, replacing the instcombine rule I added with an assertion of the same assumption would be pretty straight-forward.  I haven't worked on the sanatizers, anyone know if there's a natural place to fit such a check?
I have already thought about that,
and //i'm// not sure how it would be possible reasonably to sanitize this.

The casting const-ness away is not a bug.
Writing into such a pointer, with casted-away const-ness is not a bug yet.
The bug only happens when the original object was const.

So to diagnose, you would need to be *sure* that the write happens
into a constant object.
While you may be able to catch some cases with traditional clang
Codegen-time sanitization,
there will be trivial false-negatives, e.g. when the casting happens
in one function,
and then the casted pointer passed into another function, which actually writes.

Thus, unless the check can be inserted by this very LLVM pass
(and i'm not sure there's precedent for that.), i'm not sure how this
can be done.

The alternative solution is of course to sanitize every write,
and at run-time check if we are writing into a const object.
But current ASAN does not seem to track/know whether some memory is
constant or not.

> Philip
Roman

> On 4/25/19 12:58 PM, Eli Friedman wrote:
>
> We had a bug reported about busybox getting miscompiled due to the incorrect use of “const” before; see https://bugs.llvm.org/show_bug.cgi?id=39919 .  In that case, we weren’t removing the store; we just rescheduled it after a load from the same variable.  The comments there indicate a bug was reported against busybox, but I don’t know what happened after that.
>
>
>
> -Eli
>
>
>
> From: llvm-commits <llvm-commits-bounces at lists.llvm.org> On Behalf Of Chandler Carruth via llvm-commits
> Sent: Thursday, April 25, 2019 10:28 AM
> To: Philip Reames <listmail at philipreames.com>; Richard Smith <richard at metafoo.co.uk>
> Cc: llvm-commits <llvm-commits at lists.llvm.org>
> Subject: [EXT] Re: [llvm] r358919 - [InstCombine] Eliminate stores to constant memory
>
>
>
> I wanted to write this so people were aware this patch will break code that exists in the wild.
>
>
>
> Consider code like this from BusyBox:
>
> https://github.com/bigswitch/iods/blob/master/indigo-lb9a/busybox-1.13.3/shell/ash.c#L216
>
> extern struct globals_misc *const ash_ptr_to_globals_misc;
>
>
> The code goes on to cast away the const and store to this global:
>
> https://github.com/bigswitch/iods/blob/master/indigo-lb9a/busybox-1.13.3/shell/ash.c#L241
>
>
>
> I believe your patch is entirely correct. I believe Clang is correct to mark this memory as constant. I just wanted people to be aware that code in the wild doesn't abide by these rules.
>
>
>
> If we want to do something about it, I believe we should change Clang to not mark *extern* const globals as constant memory. +Richard Smith for confirmation on that approach. Maybe behind a flag or only in certain modes.
>
>
>
> So far, this is the only example I have found however, and so it may be sufficiently rare to not warrant doing anything about it. Sending this email in case others are debugging similar issues, hit this revision, and add information that indicates it is not as rare as I am hoping.
>
>
>
>
>
> On Mon, Apr 22, 2019 at 1:26 PM Philip Reames via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>
> Author: reames
> Date: Mon Apr 22 13:28:19 2019
> New Revision: 358919
>
> URL: http://llvm.org/viewvc/llvm-project?rev=358919&view=rev
> Log:
> [InstCombine] Eliminate stores to constant memory
>
> If we have a store to a piece of memory which is known constant, then we know the store must be storing back the same value. As a result, the store (or memset, or memmove) must either be down a dead path, or a noop. In either case, it is valid to simply remove the store.
>
> The motivating case for this involves a memmove to a buffer which is constant down a path which is dynamically dead.
>
> Note that I'm choosing to implement the less aggressive of two possible semantics here. We could simply say that the store *is undefined*, and prune the path. Consensus in the review was that the more aggressive form might be a good follow on change at a later date.
>
> Differential Revision: https://reviews.llvm.org/D60659
>
>
> Modified:
>     llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp
>     llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
>     llvm/trunk/test/Transforms/InstCombine/memcpy.ll
>     llvm/trunk/test/Transforms/InstCombine/memmove.ll
>     llvm/trunk/test/Transforms/InstCombine/memset.ll
>     llvm/trunk/test/Transforms/InstCombine/store.ll
>
> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp?rev=358919&r1=358918&r2=358919&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp (original)
> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineCalls.cpp Mon Apr 22 13:28:19 2019
> @@ -120,6 +120,15 @@ Instruction *InstCombiner::SimplifyAnyMe
>      return MI;
>    }
>
> +  // If we have a store to a location which is known constant, we can conclude
> +  // that the store must be storing the constant value (else the memory
> +  // wouldn't be constant), and this must be a noop.
> +  if (AA->pointsToConstantMemory(MI->getDest())) {
> +    // Set the size of the copy to 0, it will be deleted on the next iteration.
> +    MI->setLength(Constant::getNullValue(MI->getLength()->getType()));
> +    return MI;
> +  }
> +
>    // If MemCpyInst length is 1/2/4/8 bytes then replace memcpy with
>    // load/store.
>    ConstantInt *MemOpLength = dyn_cast<ConstantInt>(MI->getLength());
> @@ -218,6 +227,15 @@ Instruction *InstCombiner::SimplifyAnyMe
>      return MI;
>    }
>
> +  // If we have a store to a location which is known constant, we can conclude
> +  // that the store must be storing the constant value (else the memory
> +  // wouldn't be constant), and this must be a noop.
> +  if (AA->pointsToConstantMemory(MI->getDest())) {
> +    // Set the size of the copy to 0, it will be deleted on the next iteration.
> +    MI->setLength(Constant::getNullValue(MI->getLength()->getType()));
> +    return MI;
> +  }
> +
>    // Extract the length and alignment and fill if they are constant.
>    ConstantInt *LenC = dyn_cast<ConstantInt>(MI->getLength());
>    ConstantInt *FillC = dyn_cast<ConstantInt>(MI->getValue());
>
> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp?rev=358919&r1=358918&r2=358919&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp (original)
> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp Mon Apr 22 13:28:19 2019
> @@ -1438,6 +1438,12 @@ Instruction *InstCombiner::visitStoreIns
>      }
>    }
>
> +  // If we have a store to a location which is known constant, we can conclude
> +  // that the store must be storing the constant value (else the memory
> +  // wouldn't be constant), and this must be a noop.
> +  if (AA->pointsToConstantMemory(Ptr))
> +    return eraseInstFromFunction(SI);
> +
>    // Do really simple DSE, to catch cases where there are several consecutive
>    // stores to the same location, separated by a few arithmetic operations. This
>    // situation often occurs with bitfield accesses.
>
> Modified: llvm/trunk/test/Transforms/InstCombine/memcpy.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/memcpy.ll?rev=358919&r1=358918&r2=358919&view=diff
> ==============================================================================
> --- llvm/trunk/test/Transforms/InstCombine/memcpy.ll (original)
> +++ llvm/trunk/test/Transforms/InstCombine/memcpy.ll Mon Apr 22 13:28:19 2019
> @@ -40,7 +40,6 @@ define void @test3(i8* %d, i8* %s) {
>
>  define void @memcpy_to_constant(i8* %src) {
>  ; CHECK-LABEL: @memcpy_to_constant(
> -; CHECK-NEXT:    call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 4 bitcast (i128* @UnknownConstant to i8*), i8* align 1 [[SRC:%.*]], i32 16, i1 false)
>  ; CHECK-NEXT:    ret void
>  ;
>    %dest = bitcast i128* @UnknownConstant to i8*
>
> Modified: llvm/trunk/test/Transforms/InstCombine/memmove.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/memmove.ll?rev=358919&r1=358918&r2=358919&view=diff
> ==============================================================================
> --- llvm/trunk/test/Transforms/InstCombine/memmove.ll (original)
> +++ llvm/trunk/test/Transforms/InstCombine/memmove.ll Mon Apr 22 13:28:19 2019
> @@ -59,7 +59,6 @@ define void @test4(i8* %a) {
>
>  define void @memmove_to_constant(i8* %src) {
>  ; CHECK-LABEL: @memmove_to_constant(
> -; CHECK-NEXT:    call void @llvm.memmove.p0i8.p0i8.i32(i8* align 4 bitcast (i128* @UnknownConstant to i8*), i8* align 1 [[SRC:%.*]], i32 16, i1 false)
>  ; CHECK-NEXT:    ret void
>  ;
>    %dest = bitcast i128* @UnknownConstant to i8*
>
> Modified: llvm/trunk/test/Transforms/InstCombine/memset.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/memset.ll?rev=358919&r1=358918&r2=358919&view=diff
> ==============================================================================
> --- llvm/trunk/test/Transforms/InstCombine/memset.ll (original)
> +++ llvm/trunk/test/Transforms/InstCombine/memset.ll Mon Apr 22 13:28:19 2019
> @@ -26,7 +26,6 @@ define i32 @test([1024 x i8]* %target) {
>
>  define void @memset_to_constant() {
>  ; CHECK-LABEL: @memset_to_constant(
> -; CHECK-NEXT:    call void @llvm.memset.p0i8.i32(i8* align 4 bitcast (i128* @Unknown to i8*), i8 0, i32 16, i1 false)
>  ; CHECK-NEXT:    ret void
>  ;
>    %p = bitcast i128* @Unknown to i8*
>
> Modified: llvm/trunk/test/Transforms/InstCombine/store.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/store.ll?rev=358919&r1=358918&r2=358919&view=diff
> ==============================================================================
> --- llvm/trunk/test/Transforms/InstCombine/store.ll (original)
> +++ llvm/trunk/test/Transforms/InstCombine/store.ll Mon Apr 22 13:28:19 2019
> @@ -295,7 +295,6 @@ define void @write_back7(i32* %p) {
>
>  define void @store_to_constant() {
>  ; CHECK-LABEL: @store_to_constant(
> -; CHECK-NEXT:    store i32 0, i32* @Unknown, align 4
>  ; CHECK-NEXT:    ret void
>  ;
>    store i32 0, i32* @Unknown
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits