From 1e5fbf48775b9336b93a3f9fd30fa4931458628e Mon Sep 17 00:00:00 2001 From: GeorgeRG Date: Fri, 28 Aug 2026 13:35:09 +0200 Subject: [PATCH 1/2] fix(field_node): guard read-modify-write against onwrite/onread side-effect fields --- examples/generated/include/field_node.h | 21 ++++++- examples/generated/include/halcpp_utils.h | 67 +++++++++++++++++++++++ src/peakrdl_halcpp/halnode.py | 62 ++++++++++++++++++++- src/peakrdl_halcpp/include/field_node.h | 15 ++++- src/peakrdl_halcpp/include/halcpp_utils.h | 67 +++++++++++++++++++++++ src/peakrdl_halcpp/templates/addrmap.h.j2 | 10 ++++ 6 files changed, 238 insertions(+), 4 deletions(-) diff --git a/examples/generated/include/field_node.h b/examples/generated/include/field_node.h index 8e36e44..5faf9b2 100644 --- a/examples/generated/include/field_node.h +++ b/examples/generated/include/field_node.h @@ -53,7 +53,11 @@ namespace halcpp */ static constexpr uint32_t bit_mask() { - return (((1u << (width)) - 1)); + if constexpr (width >= 32) { + return ~0u; + } else { + return ((1u << width) - 1u); + } } @@ -95,7 +99,20 @@ namespace halcpp static inline void set(typename BASE_TYPE::dataType val) { if constexpr (node_has_get_v) - parent::set((parent::get() & BASE_TYPE::field_mask()) | ((val & BASE_TYPE::bit_mask()) << BASE_TYPE::start_bit)); + { + static_assert(!node_onread_hazard_v, + "This register contains a field with a destructive-read side effect " + "(onread=rclr/rset/ruser). Setting a single field requires a " + "read-modify-write, which would silently disturb that field. Write the " + "whole register instead (e.g. `my_reg = value;`)."); + + uint32_t reg_val = parent::get(); + if constexpr (node_has_onwrite_hazard_v) + reg_val = (reg_val & ~parent::onwrite_hazard_mask) + | (parent::onwrite_hazard_safe_val & parent::onwrite_hazard_mask); + + parent::set((reg_val & BASE_TYPE::field_mask()) | ((val & BASE_TYPE::bit_mask()) << BASE_TYPE::start_bit)); + } else parent::set(val << BASE_TYPE::start_bit); } diff --git a/examples/generated/include/halcpp_utils.h b/examples/generated/include/halcpp_utils.h index db1ded4..bff6246 100644 --- a/examples/generated/include/halcpp_utils.h +++ b/examples/generated/include/halcpp_utils.h @@ -127,6 +127,73 @@ namespace halcpp template constexpr bool node_has_get_v = node_has_get::value; + /** + * @brief Type trait to check if a type has an `onwrite_hazard_mask` static member. + * + * Used to detect (at compile time) whether a register type carries the write-side-effect + * masking constants emitted by the code generator, without requiring every possible + * parent type to define them. + * + * @tparam TYPE The type to check. + */ + template + struct node_has_onwrite_hazard : std::false_type + { + }; + + /** + * @brief Specialization of `node_has_onwrite_hazard` for types that define + * `onwrite_hazard_mask`. + * + * @tparam TYPE The type to check. + */ + template + struct node_has_onwrite_hazard> : std::true_type + { + }; + + /** + * @brief Convenience variable template for `node_has_onwrite_hazard`. + * + * @tparam TYPE The type to check. + */ + template + constexpr bool node_has_onwrite_hazard_v = node_has_onwrite_hazard::value; + + /** + * @brief Type trait exposing a register's `has_onread_hazard` flag, defaulting to false + * for types that don't define it. + * + * A register has an onread hazard when one of its fields has a destructive-read side + * effect (onread=rclr/rset/ruser). Reading such a register - even incidentally, as part + * of a read-modify-write to set a sibling field - silently disturbs it. + * + * @tparam TYPE The type to check. + */ + template + struct node_onread_hazard : std::false_type + { + }; + + /** + * @brief Specialization of `node_onread_hazard` for types that define `has_onread_hazard`. + * + * @tparam TYPE The type to check. + */ + template + struct node_onread_hazard> + : std::integral_constant + { + }; + + /** + * @brief Convenience variable template for `node_onread_hazard`. + * + * @tparam TYPE The type to check. + */ + template + constexpr bool node_onread_hazard_v = node_onread_hazard::value; + } #endif // !_HALCPP_UTILS_H_ diff --git a/src/peakrdl_halcpp/halnode.py b/src/peakrdl_halcpp/halnode.py index 98aa935..09509d2 100644 --- a/src/peakrdl_halcpp/halnode.py +++ b/src/peakrdl_halcpp/halnode.py @@ -1,4 +1,4 @@ -from typing import TYPE_CHECKING, Optional, Iterator, List +from typing import TYPE_CHECKING, Optional, Iterator, List, Tuple import itertools import logging @@ -302,6 +302,18 @@ def cpp_access_type(self) -> str: raise ValueError(f'Node field access rights are not found \ {self.inst.inst_name}') + @property + def onwrite(self) -> Optional[str]: + """Returns the SystemRDL 'onwrite' property name (e.g. 'woclr'), or None if unset.""" + onwrite_prop = self.get_property('onwrite') + return onwrite_prop.name if onwrite_prop is not None else None + + @property + def onread(self) -> Optional[str]: + """Returns the SystemRDL 'onread' property name (e.g. 'rclr'), or None if unset.""" + onread_prop = self.get_property('onread') + return onread_prop.name if onread_prop is not None else None + def get_enums(self): """Returns the enumeration(s) of a FieldNode. @@ -379,6 +391,54 @@ def width(self) -> int: """Returns the register width in bits, derived from the highest bit position of its fields.""" return max([c.high for c in self.halchildren(HalFieldNode)]) + 1 + # onwrite values where writing 0 is always a no-op (safe to echo back 0) + _ONWRITE_ZERO_SAFE = {'woset', 'woclr', 'wot'} + # onwrite values where writing 1 is always a no-op (safe to echo back 1) + _ONWRITE_ONE_SAFE = {'wzs', 'wzc', 'wzt'} + # onwrite values that trigger unconditionally on any write; no safe echo value exists + _ONWRITE_UNSAFE = {'wclr', 'wset', 'wuser'} + + def get_onwrite_hazard(self) -> Tuple[int, int]: + """Computes the bitmask/safe-echo-value needed to avoid disturbing sibling fields + with write side effects (onwrite=woclr/woset/... etc.) when another field in the + same register is set via read-modify-write. + + Returns + ------- + tuple + ``(mask, safe_val)`` where ``mask`` has a bit set for every bit position + belonging to a field with a maskable onwrite side effect, and ``safe_val`` + holds the no-op bit value (0 or 1) to echo back for each of those bits. + """ + mask = 0 + safe_val = 0 + for f in self.halchildren(HalFieldNode): + onwrite = f.onwrite + if onwrite is None: + continue + field_bits = ((1 << (f.high - f.low + 1)) - 1) << f.low + if onwrite in HalRegNode._ONWRITE_UNSAFE: + halnode_logger.warning( + f"Field '{f.inst_name}' in register '{self.inst_name}' has onwrite={onwrite}, " + f"which triggers unconditionally on any write to the register. This cannot be " + f"protected against in software; writing any other field in this register will " + f"disturb '{f.inst_name}'.") + continue + mask |= field_bits + if onwrite in HalRegNode._ONWRITE_ONE_SAFE: + safe_val |= field_bits + return mask, safe_val + + def has_onread_hazard(self) -> bool: + """Returns True if any field in this register has an onread side effect + (e.g. rclr, rset), making a full register read destructive. + + When True, per-field set() (which performs a read-modify-write) cannot be used + safely on this register: a mere incidental read of the register (needed to + preserve the other fields' values) would silently disturb this field. + """ + return any(f.onread is not None for f in self.halchildren(HalFieldNode)) + def get_template_line(self) -> str: """Returns the class template string.""" return f"template " diff --git a/src/peakrdl_halcpp/include/field_node.h b/src/peakrdl_halcpp/include/field_node.h index 500a2f0..e893dc6 100644 --- a/src/peakrdl_halcpp/include/field_node.h +++ b/src/peakrdl_halcpp/include/field_node.h @@ -99,7 +99,20 @@ namespace halcpp static inline void set(typename BASE_TYPE::dataType val) { if constexpr (node_has_get_v) - parent::set((parent::get() & BASE_TYPE::field_mask()) | ((val & BASE_TYPE::bit_mask()) << BASE_TYPE::start_bit)); + { + static_assert(!node_onread_hazard_v, + "This register contains a field with a destructive-read side effect " + "(onread=rclr/rset/ruser). Setting a single field requires a " + "read-modify-write, which would silently disturb that field. Write the " + "whole register instead."); + + uint32_t reg_val = parent::get(); + if constexpr (node_has_onwrite_hazard_v) + reg_val = (reg_val & ~parent::onwrite_hazard_mask) + | (parent::onwrite_hazard_safe_val & parent::onwrite_hazard_mask); + + parent::set((reg_val & BASE_TYPE::field_mask()) | ((val & BASE_TYPE::bit_mask()) << BASE_TYPE::start_bit)); + } else parent::set(val << BASE_TYPE::start_bit); } diff --git a/src/peakrdl_halcpp/include/halcpp_utils.h b/src/peakrdl_halcpp/include/halcpp_utils.h index db1ded4..bff6246 100644 --- a/src/peakrdl_halcpp/include/halcpp_utils.h +++ b/src/peakrdl_halcpp/include/halcpp_utils.h @@ -127,6 +127,73 @@ namespace halcpp template constexpr bool node_has_get_v = node_has_get::value; + /** + * @brief Type trait to check if a type has an `onwrite_hazard_mask` static member. + * + * Used to detect (at compile time) whether a register type carries the write-side-effect + * masking constants emitted by the code generator, without requiring every possible + * parent type to define them. + * + * @tparam TYPE The type to check. + */ + template + struct node_has_onwrite_hazard : std::false_type + { + }; + + /** + * @brief Specialization of `node_has_onwrite_hazard` for types that define + * `onwrite_hazard_mask`. + * + * @tparam TYPE The type to check. + */ + template + struct node_has_onwrite_hazard> : std::true_type + { + }; + + /** + * @brief Convenience variable template for `node_has_onwrite_hazard`. + * + * @tparam TYPE The type to check. + */ + template + constexpr bool node_has_onwrite_hazard_v = node_has_onwrite_hazard::value; + + /** + * @brief Type trait exposing a register's `has_onread_hazard` flag, defaulting to false + * for types that don't define it. + * + * A register has an onread hazard when one of its fields has a destructive-read side + * effect (onread=rclr/rset/ruser). Reading such a register - even incidentally, as part + * of a read-modify-write to set a sibling field - silently disturbs it. + * + * @tparam TYPE The type to check. + */ + template + struct node_onread_hazard : std::false_type + { + }; + + /** + * @brief Specialization of `node_onread_hazard` for types that define `has_onread_hazard`. + * + * @tparam TYPE The type to check. + */ + template + struct node_onread_hazard> + : std::integral_constant + { + }; + + /** + * @brief Convenience variable template for `node_onread_hazard`. + * + * @tparam TYPE The type to check. + */ + template + constexpr bool node_onread_hazard_v = node_onread_hazard::value; + } #endif // !_HALCPP_UTILS_H_ diff --git a/src/peakrdl_halcpp/templates/addrmap.h.j2 b/src/peakrdl_halcpp/templates/addrmap.h.j2 index a146716..1171fc5 100644 --- a/src/peakrdl_halcpp/templates/addrmap.h.j2 +++ b/src/peakrdl_halcpp/templates/addrmap.h.j2 @@ -51,6 +51,11 @@ namespace {{ halnode.orig_type_name }}_nm public: using TYPE = {{ r.orig_type_name|upper }}{{ r.get_cls_tmpl_params() }}; + {% set hz_mask, hz_safe_val = r.get_onwrite_hazard() %} + static constexpr uint32_t onwrite_hazard_mask = 0x{{ "%0x"|format(hz_mask) }}; + static constexpr uint32_t onwrite_hazard_safe_val = 0x{{ "%0x"|format(hz_safe_val) }}; + static constexpr bool has_onread_hazard = {{ "true" if r.has_onread_hazard() else "false" }}; + {# Add the fields to the register #} {% for f in r.halchildren(children_type=HalFieldNode, skip_buses=skip_buses) %} static halcpp::{{ f.cpp_access_type }}<{{ f.low }}, {{ f.high }}, TYPE> {{ f.inst_name }}; @@ -91,6 +96,11 @@ namespace {{ halnode.orig_type_name }}_nm public: using TYPE = {{ r.orig_type_name|upper }}{{ r.get_cls_tmpl_params() }}; + {% set hz_mask, hz_safe_val = r.get_onwrite_hazard() %} + static constexpr uint32_t onwrite_hazard_mask = 0x{{ "%0x"|format(hz_mask) }}; + static constexpr uint32_t onwrite_hazard_safe_val = 0x{{ "%0x"|format(hz_safe_val) }}; + static constexpr bool has_onread_hazard = {{ "true" if r.has_onread_hazard() else "false" }}; + {# Add the fields to the register #} {% for f in r.halchildren(children_type=HalFieldNode, skip_buses=skip_buses) %} static halcpp::{{ f.cpp_access_type }}<{{ f.low }}, {{ f.high }}, TYPE> {{ f.inst_name }}; From 994b861b65f001b52f085f6d96ae127bfce58d96 Mon Sep 17 00:00:00 2001 From: GeorgeRG Date: Fri, 28 Aug 2026 13:35:15 +0200 Subject: [PATCH 2/2] chore(examples): regenerate generated headers --- examples/generated/atxmega_spi_hal.h | 16 ++++++++++++++++ examples/generated/regs_and_mem_hal.h | 8 ++++++++ 2 files changed, 24 insertions(+) diff --git a/examples/generated/atxmega_spi_hal.h b/examples/generated/atxmega_spi_hal.h index 773d012..54581d9 100644 --- a/examples/generated/atxmega_spi_hal.h +++ b/examples/generated/atxmega_spi_hal.h @@ -19,6 +19,10 @@ namespace atxmega_spi_nm public: using TYPE = CTRL; + static constexpr uint32_t onwrite_hazard_mask = 0x0; + static constexpr uint32_t onwrite_hazard_safe_val = 0x0; + static constexpr bool has_onread_hazard = false; + static halcpp::FieldRW<0, 1, TYPE> PRESCALER; static halcpp::FieldRW<2, 3, TYPE> MODE; static halcpp::FieldRW<4, 4, TYPE> MASTER; @@ -37,6 +41,10 @@ namespace atxmega_spi_nm public: using TYPE = INTCTRL; + static constexpr uint32_t onwrite_hazard_mask = 0x0; + static constexpr uint32_t onwrite_hazard_safe_val = 0x0; + static constexpr bool has_onread_hazard = false; + static halcpp::FieldRW<0, 1, TYPE> INTLVL; using halcpp::RegRW::operator=; @@ -50,6 +58,10 @@ namespace atxmega_spi_nm public: using TYPE = STATUS; + static constexpr uint32_t onwrite_hazard_mask = 0x0; + static constexpr uint32_t onwrite_hazard_safe_val = 0x0; + static constexpr bool has_onread_hazard = false; + static halcpp::FieldRO<6, 6, TYPE> WRCOL; static halcpp::FieldRO<7, 7, TYPE> IF; }; @@ -68,6 +80,10 @@ namespace atxmega_spi_nm public: using TYPE = DATA; + static constexpr uint32_t onwrite_hazard_mask = 0x0; + static constexpr uint32_t onwrite_hazard_safe_val = 0x0; + static constexpr bool has_onread_hazard = false; + static halcpp::FieldWO<0, 7, TYPE> WDATA; static halcpp::FieldRO<0, 7, TYPE> RDATA; diff --git a/examples/generated/regs_and_mem_hal.h b/examples/generated/regs_and_mem_hal.h index 69143bf..6e35fa6 100644 --- a/examples/generated/regs_and_mem_hal.h +++ b/examples/generated/regs_and_mem_hal.h @@ -19,6 +19,10 @@ namespace regs_and_mem_nm public: using TYPE = CSR; + static constexpr uint32_t onwrite_hazard_mask = 0x0; + static constexpr uint32_t onwrite_hazard_safe_val = 0x0; + static constexpr bool has_onread_hazard = false; + static halcpp::FieldRW<0, 7, TYPE> ctrl; using halcpp::RegRW::operator=; @@ -32,6 +36,10 @@ namespace regs_and_mem_nm public: using TYPE = CSR2; + static constexpr uint32_t onwrite_hazard_mask = 0x0; + static constexpr uint32_t onwrite_hazard_safe_val = 0x0; + static constexpr bool has_onread_hazard = false; + static halcpp::FieldRW<0, 7, TYPE> ctrl; using halcpp::RegRW::operator=;