[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Aaron Gokaslan via Phabricator via cfe-commits]

Skylion007 added a comment.

Pinging @njames93 to about the questions from the previous comment. Would love 
to see something like this PR get merged.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Aaron Gokaslan via Phabricator via cfe-commits]

Skylion007 added a comment.

Use llvm::find_if instead of std::find_if


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Aaron Gokaslan via Phabricator via cfe-commits]

Skylion007 added a comment.

I noticed one other bug from testing this out on some C++ codebases.

Specifically in pybind11 we have an object class which is a child of the handle 
object. The object class is non-trivially copyable, while the handle object is 
not. Therefore, when we do not want to increase / decrease the reference count 
of object, we often rely on object's implicit casting to a handle. While I 
suppose you could std::move() to cast an object for a handle, there isn't a 
good reason to do so. You get all the drawbacks of using std::move (ie. use 
after free bugs potentially) with none of the benefits.

Finally, this is a smaller issue but there seems to be a bug where the fix it 
can apply `std::move(std::move(obj));`. That is relatively low pri and not 
blocking by any means though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Aaron Gokaslan via Phabricator via cfe-commits]

Skylion007 added a comment.

One other bug I found with this diff, is it seems to suggest calling 
std::move() on function args that are references, despite the fact that 
invalidating the reference to the input arg could be undesirable. For instance 
take a function that takes in a reference to a String, assigns a new value to 
the arg reference, and then returns the value of the reference. It suggests 
calling std::move() on the arg ref when it is returned, which invalidate the 
reference to the argument.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Aaron Gokaslan via Phabricator via cfe-commits]

Skylion007 added a comment.

In D137205#3913192 , @Febbe wrote:

> In D137205#3912243 , @Skylion007 
> wrote:
>
>> One other bug I found with this diff, is it seems to suggest calling 
>> std::move() on function args that are references, despite the fact that 
>> invalidating the reference to the input arg could be undesirable. For 
>> instance take a function that takes in a reference to a String, assigns a 
>> new value to the arg reference, and then returns the value of the reference. 
>> It suggests calling std::move() on the arg ref when it is returned, which 
>> invalidate the reference to the argument.
>
> Oh, that's not good. Actually, it should not run on references, only  
> `declRefExpr(hasType(qualType(unless(anyOf(isConstQualified(), 
> referenceType(), pointerType() ,..)` are allowed to be parsed.
>
> Did you run this check alone, without other checks?
>
> Regarding the appearance of `std::move(std::move( var );`, was there a move 
> already? Does it occur in a header file? In case of running clang-tidy with 
> fixups in parallel over multiple sources, it can happen, that a header file 
> is parsed twice at the same time, resulting in duplicate insertions.
>
> And last but not least, can you provide a source snipped or AST for both 
> appearances?

The problematic code snippit can be found here: 
https://github.com/facebookresearch/habitat-sim/blob/c88851802f1905ab483e380367753dc6a12c6d21/src/esp/metadata/managers/AbstractObjectAttributesManagerBase.h#L323

And results in this diff:

  diff --git a/src/esp/metadata/managers/AbstractObjectAttributesManagerBase.h 
b/src/esp/metadata/managers/AbstractObjectAttributesManagerBase.h
  index d5564c8f..37a38b00 100644
  --- a/src/esp/metadata/managers/AbstractObjectAttributesManagerBase.h
  +++ b/src/esp/metadata/managers/AbstractObjectAttributesManagerBase.h
  @@ -10,6 +10,8 @@
* esp::metadata::managers::AbstractObjectAttributesManager
*/
  
  +#include 
  +
   #include "esp/metadata/attributes/ObjectAttributes.h"
  
   #include "esp/metadata/MetadataUtils.h"
  @@ -312,7 +314,7 @@ AbstractObjectAttributesManager::setJSONAssetHandleAndType(
   const std::function& meshTypeSetter) {
 std::string propertiesFileDirectory = attributes->getFileDirectory();
 // save current file name
  -  std::string oldFName(assetName);
  +  std::string oldFName(std::move(assetName));
 // clear var to get new value - if returns true use this as new value
 assetName = "";
 // Map a json string value to its corresponding AssetType if found and 
cast to
  @@ -362,7 +364,7 @@ AbstractObjectAttributesManager::setJSONAssetHandleAndType(
  meshTypeSetter);
 }
   }  // value is valid prim and exists, else value is valid file and exists
  -return assetName;
  +return std::move(assetName);
 }
 // handle value is not present in JSON or is specified but does not change
 return oldFName;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Aaron Gokaslan via Phabricator via cfe-commits]

Skylion007 added a comment.

Okay, now I am getting what I believe to be segfaults:

  #0 0x564383482be4 PrintStackTraceSignalHandler(void*) Signals.cpp:0:0
  #1 0x564383480464 SignalHandler(int) Signals.cpp:0:0
  #2 0x7f7c275c9420 __restore_rt 
(/lib/x86_64-linux-gnu/libpthread.so.0+0x14420)
  #3 0x5643804b0ea5 
clang::tidy::performance::UnnecessaryCopyOnLastUseCheck::check(clang::ast_matchers::MatchFinder::MatchResult
 const&) (.cold) UnnecessaryCopyOnLastUseCheck.cpp:0:0
  #4 0x56438262bba1 clang::ast_matchers::internal::(anonymous 
namespace)::MatchASTVisitor::MatchVisitor::visitMatch(clang::ast_matchers::BoundNodes
 const&) ASTMatchFinder.cpp:0:0
  #5 0x5643826818df 
clang::ast_matchers::internal::BoundNodesTreeBuilder::visitMatches(clang::ast_matchers::internal::BoundNodesTreeBuilder::Visitor*)
 (/home/aaron/git/llvm-project/build/bin/clang-tidy+0x2d258df)

