hw/slavio_intctl.c:299: error: array subscript is above array bounds

[Loïc Minier]

Hey

Can't report this against the Ubuntu qemu-linaro source package yet, so filing it here in the mean time:
qemu-linaro RC package prepared by Steve Langasek failed to build in the maverick Linaro tools PPA with:
  CC sparc-softmmu/slavio_intctl.o
cc1: warnings being treated as errors
/build/buildd/qemu-linaro-0.13.50-2011.02-0~rc1+dfsg/hw/slavio_intctl.c: In function 'slavio_check_interrupts':
/build/buildd/qemu-linaro-0.13.50-2011.02-0~rc1+dfsg/hw/slavio_intctl.c:299: error: array subscript is above array bounds
/build/buildd/qemu-linaro-0.13.50-2011.02-0~rc1+dfsg/hw/slavio_intctl.c:295: error: array subscript is above array bounds

http://launchpadlibrarian.net/63066060/buildlog_ubuntu-maverick-armel.qemu-linaro_0.13.50-2011.02-0~rc1%2Bdfsg-0ubuntu1~ppa1_FAILEDTOBUILD.txt.gz

Cheers,

[Loïc Minier (lool)]

typedef struct SLAVIO_INTCTLState {
    SysBusDevice busdev;
    uint32_t intregm_pending;
    uint32_t intregm_disabled;
    uint32_t target_cpu;
#ifdef DEBUG_IRQ_COUNT
    uint64_t irq_count[32];
#endif
    qemu_irq cpu_irqs[MAX_CPUS][MAX_PILS];
    SLAVIO_CPUINTCTLState slaves[MAX_CPUS];
} SLAVIO_INTCTLState;

[...]

            for (j = MAX_PILS; j > 0; j--) {
                if (pil_pending & (1 << j)) {
                    if (!(s->slaves[i].irl_out & (1 << j))) {
                        qemu_irq_raise(s->cpu_irqs[i][j]);
                    }
                } else {
                    if (s->slaves[i].irl_out & (1 << j)) {
                        qemu_irq_lower(s->cpu_irqs[i][j]);
                    }
                }
            }

j starts at MAX_PILS, so [j] is out of array bounds

[Peter Maydell (pmaydell)]

Yes, I saw that this morning; pretty clear off-by-one kind of error. I'll put together a patch on Monday (I imagine upstream will want to put it in 0.14, due to branch Real Soon Now).

What version of gcc on what architecture spotted that?

[Loïc Minier (lool)]

This was in the maverick Linaro tools PPA, so using maverick gcc which is gcc-4.4 4.4.4-14ubuntu5 which includes the Linaro patchset. (You can see the package getting upgraded in the buld log)

[Loïc Minier (lool)]

(on armel)

[Peter Maydell (pmaydell)]

> (on armel)

That prompts me to wonder, did you include the "use the gcc builtins for atomic ops" patch in the packaging? It's not in the git tree at the moment, so unless you did I guess we'll fail the compile later on anyway.

I suppose that patch ought to go into the git tree and we should post it upstream (have we done that before at some point in the past?)

[Loïc Minier (lool)]

We don't have the gcc builtins for atomic ops patch in the packaging either.

i grabbed the qemu-kvm source package to see which other patches we should look at:
- 1000-undo-earlier-static: seems useful for static builds; I think I send a static build fix upstream a while ago; don't think we really need this for now
- 2000-vmmouse-adapt-to-mouse-handler-changes: probably x86 specific
- arm-higher-initrd-load-address: you have that now
- arm-ignore-writes-of-perf-reg-cp15-with-crm-12: I think you have a more complete implementation
- caps-lock-key-up-event: probably useful for the SDL UI, should really go upstream
- Detect-and-use-GCC-atomic-builtins-for-locking: definitely missing in our tree
- larger_default_ram_size: bumps default RAM size, probably only applies to x86 vms, don't care

Yes, the GCC builtins stuff was sent upstream (by me) but two comments were made:
- ppc maintainer didn't want this merged as he said basically didn't trust the gcc builtins and thought it would be slower
- I think Paul Brook made the comment that the code which was being patched could go away entirely

http://<email address hidden>/msg25560.html

I'm happy if you take a fresh look at this! :-)

[Steve Langasek (vorlon)]

I had submitted the fix for the sparc-armel build failure directly to Peter by email; attaching it here as well.

[Steve Langasek (vorlon)]

Whoops, that patch didn't hold up so well under a build test. Here's a better one.

[Peter Maydell (pmaydell)]

Steve, are you happy for me to add a "Signed-off-by: Steve Langasek <email address hidden>" to that patch when I send it upstream?
(Usual kernel rules apply, ie http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/SubmittingPatches;h=689e2371095cc5dfea9927120009341f369159aa;hb=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#l297)

[Peter Maydell (pmaydell)]

> For some reason bit 0 is always 0 in the code so will never have an effect, but that may be a (separate) bug.

It turns out that that's deliberate; the relevant register has bit 0 (and 16) read-as-zero, because it only has 15 levels of interrupt and level 0 means "no interrupts active":

http://www.ibiblio.org/pub/historic-linux/early-ports/Sparc/NCR/NCR89C105.txt

So it might be a better patch to just change the upper bound of the loop, since we will never do anything in the j==0 iteration.

[Peter Maydell (pmaydell)]

...so that's what I ended up submitting upstream:
http://patchwork.ozlabs.org/patch/81091/

[Steve Langasek (vorlon)]

On Mon, Jan 31, 2011 at 09:20:12AM -0000, Peter Maydell wrote:
> Steve, are you happy for me to add a "Signed-off-by: Steve Langasek
> <email address hidden>" to that patch when I send it upstream?
> (Usual kernel rules apply, ie
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/SubmittingPatches;h=689e2371095cc5dfea9927120009341f369159aa;hb=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#l297)

Yes, please. Sorry, bzr-git doesn't have a knob for this afaik and I didn't
realize it was required for qemu.

--
Steve Langasek Give me a lever long enough and a Free OS
Debian Developer to set it on, and I can move the world.
Ubuntu Developer http://www.debian.org/
<email address hidden> <email address hidden>

[Peter Maydell (pmaydell)]

http://patchwork.ozlabs.org/patch/81205/ is my proposal for dealing with the qemu atomic spinlocks using swp, as sent upstream. It basically rips out all the inline asm implementations as unnecessary cruft. Not sure whether it'll get any argument upstream, but I'm going to put it into qemu-linaro anyway because I like it better than the "use gcc builtins" patch. I've also completed the rebase I wanted to do, which just leaves (a) getting rid of the non-redistributable binaries and (b) some retesting before I can spin an RC2.

[Peter Maydell (pmaydell)]

I'm marking this fix-committed as I have now pushed fixes to the qemu-linaro git tree (and submitted them upstream) for:
 * the use of "swp" in qemu-lock.h
 * the compile failure in slavio_intctl.c

Loïc: if you think we should be doing anything with the other patches you mention in comment #6 can you raise separate bugs for them, please?

[Loïc Minier (lool)]

Looking at my list of patches again, I don't think we need to do anything right now; if we're bothered by specific issues, we can borrow the patches