[PATCH] Fix an overflow case in fdt_offset_ptr() detected by GCC 4.3.

[PATCH] Fix an overflow case in fdt_offset_ptr() detected by GCC 4.3.

[Jon Loeliger]

Using Gcc 4.3 detected this problem:

    ../dtc/libfdt/fdt.c: In function 'fdt_next_tag':
    ../dtc/libfdt/fdt.c:82: error: assuming signed overflow does not
    occur when assuming that (X + c) < X is always false

To fix the problem, treat the offset as an unsigned int.

The problem report and proposed fix were provided
by Steve Papacharalambous <stevep-KZfg59tc24xl57MIdRCFDg@public.gmane.org>.

Signed-off-by: Jon Loeliger <jdl-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 libfdt/fdt.c    |    2 +-
 libfdt/libfdt.h |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/libfdt/fdt.c b/libfdt/fdt.c
index 2acaec5..37085c1 100644
--- a/libfdt/fdt.c
+++ b/libfdt/fdt.c
@@ -74,7 +74,7 @@ int fdt_check_header(const void *fdt)
 	return 0;
 }
 
-const void *fdt_offset_ptr(const void *fdt, int offset, int len)
+const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
 {
 	const char *p;
 
diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index 9e4f990..60dd4a1 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -122,7 +122,7 @@
 /* Low-level functions (you probably don't need these)                */
 /**********************************************************************/
 
-const void *fdt_offset_ptr(const void *fdt, int offset, int checklen);
+const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int checklen);
 static inline void *fdt_offset_ptr_w(void *fdt, int offset, int checklen)
 {
 	return (void *)(uintptr_t)fdt_offset_ptr(fdt, offset, checklen);
-- 
1.6.0.2.g2ebc0

Re: [PATCH] Fix an overflow case in fdt_offset_ptr() detected by GCC 4.3.

[David Gibson]

On Thu, Sep 25, 2008 at 11:02:17AM -0500, Jon Loeliger wrote:
> 
> Using Gcc 4.3 detected this problem:
> 
>     ../dtc/libfdt/fdt.c: In function 'fdt_next_tag':
>     ../dtc/libfdt/fdt.c:82: error: assuming signed overflow does not
>     occur when assuming that (X + c) < X is always false

Um... I'm a little baffled by this warning, given that the line in
question is actually testing that (X+c) < X is false..

> To fix the problem, treat the offset as an unsigned int.

Nor am I entirely sure how that's supposed to help, since an unsigned
overflow is also possible.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH] Fix an overflow case in fdt_offset_ptr() detected by GCC 4.3.

[Jon Loeliger]

> 
> Using Gcc 4.3 detected this problem:
> 
>     ../dtc/libfdt/fdt.c: In function 'fdt_next_tag':
>     ../dtc/libfdt/fdt.c:82: error: assuming signed overflow does not
>     occur when assuming that (X + c) < X is always false
> 
> To fix the problem, treat the offset as an unsigned int.
> 
> The problem report and proposed fix were provided
> by Steve Papacharalambous <stevep-KZfg59tc24xl57MIdRCFDg@public.gmane.org>.
> 
> Signed-off-by: Jon Loeliger <jdl-KZfg59tc24xl57MIdRCFDg@public.gmane.org>

Applied.

jdl

Re: [PATCH] Fix an overflow case in fdt_offset_ptr() detected by GCC 4.3.

[David Gibson]

On Mon, Sep 29, 2008 at 04:13:27PM -0500, Jon Loeliger wrote:
> > 
> > Using Gcc 4.3 detected this problem:
> > 
> >     ../dtc/libfdt/fdt.c: In function 'fdt_next_tag':
> >     ../dtc/libfdt/fdt.c:82: error: assuming signed overflow does not
> >     occur when assuming that (X + c) < X is always false
> > 
> > To fix the problem, treat the offset as an unsigned int.
> > 
> > The problem report and proposed fix were provided
> > by Steve Papacharalambous <stevep-KZfg59tc24xl57MIdRCFDg@public.gmane.org>.
> > 
> > Signed-off-by: Jon Loeliger <jdl-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> 
> Applied.

