[PATCH] D153453: [MemCpyOpt] implement single BB stack-move optimization which unify the static unescaped allocas

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added a comment.

In D153453#4556190 <https://reviews.llvm.org/D153453#4556190>, @khei4 wrote:

> @vitalybuka  @glandium @eaeltsin 
> Thank you for reducing crashes, and reverting! I really appreciate your analysis.
>
>> - src has no lifetime markers - so it's considered alive all the time
>> - dst has lifetiem markers
>>
>> I guess the pass removes dst and transfer lifetimes to src, effectively reducing scope. So I would recommend to handle this case some how. In general dst lifetime can only expand src lifetime.
>
> That means Asan has the different semantics for
>
>   {
>   lifetime.start(src)
>   ...
>   lifetime.end(src)
>   ret void
>   }
>
> and
>
>   {
>   ...(no lifetime.start/end)
>   ret void
>   }

Not sure what you asking.
Lifetime intrinsics are optional. if they are omitted, alloca is valid from entry to any return. So asan, if no markers, can not detect use after scope, if the is markers, it will complain on any alloca access after the end marker.

Reproducer is probably oversimplified. In real miscompile case after the end, inserted by your transformation, there was use of the src alloca. Which was valid when we have no markers, but invalid now, because it's after the end.

I've update the reproducer df0b1df99c9ccf110482661678321e596566c725 <https://reviews.llvm.org/rGdf0b1df99c9ccf110482661678321e596566c725>.
That's what we have with D153453 <https://reviews.llvm.org/D153453>

  define void @test() {
  entry:
    %agg.tmp.sroa.14 = alloca [20 x i8], align 4
    %agg.tmp.sroa.14.128.sroa_idx = getelementptr i8, ptr %agg.tmp.sroa.14, i64 4
    call void @llvm.lifetime.start.p0(i64 20, ptr %agg.tmp.sroa.14)
    call void @llvm.memcpy.p0.p0.i64(ptr %agg.tmp.sroa.14.128.sroa_idx, ptr null, i64 1, i1 false)
    %agg.tmp3.sroa.35.128.sroa_idx = getelementptr i8, ptr %agg.tmp.sroa.14, i64 4
    call void @llvm.memcpy.p0.p0.i64(ptr inttoptr (i64 4 to ptr), ptr %agg.tmp3.sroa.35.128.sroa_idx, i64 1, i1 false)
    call void @llvm.lifetime.end.p0(i64 20, ptr %agg.tmp.sroa.14)                                   ; <------------------------------- END
    call void @llvm.memcpy.p0.p0.i64(ptr null, ptr %agg.tmp3.sroa.35.128.sroa_idx, i64 1, i1 false) ; <------------------------------- use after END
    ret void
  }



> right?
> I (perhaps mistakenly) think, in the case you give, src alloca lifetime seems to have not so much change, at least for the point that it's alive during the function call, but releasing by even automatic end and manual end might differ. Anyway, I'll try to repro the Asan crash and see the problem,
>
>> BTW. Also there are cases when lifetime is not very trivial between basic block. We don't have issue with that yet, but I think it's better to bail out and don't touch if we have more then 1 start/end for source or dst, or final lifetime can not be expressed as single range.
>
>
>
>> So the pass MUST not reduce lifetime of existing allocas - that's miscompile.
>> Expanding is acceptable, but can make stack coloring or sanitizers less efficient.
>
> Hmm, seems reasonable, it may take time, but I'll treat those! thanks!




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153453/new/

https://reviews.llvm.org/D153453