This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] performance-unnecessary-copy-initialization: Check for const reference arguments that are replaced template parameter type.
ClosedPublic

Authored by flx on Oct 23 2020, 6:48 AM.

Details

Summary

This fixes false positive cases where a non-const reference is passed to a
std::function but interpreted as a const reference.

Fix the definition of the fake std::function added in the test to match std::function and make the bug reproducible.

Diff Detail

Event Timeline

flx created this revision.Oct 23 2020, 6:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 23 2020, 6:48 AM
flx requested review of this revision.Oct 23 2020, 6:48 AM
flx updated this revision to Diff 300274.

Remove code comment.

Eugene.Zelenko added a project: Restricted Project.
flx updated this revision to Diff 300276.Oct 23 2020, 6:55 AM

Revert -std=c++17 change.

flx added a comment.Oct 23 2020, 6:57 AM

I should note that I was only able to reproduce the false positive with the actual implementation std::function and not our fake version here.

Harbormaster completed remote builds in B76188: Diff 300276.
In D90042#2350035, @flx wrote:

I should note that I was only able to reproduce the false positive with the actual implementation std::function and not our fake version here.

Any reason not to lift enough of the actual definition to be able to reproduce the issue in your test cases? Does the change in definitions break other tests?

flx added a comment.Oct 27 2020, 6:20 AM
In D90042#2350035, @flx wrote:

I should note that I was only able to reproduce the false positive with the actual implementation std::function and not our fake version here.

Any reason not to lift enough of the actual definition to be able to reproduce the issue in your test cases? Does the change in definitions break other tests?

I poured over the actual definition and couldn't find any difference wrt the call operator that would explain it. I would also think that:

template <typename T>
void foo(T&& t) {
  std::forward<T>(t).modify();
}

would be a simpler case that should trigger replacement, but it doesn't. Do you have any idea what I could be missing?

In D90042#2356265, @flx wrote:
In D90042#2350035, @flx wrote:

I should note that I was only able to reproduce the false positive with the actual implementation std::function and not our fake version here.

Any reason not to lift enough of the actual definition to be able to reproduce the issue in your test cases? Does the change in definitions break other tests?

I poured over the actual definition and couldn't find any difference wrt the call operator that would explain it. I would also think that:

template <typename T>
void foo(T&& t) {
  std::forward<T>(t).modify();
}

would be a simpler case that should trigger replacement, but it doesn't. Do you have any idea what I could be missing?

Perhaps silly question, but are you instantiating foo()?

flx updated this revision to Diff 301388.Oct 28 2020, 12:34 PM

Add instantiated template method.

flx updated this revision to Diff 301389.Oct 28 2020, 12:40 PM

Comment out replaced parameter.

flx added a comment.Oct 28 2020, 12:42 PM
In D90042#2356265, @flx wrote:
In D90042#2350035, @flx wrote:

I should note that I was only able to reproduce the false positive with the actual implementation std::function and not our fake version here.

Any reason not to lift enough of the actual definition to be able to reproduce the issue in your test cases? Does the change in definitions break other tests?

I poured over the actual definition and couldn't find any difference wrt the call operator that would explain it. I would also think that:

template <typename T>
void foo(T&& t) {
  std::forward<T>(t).modify();
}

would be a simpler case that should trigger replacement, but it doesn't. Do you have any idea what I could be missing?

Perhaps silly question, but are you instantiating foo()?

I think I added a full implementation of foo now, reverted the change, but am still not getting the negative case to fail. Can you spot an issue with the code?

In D90042#2360042, @flx wrote:
In D90042#2356265, @flx wrote:
In D90042#2350035, @flx wrote:

I should note that I was only able to reproduce the false positive with the actual implementation std::function and not our fake version here.

Any reason not to lift enough of the actual definition to be able to reproduce the issue in your test cases? Does the change in definitions break other tests?

I poured over the actual definition and couldn't find any difference wrt the call operator that would explain it. I would also think that:

template <typename T>
void foo(T&& t) {
  std::forward<T>(t).modify();
}

would be a simpler case that should trigger replacement, but it doesn't. Do you have any idea what I could be missing?

Perhaps silly question, but are you instantiating foo()?

I think I added a full implementation of foo now, reverted the change, but am still not getting the negative case to fail. Can you spot an issue with the code?

I can't, but to be honest, I'm not certain I understand how that false positive could happen in the first place. That's why I was hoping to see the original case -- one thing you could try is with the original code, pass -E to preprocess to a file, and then try reducing the test case from that output (either by hand or by using a tool like creduce), or did you already give that a shot?

flx added a comment.Nov 11 2020, 5:38 PM
In D90042#2360042, @flx wrote:
In D90042#2356265, @flx wrote:
In D90042#2350035, @flx wrote:

I should note that I was only able to reproduce the false positive with the actual implementation std::function and not our fake version here.

Any reason not to lift enough of the actual definition to be able to reproduce the issue in your test cases? Does the change in definitions break other tests?

I poured over the actual definition and couldn't find any difference wrt the call operator that would explain it. I would also think that:

template <typename T>
void foo(T&& t) {
  std::forward<T>(t).modify();
}

