rc4-mmotm1110 - Another build error

rc4-mmotm1110 - Another build error

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 504 bytes --]

gcc --version says:
gcc (GCC) 4.3.2 20081105 (Red Hat 4.3.2-7)

ka-blam number 2:

  CC      kernel/audit.o
In file included from include/net/dst.h:15,
                 from include/net/sock.h:57,
                 from kernel/audit.c:54:
include/net/neighbour.h:114: error: braced-group within expression allowed only inside a function
make[1]: *** [kernel/audit.o] Error 1

I'm placing bets on patches/align-avoid-evaluating-its-argument-twice.patch

Yep, revert that patch, and audit.o compiles again.

[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: rc4-mmotm1110 - Another build error

[Andrew Morton]

On Mon, 10 Nov 2008 22:20:33 -0500 Valdis.Kletnieks@vt.edu wrote:

> gcc --version says:
> gcc (GCC) 4.3.2 20081105 (Red Hat 4.3.2-7)
> 
> ka-blam number 2:
> 
>   CC      kernel/audit.o
> In file included from include/net/dst.h:15,
>                  from include/net/sock.h:57,
>                  from kernel/audit.c:54:
> include/net/neighbour.h:114: error: braced-group within expression allowed only inside a function
> make[1]: *** [kernel/audit.o] Error 1
> 
> I'm placing bets on patches/align-avoid-evaluating-its-argument-twice.patch
> 
> Yep, revert that patch, and audit.o compiles again.
> 

I hadn't got around to testing that one yet.

So ug.  ALIGN() is used in array sizing and hence has to be a
compile-time thing.  But ALIGN(foo, bar()) will call bar() twice.

Now how do we fix that?

Re: rc4-mmotm1110 - Another build error

[KAMEZAWA Hiroyuki]

On Mon, 10 Nov 2008 19:43:53 -0800
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Mon, 10 Nov 2008 22:20:33 -0500 Valdis.Kletnieks@vt.edu wrote:
> 
> > gcc --version says:
> > gcc (GCC) 4.3.2 20081105 (Red Hat 4.3.2-7)
> > 
> > ka-blam number 2:
> > 
> >   CC      kernel/audit.o
> > In file included from include/net/dst.h:15,
> >                  from include/net/sock.h:57,
> >                  from kernel/audit.c:54:
> > include/net/neighbour.h:114: error: braced-group within expression allowed only inside a function
> > make[1]: *** [kernel/audit.o] Error 1
> > 
> > I'm placing bets on patches/align-avoid-evaluating-its-argument-twice.patch
> > 
> > Yep, revert that patch, and audit.o compiles again.
> > 
> 
> I hadn't got around to testing that one yet.
> 
> So ug.  ALIGN() is used in array sizing and hence has to be a
> compile-time thing.  But ALIGN(foo, bar()) will call bar() twice.
> 
> Now how do we fix that?

How about provoding
  - ALIGN()  - args can be evaluated twice.
 and
  - static inline unsigned long align()

messy ?

Thanks,
-Kame

Re: rc4-mmotm1110 - Another build error

[Andrew Morton]

On Tue, 11 Nov 2008 15:54:38 +0900 KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

> On Mon, 10 Nov 2008 19:43:53 -0800
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > On Mon, 10 Nov 2008 22:20:33 -0500 Valdis.Kletnieks@vt.edu wrote:
> > 
> > > gcc --version says:
> > > gcc (GCC) 4.3.2 20081105 (Red Hat 4.3.2-7)
> > > 
> > > ka-blam number 2:
> > > 
> > >   CC      kernel/audit.o
> > > In file included from include/net/dst.h:15,
> > >                  from include/net/sock.h:57,
> > >                  from kernel/audit.c:54:
> > > include/net/neighbour.h:114: error: braced-group within expression allowed only inside a function
> > > make[1]: *** [kernel/audit.o] Error 1
> > > 
> > > I'm placing bets on patches/align-avoid-evaluating-its-argument-twice.patch
> > > 
> > > Yep, revert that patch, and audit.o compiles again.
> > > 
> > 
> > I hadn't got around to testing that one yet.
> > 
> > So ug.  ALIGN() is used in array sizing and hence has to be a
> > compile-time thing.  But ALIGN(foo, bar()) will call bar() twice.
> > 
> > Now how do we fix that?
> 
> How about provoding
>   - ALIGN()  - args can be evaluated twice.

Well.  The problem here is the risk of mistakes, and such an ALIGN()
could still be called by mistake.

>  and
>   - static inline unsigned long align()

ALIGN() is designed so that it can take any scalar types as arguments
and still do the right thing, so it can't really be implemented as an
inline.

> messy ?

I suppose we could switch all those ALIGN calls which must occur at
compile-time over to a new STATIC_ALIGN() (?) which is allowed to
evaluate its args twice, then fix ALIGN().

A bit of a pain, but any missed conversions would reliably fail to
compile and would get fixed quickly.

hm, ALIGN() is used in assembly too, but it means something totally
different, sigh.

<greps>

./arch/x86/mm/hugetlbpage.c:    addr = ALIGN(start_addr, huge_page_size(h));
./arch/x86/mm/hugetlbpage.c:            addr = ALIGN(vma->vm_end, huge_page_size(h));
./arch/x86/mm/hugetlbpage.c:            addr = ALIGN(addr, huge_page_size(h));

These will presently be calling huge_page_size() twice.

Will these:

./drivers/mtd/ubi/build.c:	ubi->ec_hdr_alsize = ALIGN(UBI_EC_HDR_SIZE, ubi->hdrs_min_io_size);
./drivers/mtd/ubi/build.c:	ubi->vid_hdr_alsize = ALIGN(UBI_VID_HDR_SIZE, ubi->hdrs_min_io_size);

do the dereference twice?  Probably not.

I didn't immediately spot any bugs, but I didn't look very closely. 
Hopefully if there are any there, we'd have heard about it.

Re: rc4-mmotm1110 - Another build error

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1460 bytes --]

