Help finding Coverity defects for generated Hexagon code

Help finding Coverity defects for generated Hexagon code

[Anton Johansson via]

[-- Attachment #1: Type: text/plain, Size: 559 bytes --]

Hi,

coverity recently reported some defects in code generated by idef-parser
(email attached). These defects are expected and we plan to emit a
/* coverity[event_tag] */ comment to disable the specific event triggered.

However, I'm not able to find the event_tag as I can't find the defect in
the QEMU coverity project, and the link in the email simply brings me
to the main project page. I've tried sorting through the defects and
searching for the CID without luck.

Any ideas? I'm not super familiar with coverity.

-- 
Anton Johansson,
rev.ng Labs Srl.

[-- Attachment #2: New Defects reported by Coverity Scan for QEMU.eml --]
[-- Type: message/rfc822, Size: 11616 bytes --]

From: scan-admin@coverity.com
To: anjo@rev.ng
Subject: New Defects reported by Coverity Scan for QEMU
Date: Fri, 19 May 2023 20:16:19 +0000 (UTC)
Message-ID: <6467d9133bd9f_57b172b16e2c9d98835043@prd-scan-dashboard-0.mail>

Hi,

Please find the latest report on new defect(s) introduced to QEMU found with Coverity Scan.

5 new defect(s) introduced to QEMU found with Coverity Scan.
9 defect(s), reported by Coverity Scan earlier, were marked fixed in the recent build analyzed by Coverity Scan.

New defect(s) Reported-by: Coverity Scan
Showing 5 of 5 defect(s)


** CID 1512512:  Control flow issues  (DEADCODE)
/target/hexagon/idef-generated-emitter.indented.c: 32297 in emit_F2_dfmpylh()


________________________________________________________________________________________________________
*** CID 1512512:  Control flow issues  (DEADCODE)
/target/hexagon/idef-generated-emitter.indented.c: 32297 in emit_F2_dfmpylh()
32291     tcg_gen_ori_i64(tmp_3, tmp_2, qemu_tmp_1);
32292     TCGv_i64 tmp_4 = tcg_temp_new_i64();
32293     tcg_gen_mul_i64(tmp_4, tmp_0, tmp_3);
32294     int64_t qemu_tmp_2 = (int64_t)((int32_t) 0x1);
32295     TCGv_i64 tmp_5 = tcg_temp_new_i64();
32296     if (qemu_tmp_2 >= 64) {
>>>     CID 1512512:  Control flow issues  (DEADCODE)
>>>     Execution cannot reach this statement: "tcg_gen_movi_i64(tmp_5, 0L);".
32297     tcg_gen_movi_i64(tmp_5, 0);
32298     } else {
32299     tcg_gen_shli_i64(tmp_5, tmp_4, qemu_tmp_2);
32300     }
32301     TCGv_i64 tmp_6 = tcg_temp_new_i64();
32302     tcg_gen_add_i64(tmp_6, RxxV, tmp_5);

** CID 1512511:  Control flow issues  (DEADCODE)
/target/hexagon/idef-generated-emitter.indented.c: 32257 in emit_F2_dfmpyll()


________________________________________________________________________________________________________
*** CID 1512511:  Control flow issues  (DEADCODE)
/target/hexagon/idef-generated-emitter.indented.c: 32257 in emit_F2_dfmpyll()
32251     } else {
32252     tcg_gen_shri_i64(tmp_3, prod, qemu_tmp_0);
32253     }
32254     int64_t qemu_tmp_1 = (int64_t)((int32_t) 0x1);
32255     TCGv_i64 tmp_4 = tcg_temp_new_i64();
32256     if (qemu_tmp_1 >= 64) {
>>>     CID 1512511:  Control flow issues  (DEADCODE)
>>>     Execution cannot reach this statement: "tcg_gen_movi_i64(tmp_4, 0L);".
32257     tcg_gen_movi_i64(tmp_4, 0);
32258     } else {
32259     tcg_gen_shli_i64(tmp_4, tmp_3, qemu_tmp_1);
32260     }
32261     tcg_gen_mov_i64(RddV, tmp_4);
32262     TCGv_i64 tmp_5 = tcg_temp_new_i64();

** CID 1512510:  Control flow issues  (DEADCODE)
/target/hexagon/idef-generated-emitter.indented.c: 16045 in emit_M2_dpmpyss_rnd_s0()


________________________________________________________________________________________________________
*** CID 1512510:  Control flow issues  (DEADCODE)
/target/hexagon/idef-generated-emitter.indented.c: 16045 in emit_M2_dpmpyss_rnd_s0()
16039     tcg_gen_addi_i64(tmp_3, tmp_2, qemu_tmp_0);
16040     int64_t qemu_tmp_1 = (int64_t)((int32_t) 0x20);
16041     TCGv_i64 tmp_4 = tcg_temp_new_i64();
16042     {
16043     int64_t shift = qemu_tmp_1;
16044     if (qemu_tmp_1 >= 64) {
>>>     CID 1512510:  Control flow issues  (DEADCODE)
>>>     Execution cannot reach this statement: "shift = 63L;".
16045         shift = 64 - 1;
16046     }
16047     tcg_gen_sari_i64(tmp_4, tmp_3, shift);
16048     }
16049     TCGv_i32 tmp_5 = tcg_temp_new_i32();
16050     tcg_gen_trunc_i64_tl(tmp_5, tmp_4);

** CID 1512509:  Control flow issues  (DEADCODE)
/target/hexagon/idef-generated-emitter.indented.c: 32287 in emit_F2_dfmpylh()


________________________________________________________________________________________________________
*** CID 1512509:  Control flow issues  (DEADCODE)
/target/hexagon/idef-generated-emitter.indented.c: 32287 in emit_F2_dfmpylh()
32281     tcg_gen_extract_i64(tmp_1, RttV, ((int32_t) 0x1) * 32, 32);
32282     int64_t qemu_tmp_0 = (int64_t)((int32_t) 0x14);
32283     TCGv_i64 tmp_2 = tcg_temp_new_i64();
32284     if (qemu_tmp_0 != 0) {
32285     tcg_gen_extract_i64(tmp_2, tmp_1, 0, qemu_tmp_0);
32286     } else {
>>>     CID 1512509:  Control flow issues  (DEADCODE)
>>>     Execution cannot reach this statement: "tcg_gen_movi_i64(tmp_2, 0L);".
32287     tcg_gen_movi_i64(tmp_2, 0);
32288     }
32289     int64_t qemu_tmp_1 = (int64_t)((int32_t) 0x100000);
32290     TCGv_i64 tmp_3 = tcg_temp_new_i64();
32291     tcg_gen_ori_i64(tmp_3, tmp_2, qemu_tmp_1);
32292     TCGv_i64 tmp_4 = tcg_temp_new_i64();

** CID 1512508:  Control flow issues  (DEADCODE)
/target/hexagon/idef-generated-emitter.indented.c: 32250 in emit_F2_dfmpyll()


________________________________________________________________________________________________________
*** CID 1512508:  Control flow issues  (DEADCODE)
/target/hexagon/idef-generated-emitter.indented.c: 32250 in emit_F2_dfmpyll()
32244     TCGv_i64 tmp_2 = tcg_temp_new_i64();
32245     tcg_gen_mul_i64(tmp_2, tmp_0, tmp_1);
32246     tcg_gen_mov_i64(prod, tmp_2);
32247     int64_t qemu_tmp_0 = (int64_t)((int32_t) 0x20);
32248     TCGv_i64 tmp_3 = tcg_temp_new_i64();
32249     if (qemu_tmp_0 >= 64) {
>>>     CID 1512508:  Control flow issues  (DEADCODE)
>>>     Execution cannot reach this statement: "tcg_gen_movi_i64(tmp_3, 0L);".
32250     tcg_gen_movi_i64(tmp_3, 0);
32251     } else {
32252     tcg_gen_shri_i64(tmp_3, prod, qemu_tmp_0);
32253     }
32254     int64_t qemu_tmp_1 = (int64_t)((int32_t) 0x1);
32255     TCGv_i64 tmp_4 = tcg_temp_new_i64();


________________________________________________________________________________________________________
To view the defects in Coverity Scan visit, https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P0qcxCbhZ31OYv50yrzEQNXe51mg-2FlKoEnRoarMq5nOxxfhqLUuo8HvG2S4Ew-3D-3DbhwO_HM6v81mUGiNDJakbNe6YIRym-2FQHdIITfL-2F6M7vogPEFZsLU1jTbzTaCeyjviVsFQbBtqkUAi0j5NgYn6TunnxkQOIazJbAGHPpUUiVWiLdepITfddm-2BZj5rEvsNpXIGxoCBaqL46nJjmZh0KtMVH7OF2PUhlwJEAF8mxruXgXKxWiIhfYkC5C9ZGRS-2Bb899ClIwrdzrG-2Bcyej9gVEhhbTg-3D-3D

  To manage Coverity Scan email notifications for "anjo@rev.ng", click https://u15810271.ct.sendgrid.net/ls/click?upn=HRESupC-2F2Czv4BOaCWWCy7my0P0qcxCbhZ31OYv50yped04pjJnmXOsUBtKYNIXxkHoxhtWuBL8xoZ3fXkWsjZDnFzdblVNdGVG7hTtMZDUMu9N-2FYFh-2BJcj3muNZzOh-2Bw69-2BzSi3-2Bq8J92Hkdofe5qo1YUQPHgoatMDbyIAxhNk-3DSIxP_HM6v81mUGiNDJakbNe6YIRym-2FQHdIITfL-2F6M7vogPEFZsLU1jTbzTaCeyjviVsFQjadlt-2B2lzdXKw3TRUawXJePqEdv2tDdi9YF5DmvrNW-2BzIqvOihj37P4PnQkxzqLli3gtnfjMNcu8OdlowwiquMdWPu63YHLdJ3vzUw8b00nEIzPsYQD9PMFGPPitGYc6d-2B71ETjA-2FyDsAw1RS5JQhw-3D-3D

Re: Help finding Coverity defects for generated Hexagon code

[Peter Maydell]

On Mon, 22 May 2023 at 21:24, Anton Johansson <anjo@rev.ng> wrote:
>
> Hi,
>
> coverity recently reported some defects in code generated by idef-parser
> (email attached). These defects are expected and we plan to emit a
> /* coverity[event_tag] */ comment to disable the specific event triggered.

We don't mark coverity false positives with comments in the
source. For the free online scanner, we just mark them as
false positives in the GUI (with an explanation of why they're
false positives).

It's weird that the ones listed in the email don't show up
in the GUI, but if they're not in the GUI as unresolved issues
then they're not a problem :-)

thanks
-- PMM

Re: Help finding Coverity defects for generated Hexagon code

[Paolo Bonzini]

On Tue, May 23, 2023 at 11:18 AM Peter Maydell <peter.maydell@linaro.org> wrote:
> On Mon, 22 May 2023 at 21:24, Anton Johansson <anjo@rev.ng> wrote:
> > Hi,
> >
> > coverity recently reported some defects in code generated by idef-parser
> > (email attached). These defects are expected and we plan to emit a
> > /* coverity[event_tag] */ comment to disable the specific event triggered.
>
> We don't mark coverity false positives with comments in the
> source. For the free online scanner, we just mark them as
> false positives in the GUI (with an explanation of why they're
> false positives).

They aren't visible in the GUI because the whole "hexagon generated
files" component is marked as not-analyzed; which apparently means it
_is_ analyzed and visible in the emails but not in the GUI.

The event tag for this error should be "dead_error_condition". In
theory, the hexagon generated files could be a good exception to the
rules that we don't mark false positives in the source, but finding
the right line to add the tag can be messy.

Paolo

Re: Help finding Coverity defects for generated Hexagon code

[Anton Johansson via]

On 5/23/23 12:29, Paolo Bonzini wrote:
> On Tue, May 23, 2023 at 11:18 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>> On Mon, 22 May 2023 at 21:24, Anton Johansson <anjo@rev.ng> wrote:
>>> Hi,
>>>
>>> coverity recently reported some defects in code generated by idef-parser
>>> (email attached). These defects are expected and we plan to emit a
>>> /* coverity[event_tag] */ comment to disable the specific event triggered.
>> We don't mark coverity false positives with comments in the
>> source. For the free online scanner, we just mark them as
>> false positives in the GUI (with an explanation of why they're
>> false positives).
> They aren't visible in the GUI because the whole "hexagon generated
> files" component is marked as not-analyzed; which apparently means it
> _is_ analyzed and visible in the emails but not in the GUI.

Ah right...

> The event tag for this error should be "dead_error_condition". In
> theory, the hexagon generated files could be a good exception to the
> rules that we don't mark false positives in the source, but finding
> the right line to add the tag can be messy.
If we decide to mark these in source, my plan was to simply emit

     if (qemu_tmp_2 >= 64) {
         /* coverity[dead_error_condition] */
         tcg_gen_movi_i64(tmp_5, 0);
     } else {
         tcg_gen_shli_i64(tmp_5, tmp_4, qemu_tmp_2);
     }

for all of these safety checks around shifts/extracts where the defect could
trigger. Maybe this is overreaching as we would also mark similar 
branches in
other instructions that are alive, but if we knew they were dead at 
translation
time we could simply not emit them to begin with.

-- 
Anton Johansson,
rev.ng Labs Srl.

Re: Help finding Coverity defects for generated Hexagon code

[Richard Henderson]

On 5/23/23 06:29, Anton Johansson via wrote:
> 
> On 5/23/23 12:29, Paolo Bonzini wrote:
>> On Tue, May 23, 2023 at 11:18 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On Mon, 22 May 2023 at 21:24, Anton Johansson <anjo@rev.ng> wrote:
>>>> Hi,
>>>>
>>>> coverity recently reported some defects in code generated by idef-parser
>>>> (email attached). These defects are expected and we plan to emit a
>>>> /* coverity[event_tag] */ comment to disable the specific event triggered.
>>> We don't mark coverity false positives with comments in the
>>> source. For the free online scanner, we just mark them as
>>> false positives in the GUI (with an explanation of why they're
>>> false positives).
>> They aren't visible in the GUI because the whole "hexagon generated
>> files" component is marked as not-analyzed; which apparently means it
>> _is_ analyzed and visible in the emails but not in the GUI.
> 
> Ah right...
> 
>> The event tag for this error should be "dead_error_condition". In
>> theory, the hexagon generated files could be a good exception to the
>> rules that we don't mark false positives in the source, but finding
>> the right line to add the tag can be messy.
> If we decide to mark these in source, my plan was to simply emit
> 
>      if (qemu_tmp_2 >= 64) {
>          /* coverity[dead_error_condition] */
>          tcg_gen_movi_i64(tmp_5, 0);
>      } else {
>          tcg_gen_shli_i64(tmp_5, tmp_4, qemu_tmp_2);
>      }
> 
> for all of these safety checks around shifts/extracts where the defect could
> trigger. Maybe this is overreaching as we would also mark similar branches in
> other instructions that are alive, but if we knew they were dead at translation
> time we could simply not emit them to begin with.

It would be simpler to do better constant propagation and folding in the generator than to 
do the markup.  All of the cases for which it warns are really quite trivial.


r~

RE: Help finding Coverity defects for generated Hexagon code

[Brian Cain]

> -----Original Message-----
> From: qemu-devel-bounces+bcain=quicinc.com@nongnu.org <qemu-devel-
> bounces+bcain=quicinc.com@nongnu.org> On Behalf Of Richard Henderson
> Sent: Tuesday, May 23, 2023 10:32 AM
> To: anjo@rev.ng; Paolo Bonzini <pbonzini@redhat.com>; Peter Maydell
> <peter.maydell@linaro.org>
> Cc: qemu-devel@nongnu.org
> Subject: Re: Help finding Coverity defects for generated Hexagon code
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary of
> any links or attachments, and do not enable macros.
> 
> On 5/23/23 06:29, Anton Johansson via wrote:
> >
> > On 5/23/23 12:29, Paolo Bonzini wrote:
> >> On Tue, May 23, 2023 at 11:18 AM Peter Maydell
> <peter.maydell@linaro.org> wrote:
> >>> On Mon, 22 May 2023 at 21:24, Anton Johansson <anjo@rev.ng> wrote:
> >>>> Hi,
> >>>>
> >>>> coverity recently reported some defects in code generated by idef-parser
> >>>> (email attached). These defects are expected and we plan to emit a
> >>>> /* coverity[event_tag] */ comment to disable the specific event triggered.
> >>> We don't mark coverity false positives with comments in the
> >>> source. For the free online scanner, we just mark them as
> >>> false positives in the GUI (with an explanation of why they're
> >>> false positives).
> >> They aren't visible in the GUI because the whole "hexagon generated
> >> files" component is marked as not-analyzed; which apparently means it
> >> _is_ analyzed and visible in the emails but not in the GUI.
> >
> > Ah right...
> >
> >> The event tag for this error should be "dead_error_condition". In
> >> theory, the hexagon generated files could be a good exception to the
> >> rules that we don't mark false positives in the source, but finding
> >> the right line to add the tag can be messy.
> > If we decide to mark these in source, my plan was to simply emit
> >
> >      if (qemu_tmp_2 >= 64) {
> >          /* coverity[dead_error_condition] */
> >          tcg_gen_movi_i64(tmp_5, 0);
> >      } else {
> >          tcg_gen_shli_i64(tmp_5, tmp_4, qemu_tmp_2);
> >      }
> >
> > for all of these safety checks around shifts/extracts where the defect could
> > trigger. Maybe this is overreaching as we would also mark similar branches in
> > other instructions that are alive, but if we knew they were dead at translation
> > time we could simply not emit them to begin with.
> 
> It would be simpler to do better constant propagation and folding in the
> generator than to
> do the markup.  All of the cases for which it warns are really quite trivial.

But the host compiler can already do this for us.  Is the markup really more work than almost anything else?

Re: Help finding Coverity defects for generated Hexagon code

[Anton Johansson via]

On 5/23/23 19:56, Brian Cain wrote:
>
>> -----Original Message-----
>> From: qemu-devel-bounces+bcain=quicinc.com@nongnu.org <qemu-devel-
>> bounces+bcain=quicinc.com@nongnu.org> On Behalf Of Richard Henderson
>> Sent: Tuesday, May 23, 2023 10:32 AM
>> To: anjo@rev.ng; Paolo Bonzini <pbonzini@redhat.com>; Peter Maydell
>> <peter.maydell@linaro.org>
>> Cc: qemu-devel@nongnu.org
>> Subject: Re: Help finding Coverity defects for generated Hexagon code
>>
>> WARNING: This email originated from outside of Qualcomm. Please be wary of
>> any links or attachments, and do not enable macros.
>>
>> On 5/23/23 06:29, Anton Johansson via wrote:
>>> On 5/23/23 12:29, Paolo Bonzini wrote:
>>>> On Tue, May 23, 2023 at 11:18 AM Peter Maydell
>> <peter.maydell@linaro.org> wrote:
>>>>> On Mon, 22 May 2023 at 21:24, Anton Johansson <anjo@rev.ng> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> coverity recently reported some defects in code generated by idef-parser
>>>>>> (email attached). These defects are expected and we plan to emit a
>>>>>> /* coverity[event_tag] */ comment to disable the specific event triggered.
>>>>> We don't mark coverity false positives with comments in the
>>>>> source. For the free online scanner, we just mark them as
>>>>> false positives in the GUI (with an explanation of why they're
>>>>> false positives).
>>>> They aren't visible in the GUI because the whole "hexagon generated
>>>> files" component is marked as not-analyzed; which apparently means it
>>>> _is_ analyzed and visible in the emails but not in the GUI.
>>> Ah right...
>>>
>>>> The event tag for this error should be "dead_error_condition". In
>>>> theory, the hexagon generated files could be a good exception to the
>>>> rules that we don't mark false positives in the source, but finding
>>>> the right line to add the tag can be messy.
>>> If we decide to mark these in source, my plan was to simply emit
>>>
>>>       if (qemu_tmp_2 >= 64) {
>>>           /* coverity[dead_error_condition] */
>>>           tcg_gen_movi_i64(tmp_5, 0);
>>>       } else {
>>>           tcg_gen_shli_i64(tmp_5, tmp_4, qemu_tmp_2);
>>>       }
>>>
>>> for all of these safety checks around shifts/extracts where the defect could
>>> trigger. Maybe this is overreaching as we would also mark similar branches in
>>> other instructions that are alive, but if we knew they were dead at translation
>>> time we could simply not emit them to begin with.
>> It would be simpler to do better constant propagation and folding in the
>> generator than to
>> do the markup.  All of the cases for which it warns are really quite trivial.
> But the host compiler can already do this for us.  Is the markup really more work than almost anything else?
To add to this:

We did look into dealing with these coverity warnings through constant
propagating a few weeks back, and yes it's not that bad.  You really
only have to deal with sign-/zeroextensions and then perform the check
at translation time instead of emitting it.

However, constant propagating only to deal with coverity warnings feels
like the wrong way to go about it.  If we want to go in this direction
we should really propagate/fold as much as possible, which requires
relaxing some assumptions on immediates (always positve and
representable in uint64_t).  Even if this is not hard to do, it
increases the complexity of the parser for something the compiler
already does for us.

For the time being, I'll submit a patch emitting a comment, and if we
decide it's worthwhile to constant fold we'll drop the comments then.

-- 
Anton Johansson,
rev.ng Labs Srl.