On 02/09/2024 9:06 am, Jan Beulich wrote:
> On 30.08.2024 22:03, Andrew Cooper wrote:
>> On 29/08/2024 3:36 pm, Jan Beulich wrote:
>>> On 29.08.2024 00:03, Andrew Cooper wrote:
>>>> It has existed in x86 CPUs since 2008, so we're only 16 years late adding
>>>> support. With all the other scafolding in place, implement arch_hweightl()
>>>> for x86.
>>>>
>>>> The only complication is that the call to arch_generic_hweightl() is behind
>>>> the compilers back. Address this by writing it in ASM and ensure that it
>>>> preserves all registers.
>>>>
>>>> Copy the code generation from generic_hweightl(). It's not a complicated
>>>> algorithm, and is easy to regenerate if needs be, but cover it with the
>>>> same
>>>> unit tests as test_generic_hweightl() just for piece of mind.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
>>>> ---
>>>> CC: Jan Beulich <jbeul...@suse.com>
>>>> CC: Roger Pau Monné <roger....@citrix.com>
>>>>
>>>> v2:
>>>> * Fix MISRA 8.2 (parameter name) and 8.5 (single declaration) regressions.
>>>> * Rename {arch->x86}-generic-hweightl.{S->c}
>>>> * Adjust ASM formating
>>>>
>>>> The __constructor trick to cause any reference to $foo() to pull in
>>>> test_$foo() only works when both are in the same TU. i.e. what I did in
>>>> v1 (putting test_arch_generic_hweightl() in the regular generic-hweightl.c)
>>>> didn't work.
>>> I'm afraid I don't understand this. What exactly didn't work, breaking in
>>> which
>>> way? Presumably as much as you, I don't really like the global asm() in a C
>>> file, when ideally the same could be written with less clutter in an
>>> assembly
>>> one.
>> We have lib-y because we wish to not include arch_generic_hweightl() if
>> it isn't referenced.
>>
>> But, test_arch_generic_hweightl() unconditionally references
>> arch_generic_hweightl() (in CONFIG_SELF_TESTS builds).
> Which I assumed was intentional: Always have the tests, to make sure the code
> doesn't go stale (or even break).
Come to think of it, we already have two subtlety different things.
common/bitops.c is SELF_TESTS only, and does both compile time
(constant-folding) and runtime (non-folded) tests on the top-level APIs.
This checks that the compiler agrees with the test parameters that I (or
hopefully in the future, others) have chosen, and that every arch_$foo()
override agrees with the compiler version.
It has already found bugs. Both __builtin foo() vs fool() mismatches
and I dread to think how long a bugs like that could have gone
unnoticed. I caught ppc's arch_hweightl() before posting v1, but noone
noticed ARM's arch_flsl() snafu when it was on the list (I found it when
retrofitting SELF_TESTS around the series).
Separately, anything that causes any {arch_,}generic_$foo()'s to be
included also pulls in the SELF_TEST for that specific function.
They're runtime only tests (no constant folding in an intentionally
out-of-line function).
But, it was intentional to try and make CONFIG_SELF_TESTS not blindly
pull in the unused generic_$foo()'s. They'll get tested if they are
referenced in a build, but the tests are only liable to break are during
bitops development or a new/weird arch/environments.
Furthermore, I expect (/hope) that we'll get to the point where we're
being conservative with selftests. It's part of why I'm trying to keep
the current ones pretty lean.
> Looking at the v2 code again I notice that while now you're running the tests
> only when the to-be-tested function is referenced from elsewhere, the testing
> has become independent of CONFIG_SELF_TESTS. I don't think that was intended?
Correct - that was an oversight. The CONFIG_SELF_TESTS guard wants to
go back in.
~Andrew
