llvm dev - Dec 2012 - [LLVMdev] Can simplifycfg kill llvm.lifetime intrinsics?

Hi!

I'm working on ASan option that uses llvm.lifetime intrinsics to detect
use-after-scope bugs. In short, the idea is to
insert calls into ASan runtime that would mark the memory as
"addressable"
or "unaddressable".
I see the following problem with the following "trivial" basic block:

for.body.lr.ph.i:                                 ; preds = %for.body.i310
  call void @llvm.lifetime.start(i64 24, i8* %174)
  call void @llvm.lifetime.start(i64 4, i8* %175)
  call void @llvm.lifetime.start(i64 24, i8* %176)
  br label %for.body.i318

r134182 by Rafael explicitly allows simplifycfg pass to merge this block
into its successor, and drop "side-effect free" lifetime.start
intrinsics. However, llvm.lifetime.end intrinsics for the same memory are
preserved, which is not only weird, but triggers ASan false positives:
1) function goes into for-loop with local variable declared in it
2) llvm.lifetime.end() at the end of the loop allows ASan to mark this
memory as unaddressable
3) at the next loop iteration access to this memory will be reported as
error.

Shouldn't simplifycfg somehow preserve / move lifetime intrinsics in its
transformations?

-- 
Alexey Samsonov, MSK
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20121217/146a6732/attachment.html>

This looks like a bug in simplifycfg. We should preserve lifetime
intrinsics due to the reasons I described.
The code in //lib/Transforms/Utils/Local.cpp:

  if (Succ->getSinglePredecessor()) {
    // BB is the only predecessor of Succ, so Succ will end up with exactly
    // the same predecessors BB had.

    // Copy over any phi, debug or lifetime instruction.
    BB->getTerminator()->eraseFromParent();
    Succ->getInstList().splice(Succ->getFirstNonPHI(),
BB->getInstList());
  }

does this only when successor of basic block being removed has a single
predecessor.
This is not the case even for simple test in
/test/Transforms/SimplifyCFG/lifetime.ll.

That said, I think for now we should just apply the patch attached to this
mail. Please take a look.

On Mon, Dec 17, 2012 at 6:59 PM, Alexey Samsonov <samsonov at
google.com>wrote:
> Hi!
>
> I'm working on ASan option that uses llvm.lifetime intrinsics to detect
> use-after-scope bugs. In short, the idea is to
> insert calls into ASan runtime that would mark the memory as
"addressable"
> or "unaddressable".
> I see the following problem with the following "trivial" basic
block:
>
> for.body.lr.ph.i:                                 ; preds = %for.body.i310
>   call void @llvm.lifetime.start(i64 24, i8* %174)
>   call void @llvm.lifetime.start(i64 4, i8* %175)
>   call void @llvm.lifetime.start(i64 24, i8* %176)
>   br label %for.body.i318
>
> r134182 by Rafael explicitly allows simplifycfg pass to merge this block
> into its successor, and drop "side-effect free" lifetime.start
> intrinsics. However, llvm.lifetime.end intrinsics for the same memory are
> preserved, which is not only weird, but triggers ASan false positives:
> 1) function goes into for-loop with local variable declared in it
> 2) llvm.lifetime.end() at the end of the loop allows ASan to mark this
> memory as unaddressable
> 3) at the next loop iteration access to this memory will be reported as
> error.
>
> Shouldn't simplifycfg somehow preserve / move lifetime intrinsics in
its
> transformations?
>
> --
> Alexey Samsonov, MSK
>


-- 
Alexey Samsonov, MSK
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20121224/2a388d2c/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: zdiff.lifetime-simplifycfg
Type: application/octet-stream
Size: 1231 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20121224/2a388d2c/attachment.obj>

On 24 December 2012 04:02, Alexey Samsonov <samsonov at google.com>
wrote:
> This looks like a bug in simplifycfg. We should preserve lifetime
intrinsics
> due to the reasons I described.
> The code in //lib/Transforms/Utils/Local.cpp:
>
>   if (Succ->getSinglePredecessor()) {
>     // BB is the only predecessor of Succ, so Succ will end up with exactly
>     // the same predecessors BB had.
>
>     // Copy over any phi, debug or lifetime instruction.
>     BB->getTerminator()->eraseFromParent();
>     Succ->getInstList().splice(Succ->getFirstNonPHI(),
BB->getInstList());
>   }
>
> does this only when successor of basic block being removed has a single
> predecessor.
> This is not the case even for simple test in
> /test/Transforms/SimplifyCFG/lifetime.ll.
>
> That said, I think for now we should just apply the patch attached to this
> mail. Please take a look.
Sorry I missed this email the first time.
>From http://llvm.org/docs/LangRef.html it looks to be valid to delete
only one of the functions. The semantics being

* only llvm.lifetime.start: Any loads before the call can be replaced
with undef. Since there is no end, all stores must be kept.
* only llvm.lifetime.end: Any stores after this can be deleted. Since
there is no start, all loads must be kept.

I guess with asan the "replace with undef/ remove store" become
"diagnose those loads/stores".

I am pretty sure we should not avoid optimizing because there is a
llvm.lifetime.* in a BB. If we decide that it is invalid to delete one
but not the other, we should change SimplifyCFG to delete both, not
avoid merging the BBs.

Nick, is the above what you had in mind when you added
llvm.lifetime.*? We should clarify the language ref even if it is
valid to drop only one of the calls.

Cheers,
Rafael