Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

Skylion007 (Aaron Gokaslan)
User

Projects

User does not belong to any projects.

User Details

User Since
Jun 6 2022, 10:47 AM (68 w, 5 d)

Recent Activity

Jan 16 2023

Skylion007 updated the diff for D141866: [llvm][NFC] Apply performance-noexcept-move-constructor to llvm core.

revert changes to JITLink

Jan 16 2023, 1:50 PM · Restricted Project, Restricted Project
Skylion007 updated the diff for D141866: [llvm][NFC] Apply performance-noexcept-move-constructor to llvm core.

Fixup one another cpp error

Jan 16 2023, 1:27 PM · Restricted Project, Restricted Project
Skylion007 updated the diff for D141866: [llvm][NFC] Apply performance-noexcept-move-constructor to llvm core.

Make jitlink cpp match

Jan 16 2023, 12:56 PM · Restricted Project, Restricted Project
Skylion007 updated the diff for D141866: [llvm][NFC] Apply performance-noexcept-move-constructor to llvm core.

Fix another forward declare

Jan 16 2023, 12:32 PM · Restricted Project, Restricted Project
Skylion007 updated the diff for D141866: [llvm][NFC] Apply performance-noexcept-move-constructor to llvm core.

Fix forward declare typo

Jan 16 2023, 12:20 PM · Restricted Project, Restricted Project
Skylion007 updated the diff for D141866: [llvm][NFC] Apply performance-noexcept-move-constructor to llvm core.

Fix forward declare typo

Jan 16 2023, 12:19 PM · Restricted Project, Restricted Project
Skylion007 updated the diff for D141866: [llvm][NFC] Apply performance-noexcept-move-constructor to llvm core.

Fixed forward declare bug

Jan 16 2023, 12:17 PM · Restricted Project, Restricted Project
Skylion007 updated the diff for D141866: [llvm][NFC] Apply performance-noexcept-move-constructor to llvm core.

Made sure headers updated too. Skip support and ADT files for now.

Jan 16 2023, 11:30 AM · Restricted Project, Restricted Project
Skylion007 requested review of D141866: [llvm][NFC] Apply performance-noexcept-move-constructor to llvm core.
Jan 16 2023, 11:01 AM · Restricted Project, Restricted Project

Jan 15 2023

Skylion007 added a comment to D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

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.

Jan 15 2023, 8:02 AM · Restricted Project, Restricted Project

Jan 12 2023

Skylion007 added a comment to D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

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:

Jan 12 2023, 8:50 AM · Restricted Project, Restricted Project

Dec 18 2022

Skylion007 added a comment to D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

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

Dec 18 2022, 8:26 AM · Restricted Project, Restricted Project

Nov 22 2022

Skylion007 requested changes to D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

Typo

Nov 22 2022, 1:54 PM · Restricted Project, Restricted Project

Nov 13 2022

Skylion007 added a comment to D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

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.

Nov 13 2022, 9:45 AM · Restricted Project, Restricted Project

Nov 9 2022

Skylion007 added a comment to D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

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

Nov 9 2022, 7:46 AM · Restricted Project, Restricted Project
Skylion007 added a comment to D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

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);
Nov 9 2022, 7:05 AM · Restricted Project, Restricted Project

Nov 8 2022

Skylion007 added a comment to D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

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

#0 0x0000564383482be4 PrintStackTraceSignalHandler(void*) Signals.cpp:0:0
#1 0x0000564383480464 SignalHandler(int) Signals.cpp:0:0
#2 0x00007f7c275c9420 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x14420)
#3 0x00005643804b0ea5 clang::tidy::performance::UnnecessaryCopyOnLastUseCheck::check(clang::ast_matchers::MatchFinder::MatchResult const&) (.cold) UnnecessaryCopyOnLastUseCheck.cpp:0:0
#4 0x000056438262bba1 clang::ast_matchers::internal::(anonymous namespace)::MatchASTVisitor::MatchVisitor::visitMatch(clang::ast_matchers::BoundNodes const&) ASTMatchFinder.cpp:0:0
#5 0x00005643826818df 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.

Nov 8 2022, 10:40 AM · Restricted Project, Restricted Project

Nov 7 2022

Skylion007 added a comment to D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

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?

Nov 7 2022, 12:30 PM · Restricted Project, Restricted Project
Skylion007 added a comment to D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

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.

Nov 7 2022, 7:33 AM · Restricted Project, Restricted Project

Nov 3 2022

Skylion007 added a comment to D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

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

Nov 3 2022, 8:12 AM · Restricted Project, Restricted Project
Skylion007 added a comment to D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check.

Use llvm::find_if instead of std::find_if

Nov 3 2022, 7:51 AM · Restricted Project, Restricted Project