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

[Peter Maydell <peter.maydell@linaro.org>]

On Fri, 1 Nov 2024 at 18:13, Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
> (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;
> }

Yes, but what does it actually *do* ? :-)  Presumably
it extracts some subpart of 'val' based on 'ext', but
what is the format it expects 'ext' to be in, and what
kinds of input are valid?

For comparison, our extract64 function says:

 * extract64:
 * @value: the value to extract the bit field from
 * @start: the lowest bit in the bit field (numbered from 0)
 * @length: the length of the bit field
 *
 * Extract from the 64 bit input @value the bit field specified by the
 * @start and @length parameters, and return it. The bit field must
 * lie entirely within the 64 bit word. It is valid to request that
 * all 64 bits are returned (ie @length 64 and @start 0).

so even if you haven't come across it before you can see
what the function is intended to do, what inputs are valid
and what are not, and so on, and you don't need to try to
reverse-engineer those from the bit operations.

I'm not necessarily opposed to having separate implementations
of these things if it means the code follows the architectural
specifications more closely, but if we do have them can
we have documentation comments that describe the behaviour?

thanks
-- PMM