Re: Help finding Coverity defects for generated Hexagon code
On 5/23/23 19:56, Brian Cain wrote:
-Original Message-
From: qemu-devel-bounces+bcain=quicinc@nongnu.org On Behalf Of Richard Henderson
Sent: Tuesday, May 23, 2023 10:32 AM
To: a...@rev.ng; Paolo Bonzini ; Peter Maydell
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
wrote:
On Mon, 22 May 2023 at 21:24, Anton Johansson 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.
RE: Help finding Coverity defects for generated Hexagon code
> -Original Message-
> From: qemu-devel-bounces+bcain=quicinc@nongnu.org bounces+bcain=quicinc@nongnu.org> On Behalf Of Richard Henderson
> Sent: Tuesday, May 23, 2023 10:32 AM
> To: a...@rev.ng; Paolo Bonzini ; Peter Maydell
>
> 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
> wrote:
> >>> On Mon, 22 May 2023 at 21:24, Anton Johansson 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
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 wrote:
On Mon, 22 May 2023 at 21:24, Anton Johansson 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
On 5/23/23 12:29, Paolo Bonzini wrote:
On Tue, May 23, 2023 at 11:18 AM Peter Maydell wrote:
On Mon, 22 May 2023 at 21:24, Anton Johansson 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
On Tue, May 23, 2023 at 11:18 AM Peter Maydell wrote: > On Mon, 22 May 2023 at 21:24, Anton Johansson 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
On Mon, 22 May 2023 at 21:24, Anton Johansson 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