Uh.. Jon.. did you see my reply to this.  I'm not at all convinced
this patches a real problem.  I suspect it's just replacing a problem
that gcc could detect with a similar one that gcc can't (and for which
we already had a test to deal with, anyway).

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH] Fix an overflow case in fdt_offset_ptr() detected by GCC 4.3.

[Jon Loeliger]

> On Mon, Sep 29, 2008 at 04:13:27PM -0500, Jon Loeliger wrote:
> > > 
> 
> Uh.. Jon.. did you see my reply to this.  I'm not at all convinced
> this patches a real problem.  I suspect it's just replacing a problem
> that gcc could detect with a similar one that gcc can't (and for which
> we already had a test to deal with, anyway).

I saw your reply, and you are welcome to read up
on the fist fight over in GCC land too.

In the meantime, it fixes a real problem that we
have at Freescale.

Steve, can you remind us the URL of the GCC discussion?

Thanks,
jdl

Re: [PATCH] Fix an overflow case in fdt_offset_ptr() detected by GCC 4.3.

[David Gibson]

On Mon, Sep 29, 2008 at 08:51:02PM -0500, Jon Loeliger wrote:
> > On Mon, Sep 29, 2008 at 04:13:27PM -0500, Jon Loeliger wrote:
> > > > 
> > 
> > Uh.. Jon.. did you see my reply to this.  I'm not at all convinced
> > this patches a real problem.  I suspect it's just replacing a problem
> > that gcc could detect with a similar one that gcc can't (and for which
> > we already had a test to deal with, anyway).
> 
> I saw your reply, and you are welcome to read up
> on the fist fight over in GCC land too.
> 
> In the meantime, it fixes a real problem that we
> have at Freescale.

Ok.. and can you explain this real problem?

If this is just to work around a silly gcc warning that appears in
some circumstances and not others, that's fine, but the commit message
should say so.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH] Fix an overflow case in fdt_offset_ptr() detected by GCC 4.3.

[Steve Papacharalambous]

On Tue, 2008-09-30 at 12:39 +1000, David Gibson wrote:
> On Mon, Sep 29, 2008 at 08:51:02PM -0500, Jon Loeliger wrote:
> > > On Mon, Sep 29, 2008 at 04:13:27PM -0500, Jon Loeliger wrote:
> > > > > 
> > > 
> > > Uh.. Jon.. did you see my reply to this.  I'm not at all convinced
> > > this patches a real problem.  I suspect it's just replacing a problem
> > > that gcc could detect with a similar one that gcc can't (and for which
> > > we already had a test to deal with, anyway).
> > 
> > I saw your reply, and you are welcome to read up
> > on the fist fight over in GCC land too.
> > 
> > In the meantime, it fixes a real problem that we
> > have at Freescale.
> 
> Ok.. and can you explain this real problem?
> 
> If this is just to work around a silly gcc warning that appears in
> some circumstances and not others, that's fine, but the commit message
> should say so.
> 

Hi David,

The error that is generated when using gcc-4.3.2 20080819 is:

[snip]
/opt/freescale/usr/local/gcc-4.3.8-eglibc-2.8.8/powerpc-linux-gnu/bin/powerpc-linux-gnu-gcc -include config/autoconf.h -m32 -Wa,-me500 -nostdinc -I /opt/freescale/usr/local/gcc-4.3.8-eglibc-2.8.8/powerpc-linux-gnu/lib/gcc/powerpc-linux-gnu/4.3.2/include -I /opt/freescale/usr/local/gcc-4.3.8-eglibc-2.8.8/powerpc-linux-gnu/lib/gcc/powerpc-linux-gnu/4.3.2/include-fixed -Iinclude -Ibin/include -I../dtc/libfdt -I../libos/include -g -std=gnu99  -include include/libos-client.h -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -O2 -msoft-float -pipe -ffixed-r2 -mmultiple -mno-altivec -funit-at-a-time -mno-string -fomit-frame-pointer -Werror -include include/libfdt_env.h -c -o bin/libfdt/fdt.o ../dtc/libfdt/fdt.c
cc1: warnings being treated as errors
../dtc/libfdt/fdt.c: In function 'fdt_next_tag':
../dtc/libfdt/fdt.c:82: error: assuming signed overflow does not occur
when assuming that (X + c) < X is always false
../dtc/libfdt/fdt.c:82: error: assuming signed overflow does not occur
when assuming that (X + c) < X is always false
../dtc/libfdt/fdt.c:82: error: assuming signed overflow does not occur
when assuming that (X + c) < X is always false
[/snip]