It worked before this PR, now it just crashes on a lot of real world codebases 
including just trying to run it for a few files on LLVM's own codebase.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Aaron Gokaslan via Phabricator via cfe-commits]

Skylion007 added a comment.

Currently, this check also tries to move static values which is very 
undesirable and unlikely to be correct.

 static auto value = std::string("CONSTANT");
  -  return value;
  +  return std::move(value);


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Aaron Gokaslan via Phabricator via cfe-commits]

Skylion007 added a comment.

Hmm, I still had a crash on this latest code (although it is crashing much more 
rarely now.

It crashes on scanning this file 
https://github.com/facebookresearch/habitat-sim/blob/5fb04078b0b8432dc4a88ec186a2b7af74163be1/src/esp/core/managedContainers/ManagedContainerBase.cpp

  Found compiler error(s).
  /home/aaron/git/llvm-project/build/bin/clang-tidy -checks=-*,*perf*last* 
-export-fixes /tmp/tmp38bqy_w9/tmpa_p4xe5f.yaml -p=build 
/home/aaron/git/habitat-sim/src/esp/core/managedContainers/ManagedContainerBase.cpp
  PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ 
and include the crash backtrace.
  Stack dump:
  0.Program arguments: /home/aaron/git/llvm-project/build/bin/clang-tidy 
-checks=-*,*perf*last* -export-fixes /tmp/tmp38bqy_w9/tmpa_p4xe5f.yaml -p=build 
/home/aaron/git/habitat-sim/src/esp/core/managedContainers/ManagedContainerBase.cpp
  1. parser at end of file
  2.ASTMatcher: Processing 'performance-unnecessary-copy-on-last-use' 
against:
CXXConstructExpr : 

  --- Bound Nodes Begin ---
  constructExpr - { CXXConstructExpr : 

 }
  param - { DeclRefExpr : 

 }
  --- Bound Nodes End ---
   #0 0x55a33b1c14f4 PrintStackTraceSignalHandler(void*) Signals.cpp:0:0
   #1 0x55a33b1bed74 SignalHandler(int) Signals.cpp:0:0
   #2 0x7f80af664420 __restore_rt 
(/lib/x86_64-linux-gnu/libpthread.so.0+0x14420)
   #3 0x55a338761176 
clang::tidy::performance::UnnecessaryCopyOnLastUseCheck::check(clang::ast_matchers::MatchFinder::MatchResult
 const&) (/home/aaron/git/llvm-project/build/bin/clang-tidy+0x10c7176)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Aaron Gokaslan via Phabricator via cfe-commits]

Skylion007 added a comment.

The main false positive I also keep seeing is in pybind11 where it suggests 
call an std::move() for an implicit conversion, but the target of the implicit 
conversion does not have an rvalue. (In this case, we have a py::object which 
inherits from py::handle, and is just py::handle with ownership semantics. This 
allows implicit conversion to a reference at anytime, and therefore std::move 
has no effect here except to complicate the code a bit.




Comment at: 
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp:114-115
+static Usage definiteLastUse(ASTContext *Context, CFG *const TheCFG,
+ DeclRefExpr const *DeclRef) {
+  assert(TheCFG != nullptr);
+

This assertion is not valid. The crash I linked above is due to "TheCFG" 
sometimes being null unfortunately. Making it return Usage::Error stops the 
crash at least. Seems to happen when it cannot find an include . 




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Aaron Gokaslan via Phabricator via cfe-commits]

Skylion007 added a comment.

I am trying this in the wild and getting some false positives where it tries to 
call std::move inside loop conditions and in the boolean condition for an if 
statement. Stuff like:

  if (auto* new_ptr = steal_ptr(std::move(old_ptr))) {
...
  }
  else {
return old_ptr; // Now it's moved from and invalid
  }

Also this can be happen for similar boolean conditionals in while, for, do 
while loops.

Interestingly, the logic in bugprone-use-after-move flags this error so maybe 
we can reuse some of that logic to detect bad std::move.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Aaron Gokaslan via Phabricator via cfe-commits]

Skylion007 added a comment.

In D137205#4054534 , @Febbe wrote:

> In D137205#4047828 , @Skylion007 
> wrote:
>
>> I am trying this in the wild and getting some false positives where it tries 
>> to call std::move inside loop conditions and in the boolean condition for an 
>> if statement. Stuff like:
>>
>>   if (SmartPtr new_ptr = steal_ptr(std::move(old_ptr))) {
>> ...
>>   }
>>   else {
>> return old_ptr; // Now it's moved from and invalid
>>   }
>>
>> Also this can be happen for similar boolean conditionals in while, for, do 
>> while loops.
>>
>> Interestingly, the logic in bugprone-use-after-move flags this error so 
>> maybe we can reuse some of that logic to detect bad std::move.
>
> Does this only happen on "init-statements" of while/if/switch?
> And thanks for the catch, I'll take a look into it as soon I have more time.
>
> Btw. I've oriented myself at the "bugprone-use-after-move" check, but simply 
> inverting it does not work here.

It happens in any conditional statements.

Another common failure case

  while(shouldStop(std::move(a))){
  // doSomething
  // a is last usage, but shouldStop called multiple times
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Aaron Gokaslan via Phabricator via cfe-commits]

Skylion007 requested changes to this revision.
Skylion007 added a comment.
This revision now requires changes to proceed.

Typo




Comment at: 
clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp:227
+
+  auto const IsMoveAssingable = cxxOperatorCallExpr(
+  hasDeclaration(cxxMethodDecl(

Typo: IsMoveAssignable


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits