[lldb] Improve setting of program for filtering disassembly #148823
Merged
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
DavidSpickett
requested review from
tedwoodward
and removed request for
JDevlieghere
|
@llvm/pr-subscribers-backend-risc-v Author: David Spickett (DavidSpickett) ChangesThis changes the example command added in #145793 so that the fdis program does not have to be a single program name. Doing so also means we can run the test on Windows where the program needs to be "python.exe script_name". I've changed "fdis set" to treat the rest of the command as the program. Then store that as a list to be passed to subprocess. If we just use a string, Python will think that "python.exe foo" is the name of an actual program instead of a program and an argument to it. This will still break if the paths have spaces in, but I'm trying to do just enough to fix the test here without rewriting all the option handling. Full diff: https://github.com/llvm/llvm-project/pull/148823.diff 2 Files Affected:
diff --git a/lldb/examples/python/filter_disasm.py b/lldb/examples/python/filter_disasm.py
index de99d4031a7fd..797144460afbd 100644
--- a/lldb/examples/python/filter_disasm.py
+++ b/lldb/examples/python/filter_disasm.py
@@ -11,8 +11,13 @@
import lldb
import subprocess
-filter_program = "crustfilt"
+class Program(list):
+ def __str__(self):
+ return " ".join(self)
+
+
+filter_program = Program(["crustfilt"])
def __lldb_init_module(debugger, dict):
debugger.HandleCommand("command script add -f filter_disasm.fdis fdis")
@@ -51,13 +56,20 @@ def fdis(debugger, args, exe_ctx, result, dict):
result.Clear()
if len(args_list) == 1 and args_list[0] == "get":
- result.PutCString(filter_program)
+ result.PutCString(str(filter_program))
result.SetStatus(lldb.eReturnStatusSuccessFinishResult)
return
- if len(args_list) == 2 and args_list[0] == "set":
- filter_program = args_list[1]
- result.PutCString("Filter program set to %s" % filter_program)
+ if args_list[0] == "set":
+ # Assume the rest is a program to run, which might be a path containing spaces
+ # or an interpreter and a file path.
+ if len(args_list) <= 1:
+ result.PutCString('"set" command requires a program argument')
+ result.SetStatus(lldb.eReturnStatusFailed)
+ return
+
+ filter_program = Program(args_list[1:])
+ result.PutCString('Filter program set to "{}"'.format(filter_program))
result.SetStatus(lldb.eReturnStatusSuccessFinishResult)
return
@@ -70,7 +82,9 @@ def fdis(debugger, args, exe_ctx, result, dict):
output = res.GetOutput()
try:
- proc = subprocess.run([filter_program], capture_output=True, text=True, input=output)
+ proc = subprocess.run(
+ filter_program, capture_output=True, text=True, input=output
+ )
except (subprocess.SubprocessError, OSError) as e:
result.PutCString("Error occurred. Original disassembly:\n\n" + output)
result.SetError(str(e))
diff --git a/lldb/test/Shell/Commands/command-disassemble-riscv32-bytes.s b/lldb/test/Shell/Commands/command-disassemble-riscv32-bytes.s
index bd40baf2643a0..78be614e3af15 100644
--- a/lldb/test/Shell/Commands/command-disassemble-riscv32-bytes.s
+++ b/lldb/test/Shell/Commands/command-disassemble-riscv32-bytes.s
@@ -1,6 +1,5 @@
# REQUIRES: riscv
-# Unsupported until we fix launching the filter program on Windows.
-# UNSUPPORTED: system-windows
+# REQUIRES: python
# This test verifies that disassemble -b prints out the correct bytes and
# format for standard and unknown riscv instructions of various sizes,
@@ -11,7 +10,7 @@
# RUN: llvm-mc -filetype=obj -mattr=+c --triple=riscv32-unknown-unknown %s -o %t
# RUN: %lldb -b %t "-o" "disassemble -b -n main" | FileCheck %s
-# RUN: %lldb -b %t -o "command script import %S/../../../examples/python/filter_disasm.py" -o "fdis set %S/Inputs/dis_filt.py" -o "fdis -n main" | FileCheck --check-prefix=FILTER %s
+# RUN: %lldb -b %t -o "command script import %S/../../../examples/python/filter_disasm.py" -o "fdis set %python %S/Inputs/dis_filt.py" -o "fdis -n main" | FileCheck --check-prefix=FILTER %s
main:
addi sp, sp, -0x20 # 16 bit standard instruction
|
|
@llvm/pr-subscribers-lldb Author: David Spickett (DavidSpickett) ChangesThis changes the example command added in #145793 so that the fdis program does not have to be a single program name. Doing so also means we can run the test on Windows where the program needs to be "python.exe script_name". I've changed "fdis set" to treat the rest of the command as the program. Then store that as a list to be passed to subprocess. If we just use a string, Python will think that "python.exe foo" is the name of an actual program instead of a program and an argument to it. This will still break if the paths have spaces in, but I'm trying to do just enough to fix the test here without rewriting all the option handling. Full diff: https://github.com/llvm/llvm-project/pull/148823.diff 2 Files Affected:
diff --git a/lldb/examples/python/filter_disasm.py b/lldb/examples/python/filter_disasm.py
index de99d4031a7fd..797144460afbd 100644
--- a/lldb/examples/python/filter_disasm.py
+++ b/lldb/examples/python/filter_disasm.py
@@ -11,8 +11,13 @@
import lldb
import subprocess
-filter_program = "crustfilt"
+class Program(list):
+ def __str__(self):
+ return " ".join(self)
+
+
+filter_program = Program(["crustfilt"])
def __lldb_init_module(debugger, dict):
debugger.HandleCommand("command script add -f filter_disasm.fdis fdis")
@@ -51,13 +56,20 @@ def fdis(debugger, args, exe_ctx, result, dict):
result.Clear()
if len(args_list) == 1 and args_list[0] == "get":
- result.PutCString(filter_program)
+ result.PutCString(str(filter_program))
result.SetStatus(lldb.eReturnStatusSuccessFinishResult)
return
- if len(args_list) == 2 and args_list[0] == "set":
- filter_program = args_list[1]
- result.PutCString("Filter program set to %s" % filter_program)
+ if args_list[0] == "set":
+ # Assume the rest is a program to run, which might be a path containing spaces
+ # or an interpreter and a file path.
+ if len(args_list) <= 1:
+ result.PutCString('"set" command requires a program argument')
+ result.SetStatus(lldb.eReturnStatusFailed)
+ return
+
+ filter_program = Program(args_list[1:])
+ result.PutCString('Filter program set to "{}"'.format(filter_program))
result.SetStatus(lldb.eReturnStatusSuccessFinishResult)
return
@@ -70,7 +82,9 @@ def fdis(debugger, args, exe_ctx, result, dict):
output = res.GetOutput()
try:
- proc = subprocess.run([filter_program], capture_output=True, text=True, input=output)
+ proc = subprocess.run(
+ filter_program, capture_output=True, text=True, input=output
+ )
except (subprocess.SubprocessError, OSError) as e:
result.PutCString("Error occurred. Original disassembly:\n\n" + output)
result.SetError(str(e))
diff --git a/lldb/test/Shell/Commands/command-disassemble-riscv32-bytes.s b/lldb/test/Shell/Commands/command-disassemble-riscv32-bytes.s
index bd40baf2643a0..78be614e3af15 100644
--- a/lldb/test/Shell/Commands/command-disassemble-riscv32-bytes.s
+++ b/lldb/test/Shell/Commands/command-disassemble-riscv32-bytes.s
@@ -1,6 +1,5 @@
# REQUIRES: riscv
-# Unsupported until we fix launching the filter program on Windows.
-# UNSUPPORTED: system-windows
+# REQUIRES: python
# This test verifies that disassemble -b prints out the correct bytes and
# format for standard and unknown riscv instructions of various sizes,
@@ -11,7 +10,7 @@
# RUN: llvm-mc -filetype=obj -mattr=+c --triple=riscv32-unknown-unknown %s -o %t
# RUN: %lldb -b %t "-o" "disassemble -b -n main" | FileCheck %s
-# RUN: %lldb -b %t -o "command script import %S/../../../examples/python/filter_disasm.py" -o "fdis set %S/Inputs/dis_filt.py" -o "fdis -n main" | FileCheck --check-prefix=FILTER %s
+# RUN: %lldb -b %t -o "command script import %S/../../../examples/python/filter_disasm.py" -o "fdis set %python %S/Inputs/dis_filt.py" -o "fdis -n main" | FileCheck --check-prefix=FILTER %s
main:
addi sp, sp, -0x20 # 16 bit standard instruction
|
This changes the example command added in llvm#145793 so that the fdis program does not have to be a single program name. Doing so also means we can run the test on Windows where the program needs to be "python.exe script_name". I've changed "fdis set" to treat the rest of the command as the program. Then store that as a list to be passed to subprocess. If we just use a string, Python will think that "python.exe foo" is the name of an actual program. This will still break if the paths have spaces in, but I'm trying to do just enough to fix the test here without rewriting all the option handling.
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.
This changes the example command added in #145793 so that the fdis program does not have to be a single program name.
Doing so also means we can run the test on Windows where the program needs to be "python.exe script_name".
I've changed "fdis set" to treat the rest of the command as the program. Then store that as a list to be passed to subprocess. If we just use a string, Python will think that "python.exe foo" is the name of an actual program instead of a program and an argument to it.
This will still break if the paths have spaces in, but I'm trying to do just enough to fix the test here without rewriting all the option handling.