Replace ArchSpec::PiecewiseCompare() with Triple::operator==() #82804
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: Adrian Prantl (adrian-prantl) ChangesLooking ast the definition of both functions this is almost an NFC change, except that Triple also looks at the SubArch (important) and ObjectFormat (less so). This fixes a bug that only manifests with how Xcode uses the SBAPI to attach to a process by name: it guesses the architecture based on the system. If the system is arm64 and the Process is arm64e Target fails to update the triple because it deemed the two to be equivalent. rdar://123338218 Full diff: https://github.com/llvm/llvm-project/pull/82804.diff 6 Files Affected:
diff --git a/lldb/include/lldb/Utility/ArchSpec.h b/lldb/include/lldb/Utility/ArchSpec.h
index a226a3a5a9b71d..50830b889b9115 100644
--- a/lldb/include/lldb/Utility/ArchSpec.h
+++ b/lldb/include/lldb/Utility/ArchSpec.h
@@ -505,11 +505,6 @@ class ArchSpec {
bool IsFullySpecifiedTriple() const;
- void PiecewiseTripleCompare(const ArchSpec &other, bool &arch_different,
- bool &vendor_different, bool &os_different,
- bool &os_version_different,
- bool &env_different) const;
-
/// Detect whether this architecture uses thumb code exclusively
///
/// Some embedded ARM chips (e.g. the ARM Cortex M0-7 line) can only execute
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index e17bfcb5d5e2ad..e982a30a3ae4ff 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -1568,14 +1568,8 @@ bool Target::SetArchitecture(const ArchSpec &arch_spec, bool set_platform,
if (m_arch.GetSpec().IsCompatibleMatch(other)) {
compatible_local_arch = true;
- bool arch_changed, vendor_changed, os_changed, os_ver_changed,
- env_changed;
- m_arch.GetSpec().PiecewiseTripleCompare(other, arch_changed,
- vendor_changed, os_changed,
- os_ver_changed, env_changed);
-
- if (!arch_changed && !vendor_changed && !os_changed && !env_changed)
+ if (m_arch.GetSpec().GetTriple() == other.GetTriple())
replace_local_arch = false;
}
}
diff --git a/lldb/source/Utility/ArchSpec.cpp b/lldb/source/Utility/ArchSpec.cpp
index fb0e985a0d5657..07ef435ef451d2 100644
--- a/lldb/source/Utility/ArchSpec.cpp
+++ b/lldb/source/Utility/ArchSpec.cpp
@@ -1421,23 +1421,6 @@ bool ArchSpec::IsFullySpecifiedTriple() const {
return true;
}
-void ArchSpec::PiecewiseTripleCompare(
- const ArchSpec &other, bool &arch_different, bool &vendor_different,
- bool &os_different, bool &os_version_different, bool &env_different) const {
- const llvm::Triple &me(GetTriple());
- const llvm::Triple &them(other.GetTriple());
-
- arch_different = (me.getArch() != them.getArch());
-
- vendor_different = (me.getVendor() != them.getVendor());
-
- os_different = (me.getOS() != them.getOS());
-
- os_version_different = (me.getOSMajorVersion() != them.getOSMajorVersion());
-
- env_different = (me.getEnvironment() != them.getEnvironment());
-}
-
bool ArchSpec::IsAlwaysThumbInstructions() const {
std::string Status;
if (GetTriple().getArch() == llvm::Triple::arm ||
diff --git a/lldb/test/API/macosx/arm64e-attach/Makefile b/lldb/test/API/macosx/arm64e-attach/Makefile
new file mode 100644
index 00000000000000..c9319d6e6888a4
--- /dev/null
+++ b/lldb/test/API/macosx/arm64e-attach/Makefile
@@ -0,0 +1,2 @@
+C_SOURCES := main.c
+include Makefile.rules
diff --git a/lldb/test/API/macosx/arm64e-attach/TestArm64eAttach.py b/lldb/test/API/macosx/arm64e-attach/TestArm64eAttach.py
new file mode 100644
index 00000000000000..90daa54baa9d36
--- /dev/null
+++ b/lldb/test/API/macosx/arm64e-attach/TestArm64eAttach.py
@@ -0,0 +1,25 @@
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class TestArm64eAttach(TestBase):
+ NO_DEBUG_INFO_TESTCASE = True
+
+ # On Darwin systems, arch arm64e means ARMv8.3 with ptrauth ABI used.
+ @skipIf(archs=no_match(["arm64e"]))
+ def test(self):
+ self.build()
+ popen = self.spawnSubprocess(self.getBuildArtifact(), [])
+ error = lldb.SBError()
+ # This simulates how Xcode attaches to a process by pid/name.
+ target = self.dbg.CreateTarget("", "arm64", "", True, error)
+ listener = lldb.SBListener("my.attach.listener")
+ process = target.AttachToProcessWithID(listener, popen.pid, error)
+ self.assertSuccess(error)
+ self.assertTrue(process, PROCESS_IS_VALID)
+ self.assertEqual(target.GetTriple().split('-')[0], "arm64e",
+ "target triple is updated correctly")
+ error = process.Kill()
+ self.assertSuccess(error)
diff --git a/lldb/test/API/macosx/arm64e-attach/main.c b/lldb/test/API/macosx/arm64e-attach/main.c
new file mode 100644
index 00000000000000..7baf2ffd8f2899
--- /dev/null
+++ b/lldb/test/API/macosx/arm64e-attach/main.c
@@ -0,0 +1,2 @@
+int getchar();
+int main() { return getchar(); }
|
You can test this locally with the following command:darker --check --diff -r ae91a427ac8f9fc7368ec052995cec6a6aeb8ea8...d8e201683c460af42b671ecb433ba84620ada1c2 lldb/test/API/macosx/arm64e-attach/TestArm64eAttach.pyView the diff from darker here.--- TestArm64eAttach.py 2024-02-23 21:04:57.000000 +0000
+++ TestArm64eAttach.py 2024-02-23 21:07:40.795599 +0000
@@ -20,9 +20,12 @@
target = self.dbg.CreateTarget("", "arm64", "", True, error)
listener = lldb.SBListener("my.attach.listener")
process = target.AttachToProcessWithID(listener, popen.pid, error)
self.assertSuccess(error)
self.assertTrue(process, PROCESS_IS_VALID)
- self.assertEqual(target.GetTriple().split('-')[0], "arm64e",
- "target triple is updated correctly")
+ self.assertEqual(
+ target.GetTriple().split("-")[0],
+ "arm64e",
+ "target triple is updated correctly",
+ )
error = process.Kill()
self.assertSuccess(error)
|
Comment on lines
+1
to
+2
| C_SOURCES := main.c | ||
| include Makefile.rules |
There was a problem hiding this comment.
Did you want to build this test for arm64e?
There was a problem hiding this comment.
That is implicit. If you are running the tests with an arm64e ARCH, the test will run (with that triple).
| @skipIf(archs=no_match(["arm64e"])) | ||
| def test(self): | ||
| self.build() | ||
| popen = self.spawnSubprocess(self.getBuildArtifact(), []) |
There was a problem hiding this comment.
To run arm64e code you need to set a -arm64e_preview_abi boot arg. I'd be surprised if the bots have that set. You'll probably want to skip this test when this fails to launch.
There was a problem hiding this comment.
The bots will already fail at the archs=no_match(["arm64e"] decorator.
Looking ast the definition of both functions this is *almost* an NFC change, except that Triple also looks at the SubArch (important) and ObjectFormat (less so). This fixes a bug that only manifests with how Xcode uses the SBAPI to attach to a process by name: it guesses the architecture based on the system. If the system is arm64 and the Process is arm64e Target fails to update the triple because it deemed the two to be equivalent. rdar://123338218
adrian-prantl
force-pushed
the
123338218
branch
from
f1541ed to
d8e2016
Compare
JDevlieghere
approved these changes
Feb 23, 2024
adrian-prantl
added a commit
to adrian-prantl/llvm-project
that referenced
this pull request
Feb 23, 2024
…82804) Looking ast the definition of both functions this is *almost* an NFC change, except that Triple also looks at the SubArch (important) and ObjectFormat (less so). This fixes a bug that only manifests with how Xcode uses the SBAPI to attach to a process by name: it guesses the architecture based on the system. If the system is arm64 and the Process is arm64e Target fails to update the triple because it deemed the two to be equivalent. rdar://123338218 (cherry picked from commit 2594095)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Looking ast the definition of both functions this is almost an NFC change, except that Triple also looks at the SubArch (important) and ObjectFormat (less so).
This fixes a bug that only manifests with how Xcode uses the SBAPI to attach to a process by name: it guesses the architecture based on the system. If the system is arm64 and the Process is arm64e Target fails to update the triple because it deemed the two to be equivalent.
rdar://123338218