Re: Potential missing checks

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

On Tue, 24 Mar 2020 at 20:39, Mansour Ahmadi <mansourweb@gmail.com> wrote:
>
> Thank you for looking into this, Peter. I agree that static analysis has false positives; that's why I called them potential. Basically, they are found based on code similarity so I might be wrong and I need a second opinion from QEMU developers. I appreciate your effort.

The thing is, you're making us do all the work here. That's
not very useful to us. It's doubly unuseful when there's
a strong chance that when we do do the work of looking
at the code it turns out that there's no problem.

"I did some static analysis, and I looked at the
results, and I dug through the QEMU code, and it
does seem to me that this could well be a bug" is
definitely useful. "I did some static analysis using
only analysis techniques that have an pretty
low false positive rate, and here is a selection of
the results" is also useful. But "I just ran the
code through an analyser that produces lots of
false positives and then I didn't do any further
human examination of the results" is of much less
utility to the project, I'm afraid.

> For the first case, I noticed a check on offset (if (offset)) before negating it and passing to stream function here.
> https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/disas/arm.c#L1748
>
> Similar scenario happened here WITHOUT the check:
> https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/disas/arm.c#L2731-L2733

So, this is in the disassembler. The difference is
just whether we print out a register+offset memory
reference where the offset happens to be zero
as "[reg, #0]" or just "[reg]", and the no-special-case-0
is for encodings which are always pc-relative.
So even if it was a missing check the results are
entirely harmless, since anybody reading the disassembly
will understand the #0 fine.

Secondly, this code is imported from binutils,
so we usually don't worry too much about fixing
up minor bugs in it.

Finally, I went and checked the Arm specs, and for
the kinds of PC-relative load/store that the second
example is handling the specified disassembly format
does mandate the "pc, #0" (whereas the other example
is correctly skipping it for 0-immediates because
in those insns the offset is optional in disassembly).

So the code is correct as it stands.

thanks
-- PMM