[cfe-dev] Missing warning when binding to const ref?

[David Blaikie]

On Tue, Apr 7, 2015 at 9:26 PM, Nikola Smiljanic <popizdeh at gmail.com> wrote:

> Yeah, something like that, it really feels like it belongs in the static
> analyzer. BTW, would any of the sanitizers catch this?
>

I believe MSan would be the tool to catch this, but I'm not sure how good
its stack lifetime tracking is just yet.

(& yes, I don't think there's a local warning (non-static analysis) that
would be good for this. Stashing a reference to a temporary in a member
variable isn't, itself, a bug - see Twine for example - you just have to
use it carefully. That said, we did have some bugs in LLVM/Clang due to
taking references to temporaries from conversions - ArrayRef<Base*>
constructed from a single Derived* - the Derived* was converted to a
temporary Base*, the ArrayRef had a pointer to that Base* which ceased to
exist at the end of the full expression... )


>
> On Wed, Apr 8, 2015 at 2:20 PM, Daniel Dilts <diltsman at gmail.com> wrote:
>
>> So, it would be something like:
>> If MaterializeTemporaryExpr and assign result to a reference that
>> persists beyond the scope of the function
>> Then generate a warning
>>
>> Something like that?  I am not an expert on this; could someone else
>> comment on the reasonableness of this?
>>
>> On Tue, Apr 7, 2015 at 8:55 PM, Nikola Smiljanic <popizdeh at gmail.com>
>> wrote:
>>
>>> We just ran into this in production, but as you can imagine it was much
>>> harder to spot. It only happened in optimized code. It was hidden behind
>>> templates and one member of the class would try to bind a const ref to int
>>> to something that was an enum. Combine that with mfc hash map and results
>>> are spectacular :)
>>>
>>> Looking at ast dump I can see MaterializeTemporaryExpr. Couldn't we just
>>> check that one of function's or constructor's arguments is coming from it?
>>>
>>> On Wed, Apr 8, 2015 at 1:41 PM, Daniel Dilts <diltsman at gmail.com> wrote:
>>>
>>>> Ah!  I finally see where your issue is.  This problem is probably
>>>> better suited for static code analysis.  For a trivial example like this
>>>> you can pretty quickly track down what is wrong, but for the more general
>>>> case I don't imagine that this being a check that can be done quickly
>>>> enough for a compiler warning.
>>>>
>>>> On Tue, Apr 7, 2015 at 8:04 PM, Nikola Smiljanic <popizdeh at gmail.com>
>>>> wrote:
>>>>
>>>>> I should have made a better comment, the types are different!
>>>>> Parameter needs to be implicitly converted to int, and we're binding a a
>>>>> const ref to this temp object whose lifetime ends once S is constructed.
>>>>> Unless I'm mistaken.
>>>>>
>>>>> On Wed, Apr 8, 2015 at 1:01 PM, Daniel Dilts <diltsman at gmail.com>
>>>>> wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> On Tue, Apr 7, 2015 at 6:11 PM, Nikola Smiljanic <popizdeh at gmail.com>
>>>>>> wrote:
>>>>>>
>>>>>>> I'm surprised this doesn't produce a warning:
>>>>>>>
>>>>>>> struct S
>>>>>>> {
>>>>>>> S(const int& i) : i(i) {}
>>>>>>> const int& i;
>>>>>>> };
>>>>>>>
>>>>>>> void foo(long l)
>>>>>>> {
>>>>>>> S s(l); // binding a const ref to temporary
>>>>>>> }
>>>>>>>
>>>>>>> int main()
>>>>>>> {
>>>>>>> foo(1l);
>>>>>>> }
>>>>>>>
>>>>>>>
>>>>>>> Why would that produce a warning?  You can bind an r-value to a
>>>>>> const l-value reference.  And, function foo takes the parameter by value,
>>>>>> so it is passing an l-value to the constructor of S.
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20150408/7060dc32/attachment.html>