Repository: pyocd/pyocd
Issue: memoryMap bound to class creates concurrency issues

There is an issue where, when you have created multiple boards of the same type and try to flash/erase them in parallel you get concurrency issues.

This is in some part due to the fact that the MemoryMap for the targets are bound to the class rather than the instance. This means that associated FlashRegions are not unique per board instance and these have some mutating state in which you get pretty reliable race-conditions.

This was verified by overloading the builtin targets and moving MemoryMap() to __init__. It would be better to have this problem fixed in pyocd though.

Changed files:
pyocd/core/memory_map.py, pyocd/core/target.py, test/unit/test_memory_map.py

diff --git a/pyocd/core/memory_map.py b/pyocd/core/memory_map.py
index b56886e9..1ae794cf 100644
--- a/pyocd/core/memory_map.py
+++ b/pyocd/core/memory_map.py
@@ -16,6 +16,7 @@
 
 from enum import Enum
 import six
+import copy
 from functools import total_ordering
 
 class MemoryType(Enum):
@@ -229,10 +230,33 @@ class MemoryRegion(MemoryRangeBase):
             return aliasValue
         
     def __getattr__(self, name):
-        v = self._attributes[name]
-        if callable(v):
-            v = v(self)
-        return v
+        try:
+            v = self._attributes[name]
+        except KeyError:
+            # Transform the KeyError from a missing attribute to the expected AttributeError.
+            raise AttributeError(name)
+        else:
+            if callable(v):
+                v = v(self)
+            return v
+
+    def __copy__(self):
+        # Custom copy is required due to our __getattr__() method.
+        return self.__class__(
+                # type=self.type,
+                start=self.start,
+                length=self.length,
+                **self._attributes
+                )
+ 
+    def __deepcopy__(self, memo):
+        # Same as __copy__ but with a deep copy of attributes.
+        return self.__class__(
+                # type=self.type,
+                start=self.start,
+                length=self.length,
+                **copy.deepcopy(self._attributes, memo)
+                )
 
     def __repr__(self):
         return "<%s@0x%x name=%s type=%s start=0x%x end=0x%x length=0x%x access=%s>" % (self.__class__.__name__, id(self), self.name, self.type, self.start, self.end, self.length, self.access)
@@ -240,7 +264,8 @@ class MemoryRegion(MemoryRangeBase):
 class RamRegion(MemoryRegion):
     """! @brief Contiguous region of RAM."""
     def __init__(self, start=0, end=0, length=None, **attrs):
-        super(RamRegion, self).__init__(type=MemoryType.RAM, start=start, end=end, length=length, **attrs)
+        attrs['type'] = MemoryType.RAM
+        super(RamRegion, self).__init__(start=start, end=end, length=length, **attrs)
 
 class RomRegion(MemoryRegion):
     """! @brief Contiguous region of ROM."""
@@ -252,7 +277,8 @@ class RomRegion(MemoryRegion):
         })
 
     def __init__(self, start=0, end=0, length=None, **attrs):
-        super(RomRegion, self).__init__(type=MemoryType.ROM, start=start, end=end, length=length, **attrs)
+        attrs['type'] = MemoryType.ROM
+        super(RomRegion, self).__init__(start=start, end=end, length=length, **attrs)
 
 class DefaultFlashWeights:
     """! @brief Default weights for flash programming operations."""
@@ -304,7 +330,8 @@ class FlashRegion(MemoryRegion):
         from ..flash.flash import Flash
 
         assert ('blocksize' in attrs) or ('sector_size' in attrs)
-        super(FlashRegion, self).__init__(type=MemoryType.FLASH, start=start, end=end, length=length, **attrs)
+        attrs['type'] = MemoryType.FLASH
+        super(FlashRegion, self).__init__(start=start, end=end, length=length, **attrs)
         self._algo = attrs.get('algo', None)
         self._flm = None
         self._flash = None
@@ -314,6 +341,16 @@ class FlashRegion(MemoryRegion):
             assert issubclass(self._flash_class, Flash)
         else:
             self._flash_class = Flash
+        
+        # Remove writable region attributes from attributes dict so there is only one copy.
+        try:
+            del self._attributes['algo']
+        except KeyError:
+            pass
+        try:
+            del self._attributes['flash_class']
+        except KeyError:
+            pass
     
     @property
     def algo(self):
@@ -359,6 +396,34 @@ class FlashRegion(MemoryRegion):
             if b != erasedByte:
                 return False
         return True
+    
+    def __copy__(self):
+        # Include the writable attributes in the copy.
+        clone = self.__class__(
+                # type=self.type,
+                start=self.start,
+                length=self.length,
+                algo=self._algo,
+                flash_class=self._flash_class,
+                **self._attributes
+                )
+        # Copy the FLM.
+        clone._flm = self._flm
+        return clone
+
+    def __deepcopy__(self, memo):
+        # Include deep copies of the writable attributes in the copy.
+        clone = self.__class__(
+                # type=self.type,
+                start=self.start,
+                length=self.length,
+                algo=copy.deepcopy(self._algo, memo),
+                flash_class=self._flash_class,
+                **copy.deepcopy(self._attributes, memo)
+                )
+        # Deep copy the FLM.
+        clone._flm = copy.deepcopy(self._flm, memo)
+        return clone
 
     def __repr__(self):
         return "<%s@0x%x name=%s type=%s start=0x%x end=0x%x length=0x%x access=%s blocksize=0x%x>" % (self.__class__.__name__, id(self), self.name, self.type, self.start, self.end, self.length, self.access, self.blocksize)
