Merge pull request #967 from flit/bugfix/gdb_step_interrupt

Step fixes and improvements

docs/options.md

@@ -76,6 +76,16 @@ working directory.
 Controls how pyOCD connects to the target. One of 'halt', 'pre-reset', 'under-reset', 'attach'.
 </td></tr>
 
+<tr><td>cpu.step.instruction.timeout</td>
+<td>float</td>
+<td>0.0</td>
+<td>
+<p>Timeout in seconds for instruction step operations. The default of 0 means no timeout.<p>
+<p>Note that stepping may take a very long time for to return in cases such as stepping over a branch
+into the Secure world where the debugger doesn't have secure debug access, or similar for Privileged
+code in the case of UDE.</p>
+</td></tr>
+
 <tr><td>dap_protocol</td>
 <td>str</td>
 <td>'default'</td>

pyocd/core/options.py

@@ -39,6 +39,8 @@
         "Path to custom config file."),
     OptionInfo('connect_mode', str, "halt",
         "One of 'halt', 'pre-reset', 'under-reset', 'attach'. Default is 'halt'."),
+    OptionInfo('cpu.step.instruction.timeout', float, 0.0,
+        "Timeout in seconds for instruction step operations. Defaults to 0, or no timeout."),
     OptionInfo('dap_protocol', str, 'default',
         "Wire protocol, either 'swd', 'jtag', or 'default'."),
     OptionInfo('dap_swj_enable', bool, True,

pyocd/core/soc_target.py

@@ -142,8 +142,8 @@ def run_token(self):
     def halt(self):
         return self.selected_core.halt()
 
-    def step(self, disable_interrupts=True, start=0, end=0):
-        return self.selected_core.step(disable_interrupts, start, end)
+    def step(self, disable_interrupts=True, start=0, end=0, hook_cb=None):
+        return self.selected_core.step(disable_interrupts, start, end, hook_cb)
 
     def resume(self):
         return self.selected_core.resume()

pyocd/core/target.py

@@ -235,7 +235,7 @@ def flush(self):
     def halt(self):
         raise NotImplementedError()
 
-    def step(self, disable_interrupts=True, start=0, end=0):
+    def step(self, disable_interrupts=True, start=0, end=0, hook_cb=None):
         raise NotImplementedError()
 
     def resume(self):

pyocd/coresight/cortex_m.py

@@ -113,6 +113,7 @@ class CortexM(Target, CoreSightCoreComponent):
     C_STEP = (1 << 2)
     C_MASKINTS = (1 << 3)
     C_SNAPSTALL = (1 << 5)
+    C_PMOV = (1 << 6)
     S_REGRDY = (1 << 16)
     S_HALT = (1 << 17)
     S_SLEEP = (1 << 18)
@@ -473,68 +474,118 @@ def halt(self):
         self.flush()
         self.session.notify(Target.Event.POST_HALT, self, Target.HaltReason.USER)
 
-    def step(self, disable_interrupts=True, start=0, end=0):
+    def step(self, disable_interrupts=True, start=0, end=0, hook_cb=None):
         """! @brief Perform an instruction level step.
         
-        This function preserves the previous interrupt mask state.
+        This API will execute one or more individual instructions on the core. With default parameters, it
+        masks interrupts and only steps a single instruction. The _start_ and _stop_ parameters define an
+        address range of [_start_, _end_). The core will be repeatedly stepped until the PC falls outside this
+        range, a debug event occurs, or the optional callback returns True.
+        
+        The _disable_interrupts_ parameter controls whether to allow stepping into interrupts. This function
+        preserves the previous interrupt mask state.
+        
+        If the _hook_cb_ parameter is set to a callable, it will be invoked repeatedly to give the caller a
+        chance to check for interrupt requests or other reasons to exit.
+
+        Note that stepping may take a very long time for to return in cases such as stepping over a branch
+        into the Secure world where the debugger doesn't have secure debug access, or similar for Privileged
+        code in the case of UDE.
+        
+        @param self The object.
+        @param disable_interrupts Boolean specifying whether to mask interrupts during the step.
+        @param start Integer start address for range stepping. Not included in the range.
+        @param end Integer end address for range stepping. The range is inclusive of this address.
+        @param hook_cb Optional callable taking no parameters and returning a boolean. The signature is
+            `hook_cb() -> bool`. Invoked repeatedly while waiting for step operations to complete. If the
+            callback returns True, then stepping is stopped immediately.
+        
+        @exception DebugError Raised if debug is not enabled on the core.
         """
-        # Was 'if self.get_state() != TARGET_HALTED:'
-        # but now value of dhcsr is saved
-        dhcsr = self.read_memory(CortexM.DHCSR)
-        if not (dhcsr & (CortexM.C_STEP | CortexM.C_HALT)):
-            LOG.error('cannot step: target not halted')
+        # Save DHCSR and make sure the core is halted. We also check that C_DEBUGEN is set because if it's
+        # not, then C_HALT is UNKNOWN.
+        dhcsr = self.read32(CortexM.DHCSR)
+        if not (dhcsr & CortexM.C_DEBUGEN):
+            raise exception.DebugError('cannot step: debug not enabled')
+        if not (dhcsr & CortexM.C_HALT):
+            LOG.error('cannot step: core not halted')
             return
 
-        LOG.debug("step core %d", self.core_number)
+        if start != end:
+            LOG.debug("step core %d (start=%#010x, end=%#010x)", self.core_number, start, end)
+        else:
+            LOG.debug("step core %d", self.core_number)
 
         self.session.notify(Target.Event.PRE_RUN, self, Target.RunType.STEP)
 
+        self._run_token += 1
+
         self.clear_debug_cause_bits()
 
-        # Save previous interrupt mask state
-        interrupts_masked = (CortexM.C_MASKINTS & dhcsr) != 0
+        # Get current state.
+        saved_maskints = dhcsr & CortexM.C_MASKINTS
+        saved_pmov = dhcsr & CortexM.C_PMOV
+        maskints_differs = bool(saved_maskints) != disable_interrupts
 
-        # Mask interrupts - C_HALT must be set when changing to C_MASKINTS
-        if not interrupts_masked and disable_interrupts:
-            self.write_memory(CortexM.DHCSR, CortexM.DBGKEY | CortexM.C_DEBUGEN | CortexM.C_HALT | CortexM.C_MASKINTS)
+        # Get the DHCSR value to use when stepping based on whether we're masking interrupts.
+        dhcsr_step = CortexM.DBGKEY | CortexM.C_DEBUGEN | CortexM.C_STEP | saved_pmov
+        if disable_interrupts:
+            dhcsr_step |= CortexM.C_MASKINTS
 
-        # Single step using current C_MASKINTS setting
-        while True:
-            if disable_interrupts or interrupts_masked:
-                self.write_memory(CortexM.DHCSR, CortexM.DBGKEY | CortexM.C_DEBUGEN | CortexM.C_MASKINTS | CortexM.C_STEP)
-            else:
-                self.write_memory(CortexM.DHCSR, CortexM.DBGKEY | CortexM.C_DEBUGEN | CortexM.C_STEP)
+        # Update mask interrupts setting - C_HALT must be set when changing to C_MASKINTS.
+        if maskints_differs:
+            self.write32(CortexM.DHCSR, dhcsr_step | CortexM.C_HALT)
 
-            # Wait for halt to auto set (This should be done before the first read)
-            while not self.read_memory(CortexM.DHCSR) & CortexM.C_HALT:
-                pass
+        # Get the step timeout. A timeout of 0 means no timeout, so we have to pass None to the Timeout class.
+        step_timeout = self.session.options.get('cpu.step.instruction.timeout') or None
+
+        while True:
+            # Single step using current C_MASKINTS setting
+            self.write32(CortexM.DHCSR, dhcsr_step)
+
+            # Wait for halt to auto set.
+            #
+            # Note that it may take a very long time for this loop to exit in cases such as stepping over
+            # a branch into the Secure world where the debugger doesn't have secure debug access, or similar
+            # for Privileged code in the case of UDE.
+            with timeout.Timeout(step_timeout) as tmo:
+                while tmo.check():
+                    if (self.read32(CortexM.DHCSR) & CortexM.C_HALT) != 0:
+                        break
+                    # Invoke the callback if provided. If it returns True, then exit the loop.
+                    if (hook_cb is not None) and hook_cb():
+                        break
 
             # Range is empty, 'range step' will degenerate to 'step'
             if start == end:
                 break
 
             # Read program counter and compare to [start, end)
             program_counter = self.read_core_register('pc')
-            if program_counter < start or end <= program_counter:
+            if (program_counter < start) or (end <= program_counter):
                 break
 
-            # Check other stop reasons
-            if self.read_memory(CortexM.DFSR) & (CortexM.DFSR_DWTTRAP | CortexM.DFSR_BKPT):
+            # Check for stop reasons other than HALTED, which will have been set by our step action.
+            if (self.read32(CortexM.DFSR) & ~CortexM.DFSR_HALTED) != 0:
                 break
 
-        # Restore interrupt mask state
-        if not interrupts_masked and disable_interrupts:
-            # Unmask interrupts - C_HALT must be set when changing to C_MASKINTS
-            self.write_memory(CortexM.DHCSR, CortexM.DBGKEY | CortexM.C_DEBUGEN | CortexM.C_HALT)
+        # Restore interrupt mask state.
+        if maskints_differs:
+            self.write32(CortexM.DHCSR,
+                    CortexM.DBGKEY | CortexM.C_DEBUGEN | CortexM.C_HALT | saved_maskints | saved_pmov)
 
         self.flush()
 
-        self._run_token += 1
-
         self.session.notify(Target.Event.POST_RUN, self, Target.RunType.STEP)
 
     def clear_debug_cause_bits(self):
-        self.write_memory(CortexM.DFSR, CortexM.DFSR_VCATCH | CortexM.DFSR_DWTTRAP | CortexM.DFSR_BKPT | CortexM.DFSR_HALTED)
+        self.write32(CortexM.DFSR,
+                CortexM.DFSR_EXTERNAL
+                | CortexM.DFSR_VCATCH
+                | CortexM.DFSR_DWTTRAP
+                | CortexM.DFSR_BKPT
+                | CortexM.DFSR_HALTED
+                )
 
     def _perform_emulated_reset(self):
         """! @brief Emulate a software reset by writing registers.

pyocd/coresight/cortex_m_v8m.py

@@ -152,6 +152,15 @@ def get_security_state(self):
             return Target.SecurityState.SECURE
         else:
             return Target.SecurityState.NONSECURE
+
+    def clear_debug_cause_bits(self):
+        self.write32(CortexM.DFSR,
+                self.DFSR_PMU
+                | CortexM.DFSR_EXTERNAL
+                | CortexM.DFSR_VCATCH
+                | CortexM.DFSR_DWTTRAP
+                | CortexM.DFSR_BKPT
+                | CortexM.DFSR_HALTED)
 
     def get_halt_reason(self):
         """! @brief Returns the reason the core has halted.

pyocd/coresight/generic_mem_ap.py

@@ -86,7 +86,7 @@ def read_memory_block32(self, addr, size):
     def halt(self):
         pass
 
-    def step(self, disable_interrupts=True, start=0, end=0):
+    def step(self, disable_interrupts=True, start=0, end=0, hook_cb=None):
         pass
 
     def reset(self, reset_type=None):

pyocd/gdbserver/gdbserver.py

@@ -625,8 +625,22 @@ def resume(self, data):
     def step(self, data, start=0, end=0):
         addr = self._get_resume_step_addr(data)
         LOG.debug("GDB step: %s (start=0x%x, end=0x%x)", data, start, end)
-        self.target.step(not self.step_into_interrupt, start, end)
-        return self.create_rsp_packet(self.get_t_response())
+
+        # Use the step hook to check for an interrupt event.
+        def step_hook():
+            # Note we don't clear the interrupt event here!
+            return self.packet_io.interrupt_event.is_set()
+        self.target.step(not self.step_into_interrupt, start, end, hook_cb=step_hook)
+
+        # Clear and handle an interrupt.
+        if self.packet_io.interrupt_event.is_set():
+            LOG.debug("Received Ctrl-C during step")
+            self.packet_io.interrupt_event.clear()
+            response = self.get_t_response(forceSignal=signals.SIGINT)
+        else:
+            response = self.get_t_response()
+
+        return self.create_rsp_packet(response)
 
     def halt(self):
         self.target.halt()

pyocd/utility/timeout.py

@@ -52,12 +52,15 @@ class Timeout(object):
     If you pass a non-zero value for _sleeptime_ to the constructor, the check() method will
     automatically sleep by default starting with the second call. You can disable auto-sleep
     by passing `autosleep=False` to check().
+    
+    Passing a timeout of None to the constructor is allowed. In this case, check() will always return
+    True and the loop must be exited via some other means.
     """
 
     def __init__(self, timeout, sleeptime=0):
         """! @brief Constructor.
         @param self
-        @param timeout The timeout in seconds.
+        @param timeout The timeout in seconds. May be None to indicate no timeout.
         @param sleeptime Time in seconds to sleep during calls to check(). Defaults to 0, thus
             check() will not sleep unless you pass a different value.
         """
@@ -83,12 +86,16 @@ def check(self, autosleep=True):
             - A non-zero _sleeptime_ was passed to the constructor.
             - The _autosleep_ parameter is True.
         
+        This method is intended to be used as the predicate of a while loop.
+        
         @param self
         @param autosleep Whether to sleep if not timed out yet. The sleeptime passed to the
             constructor must have been non-zero.
+        @retval True The timeout has _not_ occurred.
+        @retval False Timeout is passed and the loop should be exited.
         """
         # Check for a timeout.
-        if (time() - self._start) > self._timeout:
+        if (self._timeout is not None) and ((time() - self._start) > self._timeout):
             self._timed_out = True
         # Sleep if appropriate.
         elif (not self._is_first_check) and autosleep and self._sleeptime:

src/range_step/.gitignore

@@ -0,0 +1,3 @@
+*.lst
+*.bin
+*.elf

src/range_step/Makefile

@@ -0,0 +1,50 @@
+# pyOCD debugger
+# Copyright (c) 2020 Arm Limited
+# SPDX-License-Identifier: Apache-2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+PREFIX = arm-none-eabi-
+CC = $(PREFIX)gcc
+OBJCOPY = $(PREFIX)objcopy
+
+TARGET = range_step.elf
+TARGET_BIN = range_step.bin
+
+OBJECTS = range_step.o
+
+LIBRARIES =
+
+INCLUDES =
+
+ASFLAGS = -std=gnu11 -MMD -MP $(INCLUDES) -O0 -fno-common -ffunction-sections \
+		-fdata-sections -Wall -Werror -mcpu=cortex-m0 -mthumb -mfloat-abi=soft -g3 -gdwarf-2 \
+		-gstrict-dwarf -nostdlib -fpie -Wa,-adln=$(basename $@).lst
+
+LDFLAGS = -T"linker_script.ld" -Wl,-Map,$(basename $@).map,--gc-sections,-erange_step_test -nostdlib -fpie
+
+.PHONY: all
+all: $(TARGET) $(TARGET_BIN)
+
+.PHONY: clean
+clean:
+	rm -f *.o *.d *.map *.lst *.elf *.bin
+
+$(TARGET): $(OBJECTS)
+	$(CC) $(LDFLAGS) $(OBJECTS) $(LIBRARIES) -o $@
+
+$(TARGET_BIN): $(TARGET)
+	$(OBJCOPY) -O binary $(TARGET) $(TARGET_BIN)
+
+# Include dependency files.
+-include $(OBJECTS:.o=.d)

src/range_step/linker_script.ld

@@ -0,0 +1,50 @@
+/*
+ Copyright (c) 2020 ARM Limited
+
+ Licensed under the Apache License, Version 2.0 (the "License");
+ you may not use this file except in compliance with the License.
+ You may obtain a copy of the License at
+
+     http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+*/
+
+/* Memory regions */
+MEMORY
+{
+  /* Ram configurations are for smallest KL25 in family - 4K */
+  m_all        (rwx) : ORIGIN = 0x00000000, LENGTH = 0x600
+}
+
+/* Define output sections */
+SECTIONS
+{
+
+  .text :
+  {
+    . = ALIGN(4);
+
+    *(.text)           /* .text sections (code) */
+    *(.text*)          /* .text* sections (code) */
+
+    . = ALIGN(4);
+    *(.data)           /* .data sections */
+    *(.data*)          /* .data* sections */
+
+    . = ALIGN(4);
+    *(.bss)
+    *(.bss*)
+    *(COMMON)
+
+    . = ALIGN(4);
+    *(.rodata)         /* .rodata sections (constants, strings, etc.) */
+    *(.rodata*)        /* .rodata* sections (constants, strings, etc.) */
+
+  } >m_all
+
+}