Re: [PATCH] hw/riscv: fix build error with clang

[Daniel Henrique Barboza <dbarboza@ventanamicro.com>]

(Ccing Tomasz)

On 11/1/24 2:48 PM, Peter Maydell wrote:
> On Fri, 1 Nov 2024 at 17:36, Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>>
>>
>> On 11/1/24 2:08 PM, Pierrick Bouvier wrote:
>>> Introduced in 0c54ac, "hw/riscv: add RISC-V IOMMU base emulation"
>>>
>>> ../hw/riscv/riscv-iommu.c:187:17: error: redefinition of '_pext_u64'
>>>
>>>     187 | static uint64_t _pext_u64(uint64_t val, uint64_t ext)
>>>
>>>         |                 ^
>>>
>>> D:/a/_temp/msys64/clang64/lib/clang/18/include/bmi2intrin.h:217:1: note: previous definition is here
>>>
>>>     217 | _pext_u64(unsigned long long __X, unsigned long long __Y)
>>>
>>>         | ^
>>>
>>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>>> ---
>>>    hw/riscv/riscv-iommu.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/riscv/riscv-iommu.c b/hw/riscv/riscv-iommu.c
>>> index feb650549ac..f738570bac2 100644
>>> --- a/hw/riscv/riscv-iommu.c
>>> +++ b/hw/riscv/riscv-iommu.c
>>> @@ -184,7 +184,7 @@ static void riscv_iommu_pri(RISCVIOMMUState *s,
>>>    }
>>>
>>>    /* Portable implementation of pext_u64, bit-mask extraction. */
>>> -static uint64_t _pext_u64(uint64_t val, uint64_t ext)
>>> +static uint64_t pext_u64(uint64_t val, uint64_t ext)
>>
>> I suggest name it 'riscv_iommu_pext_u64' to be clear that this is a local scope function,
>> not to be mistaken with anything available in clang or any other compiler.
> 
> More generally, we should avoid using leading '_' in QEMU function
> names; those are reserved for the system.
> 
> Also, what does this function do? The comment assumes that
> the reader knows what a "pext_u64" function does, but if you
> don't then it's fairly inscrutable bit-twiddling.
> "bit-mask extraction" suggests maybe we should be using
> the bitops.h extract functions instead ?

This is the function:


/* Portable implementation of pext_u64, bit-mask extraction. */
static uint64_t _pext_u64(uint64_t val, uint64_t ext)
{
     uint64_t ret = 0;
     uint64_t rot = 1;

     while (ext) {
         if (ext & 1) {
             if (val & 1) {
                 ret |= rot;
             }
             rot <<= 1;
         }
         val >>= 1;
         ext >>= 1;
     }

     return ret;
}

Taking a look at bitops.h I'm not sure if we have a function that does that. Perhaps
we have a similar function that does that in another helper somewhere. I wouldn't
oppose using a common helper if available.

In fact I think this is not the first time we're having this talk, i.e. RISC-V code
using exclusive bitops/mask functions. target/riscv/cpu.c has a lot of these cases.
There's a whole infrastructure that ARM uses that handles regs operations that we
could use in that file, for example.

I can take a look at how we can standardize existing RISC-V code to use common helpers,
creating more common helpers if needed. Would be a 10.0 work since we're too late
for 9.2.


Thanks,

Daniel

> 
> thanks
> -- PMM