Thanks for the explanation.
On Tue, Mar 24, 2020 at 5:17 PM Peter Maydell <peter.mayd...@linaro.org> wrote: > On Tue, 24 Mar 2020 at 20:39, Mansour Ahmadi <mansour...@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 >
