[LLDB] Apply FixCodeAddress to all forms of address arguments

[DavidSpickett]

Summary

This is a follow up to https://reviews.llvm.org/D141629
and applies the change it made to all paths through ToAddress
(now DoToAddress).

I have included the test from my previous attempt
https://reviews.llvm.org/D136938.

The initial change only applied fixing to addresses that
would parse as integers, so my test case failed. Since
ToAddress has multiple exit points, I've wrapped it into
a new method DoToAddress.

Now you can call ToAddress, it will call DoToAddress and
no matter what path you take, the address will be fixed.

For the memory tagging commands we actually want the full
address (to work out mismatches). So I added ToRawAddress
for that.

I have tested this on a QEMU AArch64 Linux system with
Memory Tagging, Pointer Authentication and Top Byte Ignore
enabled. By running the new test and all other tests in
API/linux/aarch64.

Some commands have had calls to the ABI plugin removed
as ToAddress now does this for them.

The "memory region" command still needs to use the ABI plugin
to detect the end of memory when there are non-address bits.

[DavidSpickett]

Apologies it took me so long to get around to this.

As a superset of your change I presume the test you added will still pass. I don't have a Mac to confirm that myself.

[DavidSpickett]

ping!

[JDevlieghere]

I left a few nits here and there but overall this LGTM

lldb/include/lldb/Interpreter/OptionArgParser.h

19–20

25

lldb/source/Interpreter/OptionArgParser.cpp

161–162

[jasonmolenda]

LGTM.

[DavidSpickett]

Address Jonas' comments.

lldb/include/lldb/Interpreter/OptionArgParser.h

 //===-- OptionArgParser.h ---------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_INTERPRETER_OPTIONARGPARSER_H
 #define LLDB_INTERPRETER_OPTIONARGPARSER_H
 
 #include "lldb/lldb-private-types.h"
 
+#include <optional>
+
 namespace lldb_private {
 
 struct OptionArgParser {
+  /// Try to parse an address. If it succeeds return the address with the
+  /// non-address bits removed.
   static lldb::addr_t ToAddress(const ExecutionContext *exe_ctx,
                                 llvm::StringRef s, lldb::addr_t fail_value,
-                                Status *error);
+                                Status *error_ptr);
+
+  /// As for ToAddress but do not remove non-address bits from the result.
+  static lldb::addr_t ToRawAddress(const ExecutionContext *exe_ctx,
+                                   llvm::StringRef s, lldb::addr_t fail_value,
+                                   Status *error_ptr);
 
   static bool ToBoolean(llvm::StringRef s, bool fail_value, bool *success_ptr);
 
   static char ToChar(llvm::StringRef s, char fail_value, bool *success_ptr);
 
   static int64_t ToOptionEnum(llvm::StringRef s,
                               const OptionEnumValues &enum_values,
                               int32_t fail_value, Status &error);
 
   static lldb::ScriptLanguage ToScriptLanguage(llvm::StringRef s,
                                                lldb::ScriptLanguage fail_value,
                                                bool *success_ptr);
 
   // TODO: Use StringRef
   static Status ToFormat(const char *s, lldb::Format &format,
                          size_t *byte_size_ptr); // If non-NULL, then a
                                                  // byte size can precede
                                                  // the format character
+
+private:
+  static std::optional<lldb::addr_t>
+  DoToAddress(const ExecutionContext *exe_ctx, llvm::StringRef s,
+              Status *error);
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_INTERPRETER_OPTIONARGPARSER_H

lldb/source/Commands/CommandObjectMemory.cpp

if (argc > 0)
                                         LLDB_INVALID_ADDRESS, &error);
 
     if (addr == LLDB_INVALID_ADDRESS) {
       result.AppendError("invalid start address expression.");
       result.AppendError(error.AsCString());
       return false;
     }
 
