I requested access to scan.coverity.com.  Once it is granted, I'll take a look.

Thanks,
Taylor


> -----Original Message-----
> From: Peter Maydell <peter.maydell@linaro.org>
> Sent: Friday, February 19, 2021 4:52 AM
> To: Richard Henderson <richard.henderson@linaro.org>
> Cc: QEMU Developers <qemu-devel@nongnu.org>; Taylor Simpson
> <tsimpson@quicinc.com>
> Subject: Re: [PULL v2 00/35] hexagon initial commit
>
> -------------------------------------------------------------------------
> CAUTION: This email originated from outside of the organization.
> -------------------------------------------------------------------------
>
> On Thu, 18 Feb 2021 at 16:29, Richard Henderson
> <richard.henderson@linaro.org> wrote:
> > ----------------------------------------------------------------
> > Initial commit for the Qualcomm Hexagon processor.
> >
> > ----------------------------------------------------------------
>
> Hi; Coverity Scan reports a pile of new issues in the Hexagon
> code; could one of you go through them and confirm whether they
> are either false positives or else provide fixes for them, please?
>
> thanks
> -- PMM

On 2/19/21 8:30 AM, Taylor Simpson wrote:
> I requested access to scan.coverity.com.  Once it is granted, I'll take a look.

I took a quick look.  Quite a lot of the errors are related to

> #define fASHIFTL(SRC, SHAMT, REGSTYPE) \
>     (((SHAMT) >= 64) ? 0 : (fCAST##REGSTYPE##s(SRC) << (SHAMT)))

and

> #define fLSHIFTR(SRC, SHAMT, REGSTYPE) \
>     (((SHAMT) >= 64) ? 0 : (fCAST##REGSTYPE##u(SRC) >> (SHAMT)))

Coverity does not look beyond the leading comparison to inform the bounds, and
these macros are used with a 32-bit type.  It then warns that the shift could
be out of bounds.

It appears that none of the uses of fASHIFTL can actually overflow the shift count:

  * S2_asl_i has a 5-bit immediate shift count.
  * S2_addasl_rrri has a 3-bit immediate shift count.
  * S2_valign has a 3-bit scaled immediate shift count
    (on a 64-bit type).

So it looks like you could simply drop the tests entirely.
If you really want to keep it, then you should make use of REGSTYPE and bound
based on that.


r~