llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-lldb
Author: Yao Qi (qiyao)
<details>
<summary>Changes</summary>
`target create` never returns for a little-endian 32-bit MIPS ELF file,
on any host whose platform advertises `armv7m`/`armv7em` compatibility.
On Darwin that is every platform, since `PlatformDarwin::GetCompatibleArchs()`
lists both for every arm64/arm64e host and falls through to that same list
for any other host core. It is found by `lldb-target-fuzzer`.
Reproduce with a 52-byte ELF32 header (`ELFCLASS32`, `ELFDATA2LSB`,
`e_machine = EM_MIPS`, `e_flags = EF_MIPS_ARCH_32`, everything else zero),
saved as `mips32el.elf`:
```
(lldb) target create mips32el.elf
```
This never returns. Attaching a debugger to the hung process shows the
same three frames looping forever, with the call stack never growing:
```
frame #<!-- -->1: cores_match(core1=eCore_arm_armv7m, core2=eCore_mips32el,
try_inverse=false, ...) at ArchSpec.cpp:1575 [inlined]
frame #<!-- -->2: ArchSpec::IsMatch(rhs=..., match=CompatibleMatch) at
ArchSpec.cpp:1449
frame #<!-- -->3: Platform::IsCompatibleArchitecture(...) at Platform.cpp:1193
```
`cores_match()` checks `core1` against `core2`. When nothing matches and
the caller did not ask for an exact match, it sets `try_inverse` and
retries once with the two cores swapped. It retries because the rule
tables are not symmetric: a core lists only what it accepts, not everything
that accepts it.
Every case that reaches this fallback must leave `try_inverse` false on the
way out, so the swap happens at most once. Three cases,
`eCore_arm_armv7em`, `eCore_arm_armv7m`, and `eCore_mips32el`, instead set
it back to true unconditionally. When both sides of a comparison are one
of these three, the swapped call re-arms `try_inverse`, and the tail call
back into `cores_match()` recurses forever.
This is a tail call, so an optimizing compiler turns the recursion into a
loop instead of a growing stack. That is why the process spins instead of
crashing with a stack overflow.
The three cases cannot simply leave `try_inverse` false like their
siblings. `ArchSpec("mipsel").IsCompatibleMatch(ArchSpec("mips64el"))` is
true today only because of this same swap: `eCore_mips32el`'s own rule says
nothing about 64-bit MIPS, but `eCore_mips64el`'s rule accepts any 32-bit
MIPS core with the same endianness, and only the inverted comparison
reaches that rule.
Instead, split the switch into a one-directional helper that never
recurses, and try both directions from a plain, non-recursive wrapper:
```
cores_match(A, B) := cores_match_one_direction(A, B) ||
cores_match_one_direction(B, A)
```
This keeps every existing one-directional rule exactly as it was, including
the mips32el/mips64el rule above, and makes a second retry impossible no
matter what any case does.
Adds two regression tests to ArchSpecTest.cpp. One asserts that the
previously-hanging pairs now return false right away. The other locks in
the two relationships that depend on checking both directions, so a future
change cannot bring the recursion back by "fixing" the three cases the
naive way.
Assisted-by: claude
---
Full diff: https://github.com/llvm/llvm-project/pull/221899.diff
2 Files Affected:
- (modified) lldb/source/Utility/ArchSpec.cpp (+17-25)
- (modified) lldb/unittests/Utility/ArchSpecTest.cpp (+40)
``````````diff
diff --git a/lldb/source/Utility/ArchSpec.cpp b/lldb/source/Utility/ArchSpec.cpp
index 0c9202e3b5e43..af2299e3c7480 100644
--- a/lldb/source/Utility/ArchSpec.cpp
+++ b/lldb/source/Utility/ArchSpec.cpp
@@ -25,7 +25,7 @@ using namespace lldb;
using namespace lldb_private;
static bool cores_match(const ArchSpec::Core core1, const ArchSpec::Core core2,
- bool try_inverse, bool enforce_exact_match);
+ bool enforce_exact_match);
namespace lldb_private {
@@ -1446,7 +1446,7 @@ static bool
IsCompatibleEnvironment(llvm::Triple::EnvironmentType lhs,
bool ArchSpec::IsMatch(const ArchSpec &rhs, MatchType match) const {
if (GetByteOrder() != rhs.GetByteOrder() ||
- !cores_match(GetCore(), rhs.GetCore(), true, match == ExactMatch))
+ !cores_match(GetCore(), rhs.GetCore(), match == ExactMatch))
return false;
const llvm::Triple &lhs_triple = GetTriple();
@@ -1570,11 +1570,12 @@ void ArchSpec::CoreUpdated(bool update_triple) {
//===----------------------------------------------------------------------===//
// Operators.
-static bool cores_match(const ArchSpec::Core core1, const ArchSpec::Core core2,
- bool try_inverse, bool enforce_exact_match) {
- if (core1 == core2)
- return true;
-
+// Checks core1's rules only, because they are not symmetric. Must not call
+// cores_match() here, or an unmatched pair would loop between the two
+// directions forever.
+static bool cores_match_one_direction(const ArchSpec::Core core1,
+ const ArchSpec::Core core2,
+ bool enforce_exact_match) {
switch (core1) {
case ArchSpec::kCore_any:
return true;
@@ -1642,7 +1643,6 @@ static bool cores_match(const ArchSpec::Core core1, const
ArchSpec::Core core2,
return true;
if (core2 == ArchSpec::eCore_arm_armv7)
return true;
- try_inverse = true;
}
break;
@@ -1660,7 +1660,6 @@ static bool cores_match(const ArchSpec::Core core1, const
ArchSpec::Core core2,
return true;
if (core2 == ArchSpec::eCore_arm_armv7em)
return true;
- try_inverse = true;
}
break;
@@ -1678,7 +1677,6 @@ static bool cores_match(const ArchSpec::Core core1, const
ArchSpec::Core core2,
return true;
if (core2 == ArchSpec::eCore_arm_armv6m)
return true;
- try_inverse = false;
}
break;
@@ -1692,14 +1690,12 @@ static bool cores_match(const ArchSpec::Core core1,
const ArchSpec::Core core2,
return true;
if (core2 == ArchSpec::eCore_arm_armv7)
return true;
- try_inverse = false;
}
break;
case ArchSpec::eCore_x86_64_x86_64h:
case ArchSpec::eCore_x86_64_amd64:
if (!enforce_exact_match) {
- try_inverse = false;
if (core2 == ArchSpec::eCore_x86_64_x86_64)
return true;
}
@@ -1713,7 +1709,6 @@ static bool cores_match(const ArchSpec::Core core1, const
ArchSpec::Core core2,
return true;
if (core2 == ArchSpec::eCore_arm_arm64e)
return true;
- try_inverse = false;
}
break;
@@ -1725,7 +1720,6 @@ static bool cores_match(const ArchSpec::Core core1, const
ArchSpec::Core core2,
return true;
if (core2 == ArchSpec::eCore_arm_armv8)
return true;
- try_inverse = false;
}
break;
case ArchSpec::eCore_arm_aarch64:
@@ -1736,7 +1730,6 @@ static bool cores_match(const ArchSpec::Core core1, const
ArchSpec::Core core2,
return true;
if (core2 == ArchSpec::eCore_arm_arm64e)
return true;
- try_inverse = false;
}
break;
@@ -1748,7 +1741,6 @@ static bool cores_match(const ArchSpec::Core core1, const
ArchSpec::Core core2,
return true;
if (core2 == ArchSpec::eCore_arm_arm64e)
return true;
- try_inverse = false;
}
break;
@@ -1756,7 +1748,6 @@ static bool cores_match(const ArchSpec::Core core1, const
ArchSpec::Core core2,
if (!enforce_exact_match) {
if (core2 == ArchSpec::eCore_arm_generic)
return true;
- try_inverse = false;
}
break;
@@ -1765,7 +1756,6 @@ static bool cores_match(const ArchSpec::Core core1, const
ArchSpec::Core core2,
if (core2 >= ArchSpec::kCore_mips32_first &&
core2 <= ArchSpec::kCore_mips32_last)
return true;
- try_inverse = false;
}
break;
@@ -1774,7 +1764,6 @@ static bool cores_match(const ArchSpec::Core core1, const
ArchSpec::Core core2,
if (core2 >= ArchSpec::kCore_mips32el_first &&
core2 <= ArchSpec::kCore_mips32el_last)
return true;
- try_inverse = true;
}
break;
@@ -1786,7 +1775,6 @@ static bool cores_match(const ArchSpec::Core core1, const
ArchSpec::Core core2,
if (core2 >= ArchSpec::kCore_mips64_first &&
core2 <= ArchSpec::kCore_mips64_last)
return true;
- try_inverse = false;
}
break;
@@ -1798,7 +1786,6 @@ static bool cores_match(const ArchSpec::Core core1, const
ArchSpec::Core core2,
if (core2 >= ArchSpec::kCore_mips64el_first &&
core2 <= ArchSpec::kCore_mips64el_last)
return true;
- try_inverse = false;
}
break;
@@ -1810,7 +1797,6 @@ static bool cores_match(const ArchSpec::Core core1, const
ArchSpec::Core core2,
return true;
if (core2 >= ArchSpec::kCore_mips64_first && core2 <= (core1 - 1))
return true;
- try_inverse = false;
}
break;
@@ -1822,7 +1808,6 @@ static bool cores_match(const ArchSpec::Core core1, const
ArchSpec::Core core2,
return true;
if (core2 >= ArchSpec::kCore_mips64el_first && core2 <= (core1 - 1))
return true;
- try_inverse = false;
}
break;
@@ -1882,11 +1867,18 @@ static bool cores_match(const ArchSpec::Core core1,
const ArchSpec::Core core2,
default:
break;
}
- if (try_inverse)
- return cores_match(core2, core1, false, enforce_exact_match);
return false;
}
+// The match rules only work in one direction, so this checks both ways.
+static bool cores_match(const ArchSpec::Core core1, const ArchSpec::Core core2,
+ bool enforce_exact_match) {
+ if (core1 == core2)
+ return true;
+ return cores_match_one_direction(core1, core2, enforce_exact_match) ||
+ cores_match_one_direction(core2, core1, enforce_exact_match);
+}
+
bool lldb_private::operator<(const ArchSpec &lhs, const ArchSpec &rhs) {
const ArchSpec::Core lhs_core = lhs.GetCore();
const ArchSpec::Core rhs_core = rhs.GetCore();
diff --git a/lldb/unittests/Utility/ArchSpecTest.cpp
b/lldb/unittests/Utility/ArchSpecTest.cpp
index 7027c7648a273..25ce85dd7f9f7 100644
--- a/lldb/unittests/Utility/ArchSpecTest.cpp
+++ b/lldb/unittests/Utility/ArchSpecTest.cpp
@@ -482,6 +482,46 @@ TEST(ArchSpecTest, Compatibility) {
}
}
+TEST(ArchSpecTest, UnrelatedCoresCompatibleMatchTerminates) {
+ {
+ ArchSpec A("mipsel-unknown-linux");
+ ArchSpec B("armv7em-apple-none");
+ ASSERT_FALSE(A.IsCompatibleMatch(B));
+ ASSERT_FALSE(B.IsCompatibleMatch(A));
+ }
+ {
+ ArchSpec A("mipsel-unknown-linux");
+ ArchSpec B("armv7m-apple-none");
+ ASSERT_FALSE(A.IsCompatibleMatch(B));
+ ASSERT_FALSE(B.IsCompatibleMatch(A));
+ }
+}
+
+TEST(ArchSpecTest, AsymmetricCoreRulesAreCheckedBothWays) {
+ // Some cores' compatibility rules only work one way: core A accepts B,
+ // but B's own rule says nothing about A. A compatible match must check
+ // both directions to find these cases.
+ {
+ // A 32-bit MIPS core has no rule about 64-bit MIPS, but 64-bit MIPS
+ // accepts the 32-bit family with the same endianness.
+ ArchSpec A("mipsel-unknown-linux");
+ ArchSpec B("mips64el-unknown-linux");
+ ASSERT_TRUE(A.IsCompatibleMatch(B));
+ ASSERT_TRUE(B.IsCompatibleMatch(A));
+ ASSERT_FALSE(A.IsExactMatch(B));
+ ASSERT_FALSE(B.IsExactMatch(A));
+ }
+ {
+ // The two Cortex-M cores explicitly accept each other.
+ ArchSpec A("armv7em-apple-none");
+ ArchSpec B("armv7m-apple-none");
+ ASSERT_TRUE(A.IsCompatibleMatch(B));
+ ASSERT_TRUE(B.IsCompatibleMatch(A));
+ ASSERT_FALSE(A.IsExactMatch(B));
+ ASSERT_FALSE(B.IsExactMatch(A));
+ }
+}
+
TEST(ArchSpecTest, WasmCompatibility) {
// A Wasm module encodes no vendor or OS: those are properties of the runtime
// executing it. A bare wasm32 or wasm64 architecture therefore has to stay
``````````
</details>
https://github.com/llvm/llvm-project/pull/221899
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
