[PATCH] D116037: [clang-include-fixer] Fix incorrect ranking because of dangling references

[Danila Kutenin via Phabricator via cfe-commits]

danlark created this revision.
danlark added reviewers: alexfh, klimek.
Herald added subscribers: arphaman, mgrang.
danlark requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

SymbolAndSignals stores SymbolInfo which stores two std::strings. Then
the values are stored in a llvm::DenseMap<llvm::StringRef, double>. When
the sorting is happening, SymbolAndSignals are swapped and thus because
of small string optimization some strings may become invalid. This
results in incorrect ranking.

This was detected when running new std::sort algorithm against llvm
toolchain. This could have been prevented with running llvm::sort and
EXPENSIVE_CHECKS. Unfortunately, no sanitizer yelled.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116037

Files:
  clang-tools-extra/clang-include-fixer/SymbolIndexManager.cpp


Index: clang-tools-extra/clang-include-fixer/SymbolIndexManager.cpp
===================================================================
--- clang-tools-extra/clang-include-fixer/SymbolIndexManager.cpp
+++ clang-tools-extra/clang-include-fixer/SymbolIndexManager.cpp
@@ -8,8 +8,9 @@
 
 #include "SymbolIndexManager.h"
 #include "find-all-symbols/SymbolInfo.h"
-#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Path.h"
 
@@ -47,7 +48,7 @@
 
 static void rank(std::vector<SymbolAndSignals> &Symbols,
                  llvm::StringRef FileName) {
-  llvm::DenseMap<llvm::StringRef, double> Score;
+  llvm::StringMap<double> Score;
   for (const auto &Symbol : Symbols) {
     // Calculate a score from the similarity of the header the symbol is in
     // with the current file and the popularity of the symbol.
@@ -58,14 +59,14 @@
   }
   // Sort by the gathered scores. Use file name as a tie breaker so we can
   // deduplicate.
-  std::sort(Symbols.begin(), Symbols.end(),
-            [&](const SymbolAndSignals &A, const SymbolAndSignals &B) {
-              auto AS = Score[A.Symbol.getFilePath()];
-              auto BS = Score[B.Symbol.getFilePath()];
-              if (AS != BS)
-                return AS > BS;
-              return A.Symbol.getFilePath() < B.Symbol.getFilePath();
-            });
+  llvm::sort(Symbols.begin(), Symbols.end(),
+             [&](const SymbolAndSignals &A, const SymbolAndSignals &B) {
+               auto AS = Score[A.Symbol.getFilePath()];
+               auto BS = Score[B.Symbol.getFilePath()];
+               if (AS != BS)
+                 return AS > BS;
+               return A.Symbol.getFilePath() < B.Symbol.getFilePath();
+             });
 }
 
 std::vector<find_all_symbols::SymbolInfo>


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D116037.395425.patch
Type: text/x-patch
Size: 1884 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20211220/6df34fc8/attachment.bin>