[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

Wed Jan 8 16:15:00 PST 2020

JonasToth added a comment.

I did evaluate the performance of the check on LLVM with the script in the attachement. F11185973: full_transformation.sh <https://reviews.llvm.org/F11185973>

The result, after splitting all multi-decl statements with `readability-isolate-declarations` (which produces no issues) the current analysis for `const` is run for values, references and pointers (the pointer itself, not the pointee) and the automated fixes are applied. This is done for **each** TU in LLVM/{llvm,clang,clang-tools-exta,lld}, including unit-tests. Simply each target that is in the compilation_commands.json.

The patch for that: F11185980: 0001-Refactor-automatically-declare-everything-const.patch <https://reviews.llvm.org/F11185980>
That is `3697 files changed, 164'596 insertions(+), 164'596 deletions(-)`

This patch unfortunatly does not compile cleanly anymore. To fix LLVM up I had to manually fix the following things: F11186007: 0002-fixup-wrong-transformations.patch <https://reviews.llvm.org/F11186007>
These are 34 places that create compilation failures, not all of them are to blame on the analysis/transformation.

The only actual deficits in analysis seem to be:

- overloaded operators for templated classes that are unresolved
- calls to methods that are called through captured `this` im lambda expression within templated function

These are ~4 instances in all of LLVM.

- `MyType *&  my_var = foo(); *(my_var + 3) = 42;` the pointer would be qualified const, because the pointee-analysis does not exist. By default pointers will not be marked as `const`. References to pointers should probably be excluded as well.

Other problems are:

- the class does not provide a userdefined default constructor, after applying const there is a `MyType const my_var -->{}<--;` missing -> can be added with clang-tidy as well
- ternary operator has a `const` type for `true`, and non-const for `false`
- `MyType const my_var; decltype(my_var);` stops working
- in one test a type-trait that included a `decltype` stopped working, probably same problem as previous point

I do not consider the latter issues to actually be an issue in the analysis/transformation. They are very seldom and for such cases `NOLINT` is ok in my opinion.

This leads to LLVM compiling cleanly.

  $ ninja check-all
  >
  > ********************

  Testing Time: 442.99s
  ********************
  Failing Tests (9):
      LLVM-Unit :: ExecutionEngine/Orc/./OrcJITTests/CoreAPIsStandardTest.TestBasicAliases
      LLVM-Unit :: ExecutionEngine/Orc/./OrcJITTests/CoreAPIsStandardTest.TestBasicReExports
      LLVM-Unit :: ExecutionEngine/Orc/./OrcJITTests/CoreAPIsStandardTest.TestChainedAliases
      LLVM-Unit :: ExecutionEngine/Orc/./OrcJITTests/CoreAPIsStandardTest.TestReexportsGenerator
      LLVM-Unit :: ExecutionEngine/Orc/./OrcJITTests/CoreAPIsStandardTest.TestThatReExportsDontUnnecessarilyMaterialize
      LLVM :: ExecutionEngine/OrcLazy/common-symbols.ll
      LLVM :: ExecutionEngine/OrcLazy/global-ctors-and-dtors.ll
      LLVM :: ExecutionEngine/OrcLazy/global_aliases.ll
      LLVM :: ExecutionEngine/OrcLazy/hidden-visibility.ll

    Expected Passes    : 54838
    Expected Failures  : 175
    Unsupported Tests  : 531
    Unexpected Failures: 9
  FAILED: CMakeFiles/check-all

:(
There is an assertion-failure for `Orc`-stuff.

  --
  lli: /data/big/llvm-project/llvm/include/llvm/ExecutionEngine/Orc/SymbolStringPool.h:146: llvm::orc::SymbolStringPool::~SymbolStringPool(): Assertion `Pool.empty() && "Dangling references at pool destruction time"' failed.
  Stack dump:
  0.      Program arguments: /data/big/test-build/bin/lli -jit-kind=orc-lazy -extra-module /data/big/llvm-project/llvm/test/ExecutionEngine/OrcLazy/Inputs/hidden-definitions.ll /data/big/llvm-pr
  oject/llvm/test/ExecutionEngine/OrcLazy/hidden-visibility.ll
   #0 0x00007f8626fbbd4f llvm::sys::PrintStackTrace(llvm::raw_ostream&) /data/big/llvm-project/llvm/lib/Support/Unix/Signals.inc:548:13
   #1 0x00007f8626fb9fd0 llvm::sys::RunSignalHandlers() /data/big/llvm-project/llvm/lib/Support/Signals.cpp:69:18
   #2 0x00007f8626fbc155 SignalHandler(int) /data/big/llvm-project/llvm/lib/Support/Unix/Signals.inc:380:3
   #3 0x00007f86282ee770 __restore_rt (/lib64/libpthread.so.0+0x15770)
   #4 0x00007f86268cf3c1 raise (/lib64/libc.so.6+0x3a3c1)
   #5 0x00007f86268b755b abort (/lib64/libc.so.6+0x2255b)
   #6 0x00007f86268b742f (/lib64/libc.so.6+0x2242f)
   #7 0x00007f86268c6a92 (/lib64/libc.so.6+0x31a92)
   #8 0x00007f8628e34f72 std::mutex::lock() /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/include/g++-v9/bits/std_mutex.h:104:2
   #9 0x00007f8628e34f72 std::lock_guard<std::mutex>::lock_guard(std::mutex&) /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/include/g++-v9/bits/std_mutex.h:159:19
  #10 0x00007f8628e34f72 llvm::orc::SymbolStringPool::clearDeadEntries() /data/big/llvm-project/llvm/include/llvm/ExecutionEngine/Orc/SymbolStringPool.h:159:31
  #11 0x00007f8628e34f72 llvm::orc::SymbolStringPool::~SymbolStringPool() /data/big/llvm-project/llvm/include/llvm/ExecutionEngine/Orc/SymbolStringPool.h:145:3
  #12 0x00007f8628e34f72 void __gnu_cxx::new_allocator<llvm::orc::SymbolStringPool>::destroy<llvm::orc::SymbolStringPool>(llvm::orc::SymbolStringPool*) /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/inc
  lude/g++-v9/ext/new_allocator.h:153:10
  #13 0x00007f8628e34f72 void std::allocator_traits<std::allocator<llvm::orc::SymbolStringPool> >::destroy<llvm::orc::SymbolStringPool>(std::allocator<llvm::orc::SymbolStringPool>&, llvm::orc::S
  ymbolStringPool*) /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/include/g++-v9/bits/alloc_traits.h:497:8
  #14 0x00007f8628e34f72 std::_Sp_counted_ptr_inplace<llvm::orc::SymbolStringPool, std::allocator<llvm::orc::SymbolStringPool>, (__gnu_cxx::_Lock_policy)2>::_M_dispose() /usr/lib/gcc/x86_64-pc-l
  inux-gnu/9.2.0/include/g++-v9/bits/shared_ptr_base.h:557:2
  #15 0x00007f8628e587eb __gnu_cxx::__exchange_and_add_dispatch(int*, int) /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/include/g++-v9/ext/atomicity.h:81:9
  #16 0x00007f8628e587eb std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/include/g++-v9/bits/shared_ptr_base.h:167:10
  #17 0x00007f8628e587eb std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count() /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/include/g++-v9/bits/shared_ptr_base.h:730:11
  #18 0x00007f8628e587eb std::__shared_ptr<llvm::orc::SymbolStringPool, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr() /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/include/g++-v9/bits/shared_ptr_base.h:
  1169:31
  #19 0x00007f8628e587eb llvm::orc::ExecutionSession::~ExecutionSession() /data/big/llvm-project/llvm/include/llvm/ExecutionEngine/Orc/Core.h:1053:7
  #20 0x00007f8628e54f96 std::default_delete<llvm::orc::ExecutionSession>::operator()(llvm::orc::ExecutionSession*) const /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/include/g++-v9/bits/unique_ptr.h:
  81:2
  #21 0x00007f8628e54f96 std::unique_ptr<llvm::orc::ExecutionSession, std::default_delete<llvm::orc::ExecutionSession> >::~unique_ptr() /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/include/g++-v9/bits
  /unique_ptr.h:284:4
  #22 0x00007f8628e54f96 llvm::orc::LLJIT::~LLJIT() /data/big/llvm-project/llvm/lib/ExecutionEngine/Orc/LLJIT.cpp:55:1
  #23 0x000000000041c2f1 std::default_delete<llvm::orc::LLLazyJIT>::operator()(llvm::orc::LLLazyJIT*) const /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/include/g++-v9/bits/unique_ptr.h:81:2
  #24 0x000000000041c2f1 std::unique_ptr<llvm::orc::LLLazyJIT, std::default_delete<llvm::orc::LLLazyJIT> >::~unique_ptr() /usr/lib/gcc/x86_64-pc-linux-gnu/9.2.0/include/g++-v9/bits/unique_ptr.h:

But all failures seem to be this assertion.

Given this result (i mostly consider it a succes, even though the overloaded operators not working confuse me) I want to start cleaning up this patch and start committing e.g. formatting changes and everything that can be extracted from this patch.
What is currently missing are isolated unit-tests in `unittests/Analysis` for each new fixed code construct. They all live in the clang-tidy test which is suboptimal.
Once the cleaning is successfull I want to address this.

@aaron.ballman and others: what do you think of the quality of the analysis? Should this check be comittable functionality-wise?
IMHO it would really help to get some stuff into master

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943