On 24/10/2023 08:14, Jan Beulich wrote:
On 23.10.2023 22:44, Stefano Stabellini wrote:
On Mon, 23 Oct 2023, Jan Beulich wrote:
On 23.10.2023 15:19, Nicola Vetrini wrote:
On 23/10/2023 11:19, Nicola Vetrini wrote:
On 23/10/2023 09:48, Jan Beulich wrote:
On 20.10.2023 17:28, Nicola Vetrini wrote:
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -246,6 +246,12 @@ constant expressions are required.\""
"any()"}
-doc_end
+-doc_begin="The macro LOWEST_BIT encapsulates a well-known
pattern
to obtain the value
+2^ffs(x) for unsigned integers on two's complement architectures
+(all the architectures supported by Xen satisfy this
requirement)."
+-config=MC3R1.R10.1,reports+={safe,
"any_area(any_loc(any_exp(macro(^LOWEST_BIT$))))"}
+-doc_end
This being deviated this way (rather than by SAF-* comments) wants
justifying in the description. You did reply to my respective
comment on v2, but such then (imo) needs propagating into the
actual
patch as well.
Sure, will do.
--- a/docs/misra/deviations.rst
+++ b/docs/misra/deviations.rst
@@ -192,6 +192,13 @@ Deviations related to MISRA C:2012 Rules:
See automation/eclair_analysis/deviations.ecl for the
full
explanation.
- Tagged as `safe` for ECLAIR.
+ * - R10.1
+ - The well-known pattern (x & -x) applied to unsigned
integer
values on 2's
+ complement architectures (i.e., all architectures
supported
by Xen), used
+ to obtain the value 2^ffs(x), where ffs(x) is the
position of
the first
+ bit set. If no bits are set, zero is returned.
+ - Tagged as `safe` for ECLAIR.
In such an explanation there shall not be any ambiguity. Here I
see
an issue with ffs() returning 1-based bit position numbers, which
isn't in line with the use in 2^ffs(x). (Arguably use of ^ itself
is
also problematic, as that's XOR in C, not POW. I'd suggest
2^^ffs(x)
or 2**ffs(x).)
Makes sense, I think I'll use an plain english explanation to avoid
any confusion
about notation.
--- a/xen/include/xen/macros.h
+++ b/xen/include/xen/macros.h
@@ -8,8 +8,11 @@
#define DIV_ROUND(n, d) (((n) + (d) / 2) / (d))
#define DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
-#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
-#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
+/* Returns the 2^ffs(x) or 0, where ffs(x) is the index of the
lowest set bit */
+#define LOWEST_BIT(x) ((x) & -(x))
I'm afraid my concern regarding this new macro's name (voiced on
v2)
hasn't
been addressed (neither verbally nor by finding a more suitable
name).
Jan
I didn't come up with much better names, so I left it as is. Here's
a
few ideas:
- LOWEST_SET
- MASK_LOWEST_SET
- MASK_LOWEST_BIT
- MASK_FFS_0
- LOWEST_ONE
and also yours, included here for convenience, even though you felt
it
was too long:
- ISOLATE_LOW_BIT()
All maintainers from THE REST are CC-ed, so we can see if anyone
has
any suggestion.
There's also LOWEST_BIT_MASK as another possible name.
While naming-wise okay to me, it has the same "too long" issue as
ISOLATE_LOW_BIT(). Considering x86, in the BMI ISA extension, has an
insn doing exactly that (BLSI), taking inspiration from its mnemonic
may also be an option.
I don't mean to make things difficult but I prefer LOWEST_BIT or
MASK_LOWEST_BIT. It is clear at first glance. BLSI is not as clear,
unless you work on the specific ISA with BLSI.
In general, I do appreciate shorter names, but if one option is much
clearer than the other, I prefer clarity over shortness.
That's fine with me, but note that neither LOWEST_BIT nor
MASK_LOWEST_BIT
really provide the aimed at clarity. The shortest that I could think of
that would be derived from that would be LOWEST_SET_BIT_MASK() (albeit
even that leaves a bit of ambiguity, thinking about it for a little
longer). The main point I'm trying to make that _if_ we need a wrapper
macro for this in the first place (note the other thread about macros
still requiring deviation comments at all use sites for Eclair), its
name
needs to somehow express the precise operation it does (or, like would
be
the case for BLSI, make people not recognizing the name go look rather
than guess).
Jan
Ok, I'll send a revised version using MASK_LOWEST_BIT, taking into
account also the
other comments about the explanation on the macro definition
(which some IDEs even show when hovering on its usage, which could
partially address
the latter concern).
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