-    ABISP abi;
-    if (Process *proc = m_exe_ctx.GetProcessPtr())
-      abi = proc->GetABI();
-
-    if (abi)
-      addr = abi->FixDataAddress(addr);
-
     if (argc == 2) {
       lldb::addr_t end_addr = OptionArgParser::ToAddress(
           &m_exe_ctx, command[1].ref(), LLDB_INVALID_ADDRESS, nullptr);
-      if (end_addr != LLDB_INVALID_ADDRESS && abi)
-        end_addr = abi->FixDataAddress(end_addr);
 
       if (end_addr == LLDB_INVALID_ADDRESS) {
         result.AppendError("invalid end address expression.");
         result.AppendError(error.AsCString());
         return false;
       } else if (end_addr <= addr) {
         result.AppendErrorWithFormat(
             "end address (0x%" PRIx64
bool DoExecute(Args &command, CommandReturnObject &result) override {
     }
     lldb::addr_t high_addr = OptionArgParser::ToAddress(
         &m_exe_ctx, command[1].ref(), LLDB_INVALID_ADDRESS, &error);
     if (high_addr == LLDB_INVALID_ADDRESS || error.Fail()) {
       result.AppendError("invalid high address");
       return false;
     }
 
-    ABISP abi = m_exe_ctx.GetProcessPtr()->GetABI();
-    if (abi) {
-      low_addr = abi->FixDataAddress(low_addr);
-      high_addr = abi->FixDataAddress(high_addr);
-    }
-
     if (high_addr <= low_addr) {
       result.AppendError(
           "starting address must be smaller than ending address");
       return false;
     }
 
     lldb::addr_t found_location = LLDB_INVALID_ADDRESS;
 
if (argc == 1) {
       if (m_memory_region_options.m_all) {
         result.AppendError(
             "The \"--all\" option cannot be used when an address "
             "argument is given");
         return false;
       }
 
       auto load_addr_str = command[0].ref();
-      // Non-address bits in this will be handled later by GetMemoryRegion
       load_addr = OptionArgParser::ToAddress(&m_exe_ctx, load_addr_str,
                                              LLDB_INVALID_ADDRESS, &error);
       if (error.Fail() || load_addr == LLDB_INVALID_ADDRESS) {
         result.AppendErrorWithFormat("invalid address argument \"%s\": %s\n",
                                      command[0].c_str(), error.AsCString());
         return false;
       }
     } else if (argc > 1 ||

lldb/source/Commands/CommandObjectMemoryTag.cpp

bool DoExecute(Args &command, CommandReturnObject &result) override {
     if ((command.GetArgumentCount() < 1) || (command.GetArgumentCount() > 2)) {
       result.AppendError(
           "wrong number of arguments; expected at least <address-expression>, "
           "at most <address-expression> <end-address-expression>");
       return false;
     }
 
     Status error;
-    addr_t start_addr = OptionArgParser::ToAddress(
+    addr_t start_addr = OptionArgParser::ToRawAddress(
         &m_exe_ctx, command[0].ref(), LLDB_INVALID_ADDRESS, &error);
     if (start_addr == LLDB_INVALID_ADDRESS) {
       result.AppendErrorWithFormatv("Invalid address expression, {0}",
                                     error.AsCString());
       return false;
     }
 
     // Default 1 byte beyond start, rounds up to at most 1 granule later
     addr_t end_addr = start_addr + 1;
 
     if (command.GetArgumentCount() > 1) {
-      end_addr = OptionArgParser::ToAddress(&m_exe_ctx, command[1].ref(),
+      end_addr = OptionArgParser::ToRawAddress(&m_exe_ctx, command[1].ref(),
                                                LLDB_INVALID_ADDRESS, &error);
       if (end_addr == LLDB_INVALID_ADDRESS) {
         result.AppendErrorWithFormatv("Invalid end address expression, {0}",
                                       error.AsCString());
         return false;
       }
     }
 
     Process *process = m_exe_ctx.GetProcessPtr();
public:
     Status SetOptionValue(uint32_t option_idx, llvm::StringRef option_value,
                           ExecutionContext *execution_context) override {
       Status status;
       const int short_option =
           g_memory_tag_write_options[option_idx].short_option;
 
       switch (short_option) {
       case 'e':
-        m_end_addr = OptionArgParser::ToAddress(execution_context, option_value,
-                                                LLDB_INVALID_ADDRESS, &status);
+        m_end_addr = OptionArgParser::ToRawAddress(
+            execution_context, option_value, LLDB_INVALID_ADDRESS, &status);
         break;
       default:
         llvm_unreachable("Unimplemented option");
       }
 
       return status;
     }
 
protected:
   bool DoExecute(Args &command, CommandReturnObject &result) override {
     if (command.GetArgumentCount() < 2) {
       result.AppendError("wrong number of arguments; expected "
                          "<address-expression> <tag> [<tag> [...]]");
       return false;
     }
 
     Status error;
-    addr_t start_addr = OptionArgParser::ToAddress(
+    addr_t start_addr = OptionArgParser::ToRawAddress(
         &m_exe_ctx, command[0].ref(), LLDB_INVALID_ADDRESS, &error);
     if (start_addr == LLDB_INVALID_ADDRESS) {
       result.AppendErrorWithFormatv("Invalid address expression, {0}",
                                     error.AsCString());
       return false;
     }
 
     command.Shift(); // shift off start address

lldb/source/Interpreter/OptionArgParser.cpp

lldb::ScriptLanguage OptionArgParser::ToScriptLanguage(
   if (s.equals_insensitive("none"))
     return eScriptLanguageNone;
 
   if (success_ptr)
     *success_ptr = false;
   return fail_value;
 }
 
+lldb::addr_t OptionArgParser::ToRawAddress(const ExecutionContext *exe_ctx,
+                                           llvm::StringRef s,
+                                           lldb::addr_t fail_value,
+                                           Status *error_ptr) {
+  std::optional<lldb::addr_t> maybe_addr = DoToAddress(exe_ctx, s, error_ptr);
+  return maybe_addr ? *maybe_addr : fail_value;
+}
+
 lldb::addr_t OptionArgParser::ToAddress(const ExecutionContext *exe_ctx,
                                         llvm::StringRef s,
                                         lldb::addr_t fail_value,
                                         Status *error_ptr) {
+  std::optional<lldb::addr_t> maybe_addr = DoToAddress(exe_ctx, s, error_ptr);
+  if (!maybe_addr)
+    return fail_value;
+
+  lldb::addr_t addr = *maybe_addr;
+
+  if (Process *process = exe_ctx->GetProcessPtr())
+    if (ABISP abi_sp = process->GetABI())
+      addr = abi_sp->FixCodeAddress(addr);
+
+  return addr;
+}
+
+std::optional<lldb::addr_t>
+OptionArgParser::DoToAddress(const ExecutionContext *exe_ctx, llvm::StringRef s,
+                             Status *error_ptr) {
   bool error_set = false;
   if (s.empty()) {
     if (error_ptr)
       error_ptr->SetErrorStringWithFormat("invalid address expression \"%s\"",
                                           s.str().c_str());
-    return fail_value;
+    return {};
   }
 
   llvm::StringRef sref = s;
 
   lldb::addr_t addr = LLDB_INVALID_ADDRESS;
   if (!s.getAsInteger(0, addr)) {
     if (error_ptr)
       error_ptr->Clear();
-    Process *process = exe_ctx->GetProcessPtr();
-    if (process)
-      if (ABISP abi_sp = process->GetABI())
-        addr = abi_sp->FixCodeAddress(addr);
+
     return addr;
   }
 
   // Try base 16 with no prefix...
   if (!s.getAsInteger(16, addr)) {
     if (error_ptr)
       error_ptr->Clear();
     return addr;
   }
 
   Target *target = nullptr;
   if (!exe_ctx || !(target = exe_ctx->GetTargetPtr())) {
     if (error_ptr)
       error_ptr->SetErrorStringWithFormat("invalid address expression \"%s\"",
                                           s.str().c_str());
-    return fail_value;
+    return {};
   }
 
   lldb::ValueObjectSP valobj_sp;
   EvaluateExpressionOptions options;
   options.SetCoerceToId(false);
   options.SetUnwindOnError(true);
   options.SetKeepInMemory(false);
   options.SetTryAllThreads(true);
 
   ExpressionResults expr_result =
       target->EvaluateExpression(s, exe_ctx->GetFramePtr(), valobj_sp, options);
 
   bool success = false;
   if (expr_result == eExpressionCompleted) {
     if (valobj_sp)
       valobj_sp = valobj_sp->GetQualifiedRepresentationIfAvailable(
           valobj_sp->GetDynamicValueType(), true);
     // Get the address to watch.
     if (valobj_sp)
-      addr = valobj_sp->GetValueAsUnsigned(fail_value, &success);
+      addr = valobj_sp->GetValueAsUnsigned(0, &success);
     if (success) {
       if (error_ptr)
         error_ptr->Clear();
       return addr;
     } else {
       if (error_ptr) {
         error_set = true;
         error_ptr->SetErrorStringWithFormat(
if (expr_result == eExpressionCompleted) {
     }
   }
 
   if (error_ptr) {
     if (!error_set)
       error_ptr->SetErrorStringWithFormat("invalid address expression \"%s\"",
                                           s.str().c_str());
   }
-  return fail_value;
+  return {};
 }

lldb/test/API/linux/aarch64/non_address_bit_code_break/Makefile

+C_SOURCES := main.c
+
+CFLAGS_EXTRAS := -march=armv8.3-a
+
+include Makefile.rules

lldb/test/API/linux/aarch64/non_address_bit_code_break/TestAArch64LinuxNonAddressBitCodeBreak.py

+"""
+Test that "breakpoint set -a" uses the ABI plugin to remove non-address bits
+before attempting to set a breakpoint.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test import lldbutil
+
+
+class AArch64LinuxNonAddressBitCodeBreak(TestBase):
+
+    NO_DEBUG_INFO_TESTCASE = True
+
+    def do_tagged_break(self, hardware):
+        if not self.isAArch64PAuth():
+            self.skipTest('Target must support pointer authentication.')
+
+        self.build()
+        self.runCmd("file " + self.getBuildArtifact("a.out"), CURRENT_EXECUTABLE_SET)
+
+        lldbutil.run_break_set_by_file_and_line(self, "main.c",
+            line_number('main.c', '// Set break point at this line.'),
+            num_expected_locations=1)
+
+        self.runCmd("run", RUN_SUCCEEDED)
+
+        if self.process().GetState() == lldb.eStateExited:
+            self.fail("Test program failed to run.")
+
+        self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+            substrs=['stopped',
+                     'stop reason = breakpoint'])
+
+        cmd = "breakpoint set -a fnptr"
+        # LLDB only has the option to force hardware break, not software.
+        # It prefers sofware break when it can and this will be one of those cases.
+        if hardware:
+            cmd += " --hardware"
+        self.expect(cmd)
+
+        self.runCmd("continue")
+        self.assertEqual(self.process().GetState(), lldb.eStateStopped)
+        self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
+            substrs=['stopped', '`foo at main.c', 'stop reason = breakpoint'])
+
+    # AArch64 Linux always enables the top byte ignore feature
+    @skipUnlessArch("aarch64")
+    @skipUnlessPlatform(["linux"])
+    def test_software_break(self):
+        self.do_tagged_break(False)
+
+    @skipUnlessArch("aarch64")
+    @skipUnlessPlatform(["linux"])
+    def test_hardware_break(self):
+        self.do_tagged_break(True)

lldb/test/API/linux/aarch64/non_address_bit_code_break/main.c

+#include <stdint.h>
+
+void foo(void) {}
+typedef void (*FooPtr)(void);
+
+int main() {
+  FooPtr fnptr = foo;
+  // Set top byte.
+  fnptr = (FooPtr)((uintptr_t)fnptr | (uintptr_t)0xff << 56);
+  // Then apply a PAuth signature to it.
+  __asm__ __volatile__("pacdza %0" : "=r"(fnptr) : "r"(fnptr));
+  // fnptr is now:
+  // <8 bit top byte tag><pointer signature><virtual address>
+
+  foo(); // Set break point at this line.
+
+  return 0;
+}