On Mon, 10 Nov 2008 19:43:53 PST, Andrew Morton said:
> On Mon, 10 Nov 2008 22:20:33 -0500 Valdis.Kletnieks@vt.edu wrote:
> 
> > gcc --version says:
> > gcc (GCC) 4.3.2 20081105 (Red Hat 4.3.2-7)
> > 
> > ka-blam number 2:
> > 
> >   CC      kernel/audit.o
> > In file included from include/net/dst.h:15,
> >                  from include/net/sock.h:57,
> >                  from kernel/audit.c:54:
> > include/net/neighbour.h:114: error: braced-group within expression allowed 
only inside a function
> > make[1]: *** [kernel/audit.o] Error 1
> > 
> > I'm placing bets on patches/align-avoid-evaluating-its-argument-twice.patch
> > 
> > Yep, revert that patch, and audit.o compiles again.
> > 
> 
> I hadn't got around to testing that one yet.
> 
> So ug.  ALIGN() is used in array sizing and hence has to be a
> compile-time thing.  But ALIGN(foo, bar()) will call bar() twice.
> 
> Now how do we fix that?

Can we abuse __builtin_constant_p somehow?  Maybe we can make a definition
something like:

#define __align_mask(x,mask) (
	__builtin_constant_p(mask) ? {
		(((x)+(mask))&~(mask)) :
		{              \
		      typeof(mask) __mask = mask;             \
		       (((x) + __mask) & ~__mask);             \
		}
	})

If it's a compile-time constant, it's safe to evaluate mask twice.
And if it's *not* a constant, it shouldn't be used for sizing arrays
in open code.  And if it's an array local to a procedure, it probably
should be kmalloc()'ed instead.


[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: rc4-mmotm1110 - Another build error

[Andrew Morton]

On Tue, 11 Nov 2008 14:04:57 -0500
Valdis.Kletnieks@vt.edu wrote:

> On Mon, 10 Nov 2008 19:43:53 PST, Andrew Morton said:
> > On Mon, 10 Nov 2008 22:20:33 -0500 Valdis.Kletnieks@vt.edu wrote:
> > 
> > > gcc --version says:
> > > gcc (GCC) 4.3.2 20081105 (Red Hat 4.3.2-7)
> > > 
> > > ka-blam number 2:
> > > 
> > >   CC      kernel/audit.o
> > > In file included from include/net/dst.h:15,
> > >                  from include/net/sock.h:57,
> > >                  from kernel/audit.c:54:
> > > include/net/neighbour.h:114: error: braced-group within expression allowed 
> only inside a function
> > > make[1]: *** [kernel/audit.o] Error 1
> > > 
> > > I'm placing bets on patches/align-avoid-evaluating-its-argument-twice.patch
> > > 
> > > Yep, revert that patch, and audit.o compiles again.
> > > 
> > 
> > I hadn't got around to testing that one yet.
> > 
> > So ug.  ALIGN() is used in array sizing and hence has to be a
> > compile-time thing.  But ALIGN(foo, bar()) will call bar() twice.
> > 
> > Now how do we fix that?
> 
> Can we abuse __builtin_constant_p somehow?  Maybe we can make a definition
> something like:
> 
> #define __align_mask(x,mask) (
> 	__builtin_constant_p(mask) ? {
> 		(((x)+(mask))&~(mask)) :
> 		{              \
> 		      typeof(mask) __mask = mask;             \
> 		       (((x) + __mask) & ~__mask);             \
> 		}
> 	})
> 
> If it's a compile-time constant, it's safe to evaluate mask twice.

This seems to compile:

int y;
char x[__builtin_constant_p(1) ? 42 : y + 1];

so yes, that might work.  For this gcc version.  However I suspect that
it would blow up on us.  We keep on having problems with the
__builtin_constant_p() in kmalloc() doing wrong things (with gcc-3.2.x,
iirc).

It'd be worth trying though.

DIV_ROUND_UP() and roundup() are busted too.