would be a simpler case that should trigger replacement, but it doesn't. Do you have any idea what I could be missing?

Perhaps silly question, but are you instantiating foo()?

I think I added a full implementation of foo now, reverted the change, but am still not getting the negative case to fail. Can you spot an issue with the code?

I can't, but to be honest, I'm not certain I understand how that false positive could happen in the first place. That's why I was hoping to see the original case -- one thing you could try is with the original code, pass -E to preprocess to a file, and then try reducing the test case from that output (either by hand or by using a tool like creduce), or did you already give that a shot?

Thanks for the suggestion, I had never hear of creduce! After a bit of trial an error I seem to have found a more minimal example:

namespace std {                                                                                                              
template <typename> class function;                                                                                          
template <typename a, typename... b> class function<a(b...)> {                                                               
public:                                                                                                                      
  void operator()(b...);                                                                                                     
};                                                                                                                           
} // namespace std                                                                                                           
struct c {                                                                                                                   
  c();                                                                                                                       
  c(const c &);                                                                                                              
};                                                                                                                           
std::function<void(c &)> f;                                                                                                  
void d() {                                                                                                                   
  c Orig;                                                                                                                    
  c Copy = Orig;                                                                                                             
  f(Copy);                                                                                                                   
}

To be frank I can't spot a meaningful difference to the std::function copy we already have.

Here's also the test script I used for posterity:

#!/bin/bash                                                                                                                  
                                                                                                                             
trap 'exit 1' ERR                                                                                                            
out=$(tempfile)                                                                                                              
clang-tidy  --checks=-*,performance-unnecessary-copy-initialization --extra-arg=-std=c++17 full.cc > $out                                     
grep "warning: local copy.*Copy" $out                                                                                        
grep "f(Copy)" full.cc                                                                                                       
grep "std::function" full.cc

I'll update the test case with this next.

In D90042#2390203, @flx wrote:

Thanks for the suggestion, I had never hear of creduce!

Glad to have introduced you to it -- it's a great tool!

After a bit of trial an error I seem to have found a more minimal example:

namespace std {                                                                                                              
template <typename> class function;                                                                                          
template <typename a, typename... b> class function<a(b...)> {                                                               
public:                                                                                                                      
  void operator()(b...);                                                                                                     
};                                                                                                                           
} // namespace std                                                                                                           
struct c {                                                                                                                   
  c();                                                                                                                       
  c(const c &);                                                                                                              
};                                                                                                                           
std::function<void(c &)> f;                                                                                                  
void d() {                                                                                                                   
  c Orig;                                                                                                                    
  c Copy = Orig;                                                                                                             
  f(Copy);                                                                                                                   
}

To be frank I can't spot a meaningful difference to the std::function copy we already have.

Aha, I may have spotted it. The call operators have subtly different signatures and the signature we have in our test file is wrong. Note the && in our test file compared to what the standard defines: http://eel.is/c++draft/func.wrap.func#inv which is what's causing the difference here: https://godbolt.org/z/hxfM7P

flx updated this revision to Diff 304831.Nov 12 2020, 7:20 AM

Fixed definition of fake std::function which now makes the bug fixed by this change reproducible.

flx updated this revision to Diff 304833.Nov 12 2020, 7:27 AM

Remove unnecessary test code that is not needed.

flx added a comment.Nov 12 2020, 7:28 AM
In D90042#2390203, @flx wrote:

Thanks for the suggestion, I had never hear of creduce!

Glad to have introduced you to it -- it's a great tool!

After a bit of trial an error I seem to have found a more minimal example:

namespace std {                                                                                                              
template <typename> class function;                                                                                          
template <typename a, typename... b> class function<a(b...)> {                                                               
public:                                                                                                                      
  void operator()(b...);                                                                                                     
};                                                                                                                           
} // namespace std                                                                                                           
struct c {                                                                                                                   
  c();                                                                                                                       
  c(const c &);                                                                                                              
};                                                                                                                           
std::function<void(c &)> f;                                                                                                  
void d() {                                                                                                                   
  c Orig;                                                                                                                    
  c Copy = Orig;                                                                                                             
  f(Copy);                                                                                                                   
}

To be frank I can't spot a meaningful difference to the std::function copy we already have.

Aha, I may have spotted it. The call operators have subtly different signatures and the signature we have in our test file is wrong. Note the && in our test file compared to what the standard defines: http://eel.is/c++draft/func.wrap.func#inv which is what's causing the difference here: https://godbolt.org/z/hxfM7P

Ah! That is subtle and surprising, but makes sense. I confirmed that the new test cases fail now before the fix is put in place. Thanks for your help!

flx updated this revision to Diff 304834.Nov 12 2020, 7:30 AM

Updated change description.

flx edited the summary of this revision. (Show Details)Nov 12 2020, 7:31 AM
Harbormaster completed remote builds in B78621: Diff 304831.
aaron.ballman accepted this revision.Nov 16 2020, 11:14 AM

LGTM, thank you for the fix!

This revision is now accepted and ready to land.Nov 16 2020, 11:14 AM
flx added a comment.Nov 16 2020, 12:35 PM

LGTM, thank you for the fix!

Thanks for helping track down the difference in our definition of std::function :)