[PATCH] D74094: Reapply: [IRGen] Emit lifetime intrinsics around temporary aggregate argument allocas

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

In D74094#4643577 <https://reviews.llvm.org/D74094#4643577>, @rjmccall wrote:

> In D74094#4643558 <https://reviews.llvm.org/D74094#4643558>, @nickdesaulniers wrote:
>
>> In D74094#4643495 <https://reviews.llvm.org/D74094#4643495>, @rjmccall wrote:
>>
>>> I can't easily tell you because the "standalone example" involves a million templates that I don't know anything about, and nobody seems to have done the analysis to identify the specific source of the miscompile.
>>
>> Sure; @alexfh @vitalybuka can you both please help produce a further reduced test case from https://reviews.llvm.org/D74094#4633799 ? Otherwise I'm beginning to think this patch may have been reverted prematurely; before an understood + reduced test case was reduced.
>
> Well, it's good to be conservative about adding new optimizations if they're causing apparent miscompiles.  Even if the optimization is just exposing existing UB in some new one, that's something we should understand as a project.  I have no problem with the eager revert.
>
> It would be great if Alex and Vitaly could help track down the exact problem.  However, if they can't, I think it's ultimately your responsibility as the person hoping to land the optimization to understand why the miscompile is not in fact your fault.

I've cloned:

1. abseil @ f3eae68bd1d17df708f2b59a43ad7e837a616e6a
2. eigen @ 8f858a4ea8106b52f3cf475ffc5e0e9c6a91baa2
3. googletest @ eab0e7e289db13eabfc246809b0284dac02a369d

I had to build googletest.  Then I built a.out via:

  $ clang++ foo.cpp -I eigen/ -I googletest/googletest/include/ \
    -I googletest/googlemock/include/ -I abseil-cpp googletest/build/lib/libgtest.a \
    -std=c++17 -O3 -fsanitize=address -march=haswell

With this change re-applied (`b7f4915644844fb9f32e8763922a070f5fe4fd29` reverted), a.out runs without issue.   So yes, I need something other than a godbolt link which uses clang-trunk which isn't stable after a revert to understand what they are talking about.

>>> What I can tell you is that there is an enormous semantic difference between passing a pr-value argument to a parameter that's passed by reference and to a parameter that's passed by value.  In the former case, the reference is bound to a temporary with full-expression lifetime, and it is totally reasonable to return its address.  In the latter case, the parameter object will often have a lifetime bound by the current function, and you cannot return its address without it being UB.
>>
>> I'm trying to come up with an example of "the former."
>>
>>   struct foo {
>>     int x;
>>   };
>>   
>>   // Returns a reference to the minimum foo.
>>   foo &min(foo &a, foo &b);
>>   
>>   void consume (foo&);
>>   
>>   void bar () {
>>     consume(min(foo{0}, foo{1}));
>>   }
>>
>> doesn't compile:
>>
>>   foo.cpp:11:11: error: no matching function for call to 'min'
>>      11 |   consume(min(foo{0}, foo{1}));
>>         |           ^~~
>>   foo.cpp:6:6: note: candidate function not viable: expects an lvalue for 1st argument
>>       6 | foo &min(foo &a, foo &b);
>>         |      ^   ~~~~~~
>>
>> Have I misinterpreted what you meant by "passing a pr-value argument to a parameter that's passed by reference?"
>
> Okay, to be frank, this is a bit of a red flag for me.  Optimizations like this are ultimately rooted in language rules, and I'm not sure you can effectively do this work if you don't understand the basics of C++ parameter passing.

Fuck me for trying to help, right?  If x-values are part of the "basics" of parameter passing, I'm afraid to ask about the more complicated cases.

> In C++, you cannot bind a non-const l-value reference to a temporary, but you can bind either a const l-value reference or an x-value reference to one.  So you could declare `min` as either `const foo &min(const foo &a, const foo &b);` or `foo &&min(foo &&a, foo &&b);`.

Thanks, that's //all// I needed.

With `b7f4915644844fb9f32e8763922a070f5fe4fd29` reverted, neither of those cases look fishy to me:
https://gist.github.com/nickdesaulniers/1c7b962222639e54c3339b63b572b166


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74094