@@ -375,7 +440,8 @@ class DeviceRegion(MemoryRegion):
         })
 
     def __init__(self, start=0, end=0, length=None, **attrs):
-        super(DeviceRegion, self).__init__(type=MemoryType.DEVICE, start=start, end=end, length=length, **attrs)
+        attrs['type'] = MemoryType.DEVICE
+        super(DeviceRegion, self).__init__(start=start, end=end, length=length, **attrs)
 
 ## @brief Map from memory type to class.         
 MEMORY_TYPE_CLASS_MAP = {
@@ -463,6 +529,9 @@ class MemoryMap(object):
         for r in self.get_regions_of_type(type):
             return r
         return None
+    
+    def __eq__(self, other):
+        return isinstance(other, MemoryMap) and (self._regions == other._regions)
 
     def __iter__(self):
         """! @brief Enable iteration over the memory map."""
diff --git a/pyocd/core/target.py b/pyocd/core/target.py
index 8dc610d3..8dee60b9 100644
--- a/pyocd/core/target.py
+++ b/pyocd/core/target.py
@@ -14,9 +14,11 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+from enum import Enum
+import copy
+
 from .memory_interface import MemoryInterface
 from .memory_map import MemoryMap
-from enum import Enum
 
 class Target(MemoryInterface):
 
@@ -101,7 +103,9 @@ class Target(MemoryInterface):
         self.vendor = self.VENDOR
         self.part_families = []
         self.part_number = ""
-        self.memory_map = memoryMap or MemoryMap()
+        # Make a target-specific copy of the memory map. This is safe to do without locking
+        # because the memory map may not be mutated until target initialization.
+        self.memory_map = (copy.deepcopy(memoryMap)) if memoryMap else MemoryMap()
         self._svd_location = None
         self._svd_device = None
 
diff --git a/test/unit/test_memory_map.py b/test/unit/test_memory_map.py
index b2b5a343..8ec211f6 100644
--- a/test/unit/test_memory_map.py
+++ b/test/unit/test_memory_map.py
@@ -1,5 +1,5 @@
 # pyOCD debugger
-# Copyright (c) 2016 Arm Limited
+# Copyright (c) 2016-2019 Arm Limited
 # SPDX-License-Identifier: Apache-2.0
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
@@ -28,6 +28,7 @@ from pyocd.core.memory_map import (
     )
 import pytest
 import logging
+import copy
 
 @pytest.fixture(scope='function')
 def flash():
@@ -217,7 +218,7 @@ class TestMemoryRegion:
         assert rom.start == 0x1c000000
         assert rom.end == 0x1c003fff
         assert rom.length == 0x4000
-        with pytest.raises(KeyError):
+        with pytest.raises(AttributeError):
             rom.blocksize
         assert rom.name == 'rom'
         assert not rom.is_flash
@@ -235,7 +236,7 @@ class TestMemoryRegion:
         assert ram1.start == 0x20000000
         assert ram1.end == 0x200003ff
         assert ram1.length == 0x400
-        with pytest.raises(KeyError):
+        with pytest.raises(AttributeError):
             ram1.blocksize
         assert ram1.name == 'ram'
         assert not ram1.is_flash
@@ -253,7 +254,7 @@ class TestMemoryRegion:
         assert ram2.start == 0x20000400
         assert ram2.end == 0x200007ff
         assert ram2.length == 0x400
-        with pytest.raises(KeyError):
+        with pytest.raises(AttributeError):
             ram2.blocksize
         assert ram2.name == 'ram2'
         assert not ram2.is_flash
@@ -287,6 +288,27 @@ class TestMemoryRegion:
         assert ram1.intersects_range(0x20000020, length=0x10)
         assert ram1.intersects_range(0x1fff0000, end=0x20001000)
         assert ram1.intersects_range(0x1ffff000, length=0x40000)
+    
+    def test_copy_ram(self, ram1):
+        ramcpy = copy.copy(ram1)
+        assert ramcpy.type == ram1.type
+        assert ramcpy.start == ram1.start
+        assert ramcpy.end == ram1.end
+        assert ramcpy.length == ram1.length
+        assert ramcpy.name == ram1.name
+        assert ramcpy == ram1
+    
+    def test_deepcopy_ram(self, ram1):
+        ramcpy = copy.deepcopy(ram1)
+        assert ramcpy == ram1
+    
+    def test_copy_flash(self, flash):
+        flashcpy = copy.copy(flash)
+        assert flashcpy == flash
+    
+    def test_deepcopy_flash(self, flash):
+        flashcpy = copy.deepcopy(flash)
+        assert flashcpy == flash
 
 
 # MemoryMap test cases.
@@ -375,5 +397,20 @@ class TestMemoryMap:
     
     def test_alias(self, memmap2, ram2, ram_alias):
         assert ram_alias.alias is ram2
+    
+    def test_copy(self, memmap):
+        mapcpy = copy.copy(memmap)
+        assert id(mapcpy) != id(memmap)
+        assert id(mapcpy.get_first_region_of_type(MemoryType.RAM)) == \\
+            id(memmap.get_first_region_of_type(MemoryType.RAM))
+        assert mapcpy == memmap
+    
+    def test_deep_copy(test, memmap):
+        mapcpy = copy.deepcopy(memmap)
+        assert id(mapcpy) != id(memmap)
+        assert id(mapcpy.get_first_region_of_type(MemoryType.RAM)) != \\
+            id(memmap.get_first_region_of_type(MemoryType.RAM))
+        assert mapcpy == memmap
+