[lldb] Fix SBTarget::ReadInstruction with flavor

[da-viper]

When you call the SBTarget::ReadInstructions with flavor from lldb crashes. This is because the wrong order of the DisassemblyBytes constructor this fixes that

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Ebuka Ezike (da-viper)

Changes

When you call the SBTarget::ReadInstructions with flavor from lldb crashes. This is because the wrong order of the DisassemblyBytes constructor this fixes that

---

Full diff: https://github.com/llvm/llvm-project/pull/134626.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 0fed1bbfed6a7..b42ada42b0931 100644
--- a/lldb/source/API/SBTarget.cpp
+++ b/lldb/source/API/SBTarget.cpp
@@ -2021,9 +2021,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..e93c8476d8e00
--- /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):
+
+    @skipIf(archs=no_match(["x86_64", "x86", "i386"]), oslist=["windows"])
+    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-actions]

✅ With the latest revision this PR passed the Python code formatter.

[jimingham]

LGTM, thanks for fixing this!

[da-viper]

I was also wondering if this should be backported as it also affects llvm 20.x branch

[JDevlieghere]

I was also wondering if this should be backported as it also affects llvm 20.x branch

Given its low risk I'm okay back porting this, if you think it's important. That said, I haven't seen any other reports so I'm guessing this crash isn't particularly widespread.

[da-viper]

I think it should be.
Most distributions are still on llvm 19 as llvm 20.1 was released last month. Maybe that's why there haven't been any reports.

This only affects the 20 and 21 branches.

[da-viper]

/cherry-pick 6c8b0a3 4be5e33 7417e79

[llvmbot]

Failed to cherry-pick: 6c8b0a3

https://github.com/llvm/llvm-project/actions/runs/14400492146

Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request

[da-viper]

separate it like this then it does not and it.

    @skipIf(archs=no_match(["x86_64", "x86", "i386"]))
    @skipIfWindows

[da-viper]

@DavidSpickett is there a way to run the merge build bot before merging to check if the test passes on other platforms ?

[DavidSpickett]

At this time no, it's a feature I'd like to have one day. My advice would have been to land it and see what happens :)

Unless we had pre-commit Windows lldb, but I think right now it's only Linux and it gets disabled some times due to instability.

It's fixed for AArch64 Linux at least - https://lab.llvm.org/buildbot/#/builders/59/builds/15858.

[ilovepi]

I think we're seeing this persist on x86_64 mac bots https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/lldb-mac-x64/b8717871333718166241/infra I'm not sure what the right spelling is to opt out.. I think @DavidSpickett's original suggestion may work better if we need to opt out more than just windows.

[da-viper]

Macos x86_64 also follows the systemV amd64 abi calling convention. So it should not fail or be skipped.

In the linked build bot, I could not see the test (lldb/test/API/python_api/target/read-instructions-flavor/TestTargetReadInstructionsFlavor.py) failing could you link it just to confirm.

[ilovepi]

@da-viper Ah, shoot. Sorry for the noise, I think it may have been 2fd860c, which was just reverted. thanks for taking a look. :)

[da-viper]

/cherrypick dda53be 5b384c3