[lld] r193029 - Fix bug that CompareAtoms::compare is not transitive.
Rui Ueyama
ruiu at google.com
Fri Oct 18 20:18:19 PDT 2013
Author: ruiu
Date: Fri Oct 18 22:18:18 2013
New Revision: 193029
URL: http://llvm.org/viewvc/llvm-project?rev=193029&view=rev
Log:
Fix bug that CompareAtoms::compare is not transitive.
This patch fixes a bug in r190608. The results of a comparison function
passed to std::sort must be transitive, which is, if a < b and b < c, and if
a != b, a < c must be also true. CompareAtoms::compare did not actually
guarantee the transitivity. As a result the sort results were sometimes just
wrong.
Consider there are three atoms, X, Y, and Z, whose file ordinals are 1, 2, 3,
respectively. Z has a property "layout-after X". In this case, all the
following conditionals become true:
X < Y because X's ordinal is less than Y's
Y < Z because Y's ordinal is less than Z's
Z < X because of the layout-after relationship
This is not of course transitive. The reason why this happened is because
we used follow-on relationships for comparison if two atoms falls in the same
follow-on chain, but we used each atom's properties if they did not. This patch
fixes the issue by using follow-on root atoms for comparison to get consistent
results.
Differential Revision: http://llvm-reviews.chandlerc.com/D1980
Added:
lld/trunk/test/core/layout-transitivity.objtxt
Modified:
lld/trunk/include/lld/Passes/LayoutPass.h
lld/trunk/lib/Passes/LayoutPass.cpp
Modified: lld/trunk/include/lld/Passes/LayoutPass.h
URL: http://llvm.org/viewvc/llvm-project/lld/trunk/include/lld/Passes/LayoutPass.h?rev=193029&r1=193028&r2=193029&view=diff
==============================================================================
--- lld/trunk/include/lld/Passes/LayoutPass.h (original)
+++ lld/trunk/include/lld/Passes/LayoutPass.h Fri Oct 18 22:18:18 2013
@@ -68,11 +68,6 @@ private:
// Build a map of Atoms to ordinals for sorting the atoms
void buildOrdinalOverrideMap(MutableFile::DefinedAtomRange &range);
-#ifndef NDEBUG
- // Check if the follow-on graph is a correct structure. For debugging only.
- void checkFollowonChain(MutableFile::DefinedAtomRange &range);
-#endif
-
typedef llvm::DenseMap<const DefinedAtom *, const DefinedAtom *> AtomToAtomT;
typedef llvm::DenseMap<const DefinedAtom *, uint64_t> AtomToOrdinalT;
@@ -96,6 +91,14 @@ private:
bool checkAllPrevAtomsZeroSize(const DefinedAtom *targetAtom);
void setChainRoot(const DefinedAtom *targetAtom, const DefinedAtom *root);
+
+#ifndef NDEBUG
+ // Check if the follow-on graph is a correct structure. For debugging only.
+ void checkFollowonChain(MutableFile::DefinedAtomRange &range);
+
+ typedef std::vector<const DefinedAtom *>::iterator DefinedAtomIter;
+ void checkTransitivity(DefinedAtomIter begin, DefinedAtomIter end) const;
+#endif
};
} // namespace lld
Modified: lld/trunk/lib/Passes/LayoutPass.cpp
URL: http://llvm.org/viewvc/llvm-project/lld/trunk/lib/Passes/LayoutPass.cpp?rev=193029&r1=193028&r2=193029&view=diff
==============================================================================
--- lld/trunk/lib/Passes/LayoutPass.cpp (original)
+++ lld/trunk/lib/Passes/LayoutPass.cpp Fri Oct 18 22:18:18 2013
@@ -28,6 +28,19 @@ std::string formatReason(StringRef reaso
Twine(reason) + " (" + Twine(leftVal) + ", " + Twine(rightVal) + ")";
return std::move(msg.str());
}
+} // end anonymous namespace
+
+// Less-than relationship of two atoms must be transitive, which is, if a < b
+// and b < c, a < c must be true. This function checks the transitivity by
+// checking the sort results.
+void LayoutPass::checkTransitivity(DefinedAtomIter begin,
+ DefinedAtomIter end) const {
+ for (DefinedAtomIter i = begin; (i + 1) != end; ++i) {
+ for (DefinedAtomIter j = i + 1; j != end; ++j) {
+ assert(_compareAtoms(*i, *j));
+ assert(!_compareAtoms(*j, *i));
+ }
+ }
}
#endif // NDEBUG
@@ -60,27 +73,27 @@ bool LayoutPass::CompareAtoms::compare(c
}
}
- AtomToOrdinalT::const_iterator lPos = _layout._ordinalOverrideMap.find(left);
- AtomToOrdinalT::const_iterator rPos = _layout._ordinalOverrideMap.find(right);
- AtomToOrdinalT::const_iterator end = _layout._ordinalOverrideMap.end();
+ // Find the root of the chain if it is a part of a follow-on chain.
+ auto leftFind = _layout._followOnRoots.find(left);
+ auto rightFind = _layout._followOnRoots.find(right);
+ const DefinedAtom *leftRoot =
+ (leftFind == _layout._followOnRoots.end()) ? left : leftFind->second;
+ const DefinedAtom *rightRoot =
+ (rightFind == _layout._followOnRoots.end()) ? right : rightFind->second;
// Sort atoms by their ordinal overrides only if they fall in the same
// chain.
- auto leftAtom = _layout._followOnRoots.find(left);
- auto rightAtom = _layout._followOnRoots.find(right);
-
- if (leftAtom != _layout._followOnRoots.end() &&
- rightAtom != _layout._followOnRoots.end() &&
- leftAtom->second == rightAtom->second) {
- if ((lPos != end) && (rPos != end)) {
- DEBUG(reason = formatReason("override", lPos->second, rPos->second));
- return lPos->second < rPos->second;
- }
+ AtomToOrdinalT::const_iterator lPos = _layout._ordinalOverrideMap.find(left);
+ AtomToOrdinalT::const_iterator rPos = _layout._ordinalOverrideMap.find(right);
+ AtomToOrdinalT::const_iterator end = _layout._ordinalOverrideMap.end();
+ if (leftRoot == rightRoot && lPos != end && rPos != end) {
+ DEBUG(reason = formatReason("override", lPos->second, rPos->second));
+ return lPos->second < rPos->second;
}
// Sort same permissions together.
- DefinedAtom::ContentPermissions leftPerms = left->permissions();
- DefinedAtom::ContentPermissions rightPerms = right->permissions();
+ DefinedAtom::ContentPermissions leftPerms = leftRoot->permissions();
+ DefinedAtom::ContentPermissions rightPerms = rightRoot->permissions();
if (leftPerms != rightPerms) {
DEBUG(reason =
@@ -89,8 +102,8 @@ bool LayoutPass::CompareAtoms::compare(c
}
// Sort same content types together.
- DefinedAtom::ContentType leftType = left->contentType();
- DefinedAtom::ContentType rightType = right->contentType();
+ DefinedAtom::ContentType leftType = leftRoot->contentType();
+ DefinedAtom::ContentType rightType = rightRoot->contentType();
if (leftType != rightType) {
DEBUG(reason = formatReason("contentType", (int)leftType, (int)rightType));
@@ -98,8 +111,8 @@ bool LayoutPass::CompareAtoms::compare(c
}
// Sort by .o order.
- const File *leftFile = &left->file();
- const File *rightFile = &right->file();
+ const File *leftFile = &leftRoot->file();
+ const File *rightFile = &rightRoot->file();
if (leftFile != rightFile) {
DEBUG(reason = formatReason(".o order", (int)leftFile->ordinal(),
@@ -108,12 +121,12 @@ bool LayoutPass::CompareAtoms::compare(c
}
// Sort by atom order with .o file.
- uint64_t leftOrdinal = left->ordinal();
- uint64_t rightOrdinal = right->ordinal();
+ uint64_t leftOrdinal = leftRoot->ordinal();
+ uint64_t rightOrdinal = rightRoot->ordinal();
if (leftOrdinal != rightOrdinal) {
- DEBUG(reason = formatReason("ordinal", (int)left->ordinal(),
- (int)right->ordinal()));
+ DEBUG(reason = formatReason("ordinal", (int)leftRoot->ordinal(),
+ (int)rightRoot->ordinal()));
return leftOrdinal < rightOrdinal;
}
@@ -548,6 +561,8 @@ void LayoutPass::perform(MutableFile &me
// sort the atoms
std::sort(atomRange.begin(), atomRange.end(), _compareAtoms);
+ DEBUG(checkTransitivity(atomRange.begin(), atomRange.end()));
+
DEBUG({
llvm::dbgs() << "sorted atoms:\n";
printDefinedAtoms(atomRange);
Added: lld/trunk/test/core/layout-transitivity.objtxt
URL: http://llvm.org/viewvc/llvm-project/lld/trunk/test/core/layout-transitivity.objtxt?rev=193029&view=auto
==============================================================================
--- lld/trunk/test/core/layout-transitivity.objtxt (added)
+++ lld/trunk/test/core/layout-transitivity.objtxt Fri Oct 18 22:18:18 2013
@@ -0,0 +1,33 @@
+# RUN: lld -core --add-pass layout -mllvm -debug %s 2> /dev/null | FileCheck %s
+
+---
+defined-atoms:
+ - name: fn3
+ scope: global
+ - name: fn2
+ scope: global
+ references:
+ - kind: layout-after
+ offset: 0
+ target: fn3
+ - name: fn
+ scope: global
+ references:
+ - kind: layout-after
+ offset: 0
+ target: fn1
+ - name: fn4
+ scope: global
+ - name: fn1
+ scope: global
+ references:
+ - kind: layout-after
+ offset: 0
+ target: fn2
+...
+
+# CHECK: - name: fn
+# CHECK: - name: fn1
+# CHECK: - name: fn2
+# CHECK: - name: fn3
+# CHECK: - name: fn4
More information about the llvm-commits
mailing list