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>
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>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