which does not occur with earlier versions of gcc.  For example the same
command builds without errors with gcc-4.2.3.

A good starting point for the discussion on this subject is:
http://gcc.gnu.org/ml/gcc/2008-04/msg00618.html

I suspect that you're right and this error doesn't occur in all
circumstances, so perhaps the commit message should be changed,

Best regards,

Steve

Re: [PATCH] Fix an overflow case in fdt_offset_ptr() detected by GCC 4.3.

[Scott Wood]

David Gibson wrote:
> On Mon, Sep 29, 2008 at 08:51:02PM -0500, Jon Loeliger wrote:
>>> On Mon, Sep 29, 2008 at 04:13:27PM -0500, Jon Loeliger wrote:
>>> Uh.. Jon.. did you see my reply to this.  I'm not at all convinced
>>> this patches a real problem.  I suspect it's just replacing a problem
>>> that gcc could detect with a similar one that gcc can't (and for which
>>> we already had a test to deal with, anyway).
>> I saw your reply, and you are welcome to read up
>> on the fist fight over in GCC land too.
>>
>> In the meantime, it fixes a real problem that we
>> have at Freescale.
> 
> Ok.. and can you explain this real problem?
> 
> If this is just to work around a silly gcc warning that appears in
> some circumstances and not others, that's fine, but the commit message
> should say so.

It seems that the C standard says that signed overflow is undefined, but 
not unsigned overflow.  Presumably that was to accomodate 
non-twos-complement machines (which we don't really care about), but GCC 
abuses it to completely optimize away any paths that depend on such 
overflow, and the fact that they had to add a warning about it didn't 
strike them as a reason why they should maybe not go ahead with such an 
optimization by default, even if it's permitted by the spec.

-Scott

Re: [PATCH] Fix an overflow case in fdt_offset_ptr() detected by GCC 4.3.

[David Gibson]

On Tue, Sep 30, 2008 at 11:28:18AM -0500, Scott Wood wrote:
> David Gibson wrote:
>> On Mon, Sep 29, 2008 at 08:51:02PM -0500, Jon Loeliger wrote:
>>>> On Mon, Sep 29, 2008 at 04:13:27PM -0500, Jon Loeliger wrote:
>>>> Uh.. Jon.. did you see my reply to this.  I'm not at all convinced
>>>> this patches a real problem.  I suspect it's just replacing a problem
>>>> that gcc could detect with a similar one that gcc can't (and for which
>>>> we already had a test to deal with, anyway).
>>> I saw your reply, and you are welcome to read up
>>> on the fist fight over in GCC land too.
>>>
>>> In the meantime, it fixes a real problem that we
>>> have at Freescale.
>>
>> Ok.. and can you explain this real problem?
>>
>> If this is just to work around a silly gcc warning that appears in
>> some circumstances and not others, that's fine, but the commit message
>> should say so.
>
> It seems that the C standard says that signed overflow is undefined, but  
> not unsigned overflow.  Presumably that was to accomodate  
> non-twos-complement machines (which we don't really care about), but GCC  
> abuses it to completely optimize away any paths that depend on such  
> overflow, and the fact that they had to add a warning about it didn't  
> strike them as a reason why they should maybe not go ahead with such an  
> optimization by default, even if it's permitted by the spec.

Ah, ok!  The old C undefined behaviour problem.  Ok, now this patch
makes sense - we were indeed assuming that overflow behaved as you'd
expect for a two's complement machine

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson