Skip to content

[lldb] Fix SBTarget::ReadInstruction with flavor #136034

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 25, 2025

Conversation

da-viper
Copy link
Contributor

Backported #134626 to 20.X

@da-viper da-viper requested a review from JDevlieghere as a code owner April 16, 2025 21:15
@llvmbot llvmbot added the lldb label Apr 16, 2025
@llvmbot
Copy link
Member

llvmbot commented Apr 16, 2025

@llvm/pr-subscribers-lldb

Author: Ebuka Ezike (da-viper)

Changes

Backported #134626 to 20.X


Full diff: https://github.com/llvm/llvm-project/pull/136034.diff

4 Files Affected:

  • (modified) lldb/source/API/SBTarget.cpp (+3-3)
  • (added) lldb/test/API/python_api/target/read-instructions-flavor/Makefile (+3)
  • (added) lldb/test/API/python_api/target/read-instructions-flavor/TestTargetReadInstructionsFlavor.py (+40)
  • (added) lldb/test/API/python_api/target/read-instructions-flavor/main.c (+21)
diff --git a/lldb/source/API/SBTarget.cpp b/lldb/source/API/SBTarget.cpp
index 2a33161bc21ed..665b903aff79d 100644
--- a/lldb/source/API/SBTarget.cpp
+++ b/lldb/source/API/SBTarget.cpp
@@ -2020,9 +2020,9 @@ lldb::SBInstructionList SBTarget::ReadInstructions(lldb::SBAddress base_addr,
                                 error, force_live_memory, &load_addr);
       const bool data_from_file = load_addr == LLDB_INVALID_ADDRESS;
       sb_instructions.SetDisassembler(Disassembler::DisassembleBytes(
-          target_sp->GetArchitecture(), nullptr, target_sp->GetDisassemblyCPU(),
-          target_sp->GetDisassemblyFeatures(), flavor_string, *addr_ptr,
-          data.GetBytes(), bytes_read, count, data_from_file));
+          target_sp->GetArchitecture(), nullptr, flavor_string,
+          target_sp->GetDisassemblyCPU(), target_sp->GetDisassemblyFeatures(),
+          *addr_ptr, data.GetBytes(), bytes_read, count, data_from_file));
     }
   }
 
diff --git a/lldb/test/API/python_api/target/read-instructions-flavor/Makefile b/lldb/test/API/python_api/target/read-instructions-flavor/Makefile
new file mode 100644
index 0000000000000..10495940055b6
--- /dev/null
+++ b/lldb/test/API/python_api/target/read-instructions-flavor/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules
diff --git a/lldb/test/API/python_api/target/read-instructions-flavor/TestTargetReadInstructionsFlavor.py b/lldb/test/API/python_api/target/read-instructions-flavor/TestTargetReadInstructionsFlavor.py
new file mode 100644
index 0000000000000..12805985798de
--- /dev/null
+++ b/lldb/test/API/python_api/target/read-instructions-flavor/TestTargetReadInstructionsFlavor.py
@@ -0,0 +1,40 @@
+"""
+Test SBTarget Read Instruction.
+"""
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+
+
+class TargetReadInstructionsFlavor(TestBase):
+    @skipIfWindows
+    @skipIf(archs=no_match(["x86_64", "x86", "i386"]))
+    def test_read_instructions_with_flavor(self):
+        self.build()
+        executable = self.getBuildArtifact("a.out")
+
+        # create a target
+        target = self.dbg.CreateTarget(executable)
+        self.assertTrue(target.IsValid(), "target is not valid")
+
+        functions = target.FindFunctions("test_add")
+        self.assertEqual(len(functions), 1)
+        test_add = functions[0]
+
+        test_add_symbols = test_add.GetSymbol()
+        self.assertTrue(
+            test_add_symbols.IsValid(), "test_add function symbols is not valid"
+        )
+
+        expected_instructions = (("mov", "eax, edi"), ("add", "eax, esi"), ("ret", ""))
+        test_add_insts = test_add_symbols.GetInstructions(target, "intel")
+        # clang adds an extra nop instruction but gcc does not. It makes more sense
+        # to check if it is at least 3
+        self.assertLessEqual(len(expected_instructions), len(test_add_insts))
+
+        # compares only the expected instructions
+        for expected_instr, instr in zip(expected_instructions, test_add_insts):
+            self.assertTrue(instr.IsValid(), "instruction is not valid")
+            expected_mnemonic, expected_op_str = expected_instr
+            self.assertEqual(instr.GetMnemonic(target), expected_mnemonic)
+            self.assertEqual(instr.GetOperands(target), expected_op_str)
diff --git a/lldb/test/API/python_api/target/read-instructions-flavor/main.c b/lldb/test/API/python_api/target/read-instructions-flavor/main.c
new file mode 100644
index 0000000000000..6022d63fb6ed7
--- /dev/null
+++ b/lldb/test/API/python_api/target/read-instructions-flavor/main.c
@@ -0,0 +1,21 @@
+
+// This simple program is to test the lldb Python API SBTarget ReadInstruction
+// function.
+//
+// When the target is create we get all the instructions using the intel
+// flavor and see if it is correct.
+
+int test_add(int a, int b);
+
+__asm__("test_add:\n"
+        "    movl    %edi, %eax\n"
+        "    addl    %esi, %eax\n"
+        "    ret     \n");
+
+int main(int argc, char **argv) {
+  int a = 10;
+  int b = 20;
+  int result = test_add(a, b);
+
+  return 0;
+}
\ No newline at end of file

@github-project-automation github-project-automation bot moved this from Needs Triage to Needs Merge in LLVM Release Status Apr 19, 2025
@da-viper
Copy link
Contributor Author

@JDevlieghere I do not have the right permission to merge this to the 20.X branch as it is protected.

@DavidSpickett
Copy link
Collaborator

One of the release managers will do that for us at some point.

da-viper and others added 2 commits April 25, 2025 16:35
When you call the `SBTarget::ReadInstructions` with flavor from lldb
crashes. This is because the wrong order of the `DisassemblyBytes`
constructor this fixes that

---------

Signed-off-by: Ebuka Ezike <[email protected]>
Original in llvm#134626 was
written as if it was "this or this" but it's "this and this".

So the test ran on AArch64 Linux, because Linux is not Windows.

Split out the Windows check to fix that.
@tstellar tstellar force-pushed the cherry-pick-read-instr branch from d937fc0 to 8f288eb Compare April 25, 2025 23:36
@tstellar tstellar merged commit 8f288eb into llvm:release/20.x Apr 25, 2025
6 of 9 checks passed
@github-project-automation github-project-automation bot moved this from Needs Merge to Done in LLVM Release Status Apr 25, 2025
Copy link

@da-viper (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR.

@da-viper
Copy link
Contributor Author

Fix crash when calling SBTarget::GetInstruction() with the flavor parameter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

6 participants