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