[lldb][lldb-dap] Use the default disassembly flavour if none is provided.

[da-viper]

This is the currently the default for SBTarget::ReadInstructions(SBAddress, uint32_t). But not for others, to make it consistent used the user assigned instruction flavour.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Ebuka Ezike (da-viper)

Changes

This is the currently the default for SBTarget::ReadInstructions(SBAddress, uint32_t). But not for others, to make it consistent used the user assigned instruction flavour.

---

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

3 Files Affected:

• (modified) lldb/source/API/SBTarget.cpp (+20-1)
• (modified) lldb/source/Commands/CommandObjectDisassemble.cpp (-1)
• (modified) lldb/tools/lldb-dap/Handler/DisassembleRequestHandler.cpp (+1-16)

diff --git a/lldb/source/API/SBTarget.cpp b/lldb/source/API/SBTarget.cpp
index cd8a770a0ec04..8d97c454c300c 100644
--- a/lldb/source/API/SBTarget.cpp
+++ b/lldb/source/API/SBTarget.cpp
@@ -2039,7 +2039,17 @@ lldb::SBInstructionList SBTarget::ReadInstructions(lldb::SBAddress base_addr,
       const size_t bytes_read =
           target_sp->ReadMemory(*addr_ptr, data.GetBytes(), data.GetByteSize(),
                                 error, force_live_memory, &load_addr);
+
       const bool data_from_file = load_addr == LLDB_INVALID_ADDRESS;
+      if (!flavor_string || flavor_string[0] == '\0') {
+        // FIXME - we don't have the mechanism in place to do per-architecture
+        // settings.  But since we know that for now we only support flavors on
+        // x86 & x86_64,
+        const llvm::Triple::ArchType arch =
+            target_sp->GetArchitecture().GetTriple().getArch();
+        if (arch == llvm::Triple::x86 || arch == llvm::Triple::x86_64)
+          flavor_string = target_sp->GetDisassemblyFlavor();
+      }
       sb_instructions.SetDisassembler(Disassembler::DisassembleBytes(
           target_sp->GetArchitecture(), nullptr, flavor_string,
           target_sp->GetDisassemblyCPU(), target_sp->GetDisassemblyFeatures(),
@@ -2098,7 +2108,16 @@ SBTarget::GetInstructionsWithFlavor(lldb::SBAddress base_addr,
     if (base_addr.get())
       addr = *base_addr.get();
 
-    const bool data_from_file = true;
+    constexpr bool data_from_file = true;
+    if (!flavor_string || flavor_string[0] == '\0') {
+      // FIXME - we don't have the mechanism in place to do per-architecture
+      // settings.  But since we know that for now we only support flavors on
+      // x86 & x86_64,
+      const llvm::Triple::ArchType arch =
+          target_sp->GetArchitecture().GetTriple().getArch();
+      if (arch == llvm::Triple::x86 || arch == llvm::Triple::x86_64)
+        flavor_string = target_sp->GetDisassemblyFlavor();
+    }
 
     sb_instructions.SetDisassembler(Disassembler::DisassembleBytes(
         target_sp->GetArchitecture(), nullptr, flavor_string,
diff --git a/lldb/source/Commands/CommandObjectDisassemble.cpp b/lldb/source/Commands/CommandObjectDisassemble.cpp
index 5774effb9e9ba..70e687e19ac6d 100644
--- a/lldb/source/Commands/CommandObjectDisassemble.cpp
+++ b/lldb/source/Commands/CommandObjectDisassemble.cpp
@@ -190,7 +190,6 @@ void CommandObjectDisassemble::CommandOptions::OptionParsingStarting(
     // architecture.  For now GetDisassemblyFlavor is really only valid for x86
     // (and for the llvm assembler plugin, but I'm papering over that since that
     // is the only disassembler plugin we have...
-    // This logic is duplicated in `Handler/DisassembleRequestHandler`.
     if (target->GetArchitecture().GetTriple().getArch() == llvm::Triple::x86 ||
         target->GetArchitecture().GetTriple().getArch() ==
             llvm::Triple::x86_64) {
diff --git a/lldb/tools/lldb-dap/Handler/DisassembleRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/DisassembleRequestHandler.cpp
index c9061ef19f17a..a7d70d4c3aceb 100644
--- a/lldb/tools/lldb-dap/Handler/DisassembleRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/DisassembleRequestHandler.cpp
@@ -192,21 +192,6 @@ DisassembleRequestHandler::Run(const DisassembleArguments &args) const {
     return llvm::make_error<DAPError>(
         "Memory reference not found in the current binary.");
 
-  std::string flavor_string;
-  const auto target_triple = llvm::StringRef(dap.target.GetTriple());
-  // This handles both 32 and 64bit x86 architecture. The logic is duplicated in
-  // `CommandObjectDisassemble::CommandOptions::OptionParsingStarting`
-  if (target_triple.starts_with("x86")) {
-    const lldb::SBStructuredData flavor =
-        dap.debugger.GetSetting("target.x86-disassembly-flavor");
-
-    const size_t str_length = flavor.GetStringValue(nullptr, 0);
-    if (str_length != 0) {
-      flavor_string.resize(str_length + 1);
-      flavor.GetStringValue(flavor_string.data(), flavor_string.length());
-    }
-  }
-
   // Offset (in instructions) to be applied after the byte offset (if any)
   // before disassembling. Can be negative.
   int64_t instruction_offset = args.instructionOffset.value_or(0);
@@ -219,7 +204,7 @@ DisassembleRequestHandler::Run(const DisassembleArguments &args) const {
         "Unexpected error while disassembling instructions.");
 
   lldb::SBInstructionList insts = dap.target.ReadInstructions(
-      disassemble_start_addr, args.instructionCount, flavor_string.c_str());
+      disassemble_start_addr, args.instructionCount);
   if (!insts.IsValid())
     return llvm::make_error<DAPError>(
         "Unexpected error while disassembling instructions.");

[JDevlieghere]

This PR wasn't approved when it was merged, similar to #142030 yesterday. Except for trivial changes, please wait for an approval before merging. If you want to reminder reviewers to take a look you can ping the PR.

[da-viper]

@JDevlieghere sorry about that, I did assume since it was small. will wait for the approval hence forth.

[JDevlieghere]

Thanks! For trivial changes you can merge the PR directly (conveying that you're not waiting on reviews and that the PR is meant as an FYI) or you can leave a comment saying "this is trivial but I'll let it sit here for a day in case someone wants to look at it".

[da-viper]

One more thing to confirm, If it is the case it is fixing a failing test on build bot, I also wait ?

[JDevlieghere]

One more thing to confirm, If it is the case it is fixing a failing test on build bot, I also wait ?

You can go ahead and merge that right away. I'd consider that a trivial change or similar to a revert, which also doesn't require approvals.