[PATCH]AMDHT misbehaves on recent compilers

[Patrick Georgi]

Hi,

src/northbridge/amdht/ht_wrapper.c miscompiles on recent compilers
(gcc-4.4.1 in crossgcc for example). The compiler is correct in what it
does, our code isn't.

The issue is that with recent compilers swaplist is generated on the
stack, and then a pointer to that structure on stack is passed around.
The data is nearly immediately destroyed by subsequent calls.

The "const" modifier only makes the compiler ensure that no write
operations are made to the data, but says nothing about the life cycle.
To force the compiler to keep the array in read-only memory, it must be
global const or static const.

Thanks go to Myles for isolating the problem.

Signed-off-by: Patrick Georgi

[ron minnich]

arg, that kind of error is almost embarassing :-)

Acked-by: Ronald G. Minnich

ron

[Myles Watson]

> arg, that kind of error is almost embarassing :-)

This seems like something that could have easily been caught by static
analysis, but I can't find a compiler warning that would have caught it.

Could scan-build have caught it (I looked and didn't find it in the output)?
Other tools?

Thanks,
Myles

[Stefan Reinauer]

On 1/6/10 10:31 PM, Myles Watson wrote:
> > arg, that kind of error is almost embarassing :-)
>
> This seems like something that could have easily been caught by static
> analysis, but I can't find a compiler warning that would have caught it.
>
> Could scan-build have caught it (I looked and didn't find it in the output)?
> Other tools?

scan-build does not catch it (as of r92410)

Here's a simple test case if someone wants to try other tools:

#include <stdio.h>
const char *func_foo(void)
{
    const char *foo="foobar\n";
    return foo;
}
int main(void)
{
    const char *foo;

foo = func_foo();
    printf("%s\n", foo);
    return 0;
}

sparse says:
test.c:2:12: warning: symbol 'func_foo' was not declared. Should it be
static?
which could indicate it catches the error, not sure.

Stefan

[Kevin O'Connor]

On Thu, Jan 07, 2010 at 02:06:18AM +0100, Stefan Reinauer wrote:
> Here's a simple test case if someone wants to try other tools:
>
> #include <stdio.h>
> const char *func_foo(void)
> {
>     const char *foo="foobar\n";

I don't think the above is a problem (the pointer foo resides on the
stack, but the string contents do not).  The following is a problem:

const char foo[]="foobar\n";

(both the pointer and the array contents are on the stack), and indeed
this does generate a warning from gcc.

Also, had  AMD_CB_ManualBUIDSwapList() returned a  pointer to swaplist
instead  of  returning it  via  a parameter  then  it  too would  have
generated a warning.

-Kevin

[Stefan Reinauer]

On 1/7/10 5:23 AM, Kevin O'Connor wrote:
> I don't think the above is a problem (the pointer foo resides on the
> stack, but the string contents do not).  The following is a problem:
> const char foo[]="foobar\n";
>
> (both the pointer and the array contents are on the stack), and indeed
> this does generate a warning from gcc.

Not with my default 4.3.2  -Wall -Wextra

Stefan

[Kevin O'Connor]

On Thu, Jan 07, 2010 at 10:09:23AM +0100, Stefan Reinauer wrote:
> On 1/7/10 5:23 AM, Kevin O'Connor wrote:
> > I don't think the above is a problem (the pointer foo resides on the
> > stack, but the string contents do not).  The following is a problem:
> > const char foo[]="foobar\n";
> >
> > (both the pointer and the array contents are on the stack), and indeed
> > this does generate a warning from gcc.
>
> Not with my default 4.3.2  -Wall -Wextra

That's odd.  I used:

-----------------------------------------------
#include <stdio.h>
const char *func_foo(void)
{
    const char foo[]="foobar\n";
    return foo;
}
int main(void)
{
    const char *foo;

foo = func_foo();
    printf("%s\n", foo);
    return 0;
}
-----------------------------------------------

$ gcc --version
gcc (GCC) 4.4.2 20091222 (Red Hat 4.4.2-20)
$ gcc -O -Wall constonstack.c 
constonstack.c: In function ‘func_foo’:
constonstack.c:5: warning: function returns address of local variable

$ gcc34 --version
gcc34 (GCC) 3.4.6 20060404 (Red Hat 3.4.6-18)
$ gcc34 -O -Wall constonstack.c 
constonstack.c: In function `func_foo':
constonstack.c:5: warning: function returns address of local variable

-Kevin

[Stefan Reinauer]

On 1/7/10 2:41 PM, Kevin O'Connor wrote:
> On Thu, Jan 07, 2010 at 10:09:23AM +0100, Stefan Reinauer wrote:
> > On 1/7/10 5:23 AM, Kevin O'Connor wrote:
> > > I don't think the above is a problem (the pointer foo resides on the
> > > stack, but the string contents do not).  The following is a problem:
> > > const char foo[]="foobar\n";
> > >
> > > (both the pointer and the array contents are on the stack), and indeed
> > > this does generate a warning from gcc.
> >
> > Not with my default 4.3.2  -Wall -Wextra
>
> That's odd.  I used:

..

Hm.. seems I was wrong. I verified and I get the same message. Why
didn't the code in coreboot trigger one?

Stefan

[Myles Watson]

> Hm.. seems I was wrong. I verified and I get the same message. Why
> didn't the code in coreboot trigger one?

Kevin's take:
> > Also, had  AMD_CB_ManualBUIDSwapList() returned a  pointer to swaplist
> > instead  of  returning it  via  a parameter  then  it  too would  have
> > generated a warning.
> > -Kevin

Thanks,
Myles