PAGE_ALIGN() compile breakage

PAGE_ALIGN() compile breakage

[Adrian Bunk]

Commit 27ac792ca0b0a1e7e65f20342260650516c95864
(PAGE_ALIGN(): correctly handle 64-bit values on 32-bit architectures)
causes on some architectures (e.g. avr32 and mips) compile errors
like the following in some configurations starting with:

<--  snip  -->

...
  CC      init/main.o
In file included from 
/home/bunk/linux/kernel-2.6/git/linux-2.6/include/linux/utsname.h:35,
                 from /home/bunk/linux/kernel-2.6/git/linux-2.6/init/main.c:20:
/home/bunk/linux/kernel-2.6/git/linux-2.6/include/linux/sched.h: In function 'arch_pick_mmap_layout':
/home/bunk/linux/kernel-2.6/git/linux-2.6/include/linux/sched.h:2149: error: implicit declaration of function 'PAGE_ALIGN'
make[2]: *** [init/main.o] Error 1

<--  snip  -->

and more nasty problems follow later.

My suggestion is to:
- revert commit 27ac792ca0b0a1e7e65f20342260650516c95864 and then
- fix all PAGE_ALIGN() instances without moving them.

Unifying code is a good thing, but in this case it is not worth the 
trouble it causes by poking into the heart of our headers mess.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: PAGE_ALIGN() compile breakage

[Andrew Morton]

On Fri, 25 Jul 2008 11:39:43 +0300 Adrian Bunk <bunk@kernel.org> wrote:

> Commit 27ac792ca0b0a1e7e65f20342260650516c95864
> (PAGE_ALIGN(): correctly handle 64-bit values on 32-bit architectures)
> causes on some architectures (e.g. avr32 and mips) compile errors
> like the following in some configurations starting with:
> 
> <--  snip  -->
> 
> ...
>   CC      init/main.o
> In file included from 
> /home/bunk/linux/kernel-2.6/git/linux-2.6/include/linux/utsname.h:35,
>                  from /home/bunk/linux/kernel-2.6/git/linux-2.6/init/main.c:20:
> /home/bunk/linux/kernel-2.6/git/linux-2.6/include/linux/sched.h: In function 'arch_pick_mmap_layout':
> /home/bunk/linux/kernel-2.6/git/linux-2.6/include/linux/sched.h:2149: error: implicit declaration of function 'PAGE_ALIGN'
> make[2]: *** [init/main.o] Error 1
> 

pls test:

diff -puN include/linux/sched.h~a include/linux/sched.h
--- a/include/linux/sched.h~a
+++ a/include/linux/sched.h
@@ -2139,16 +2139,7 @@ static inline void set_task_cpu(struct t
 
 #endif /* CONFIG_SMP */
 
-#ifdef HAVE_ARCH_PICK_MMAP_LAYOUT
 extern void arch_pick_mmap_layout(struct mm_struct *mm);
-#else
-static inline void arch_pick_mmap_layout(struct mm_struct *mm)
-{
-	mm->mmap_base = TASK_UNMAPPED_BASE;
-	mm->get_unmapped_area = arch_get_unmapped_area;
-	mm->unmap_area = arch_unmap_area;
-}
-#endif
 
 #ifdef CONFIG_TRACING
 extern void
diff -puN mm/mmap.c~a mm/mmap.c
--- a/mm/mmap.c~a
+++ a/mm/mmap.c
@@ -2268,3 +2268,12 @@ int install_special_mapping(struct mm_st
 
 	return 0;
 }
+
+#ifndef HAVE_ARCH_PICK_MMAP_LAYOUT
+void arch_pick_mmap_layout(struct mm_struct *mm)
+{
+	mm->mmap_base = TASK_UNMAPPED_BASE;
+	mm->get_unmapped_area = arch_get_unmapped_area;
+	mm->unmap_area = arch_unmap_area;
+}
+#endif
_

> 
> and more nasty problems follow later.
> 
> My suggestion is to:
> - revert commit 27ac792ca0b0a1e7e65f20342260650516c95864 and then
> - fix all PAGE_ALIGN() instances without moving them.

Every time this patch blew up (and it did it often), fixing it resulted
in overall improvements.

> Unifying code is a good thing, but in this case it is not worth the 
> trouble it causes by poking into the heart of our headers mess.

Well.   If we leave it a mess, it'll stay a mess.

Re: PAGE_ALIGN() compile breakage

[Adrian Bunk]

On Fri, Jul 25, 2008 at 01:55:37AM -0700, Andrew Morton wrote:
> On Fri, 25 Jul 2008 11:39:43 +0300 Adrian Bunk <bunk@kernel.org> wrote:
> 
> > Commit 27ac792ca0b0a1e7e65f20342260650516c95864
> > (PAGE_ALIGN(): correctly handle 64-bit values on 32-bit architectures)
> > causes on some architectures (e.g. avr32 and mips) compile errors
> > like the following in some configurations starting with:
> > 
> > <--  snip  -->
> > 
> > ...
> >   CC      init/main.o
> > In file included from 
> > /home/bunk/linux/kernel-2.6/git/linux-2.6/include/linux/utsname.h:35,
> >                  from /home/bunk/linux/kernel-2.6/git/linux-2.6/init/main.c:20:
> > /home/bunk/linux/kernel-2.6/git/linux-2.6/include/linux/sched.h: In function 'arch_pick_mmap_layout':
> > /home/bunk/linux/kernel-2.6/git/linux-2.6/include/linux/sched.h:2149: error: implicit declaration of function 'PAGE_ALIGN'
> > make[2]: *** [init/main.o] Error 1
> > 
> 
> pls test:
> 
> diff -puN include/linux/sched.h~a include/linux/sched.h
> --- a/include/linux/sched.h~a
> +++ a/include/linux/sched.h
> @@ -2139,16 +2139,7 @@ static inline void set_task_cpu(struct t
>  
>  #endif /* CONFIG_SMP */
>  
> -#ifdef HAVE_ARCH_PICK_MMAP_LAYOUT
>  extern void arch_pick_mmap_layout(struct mm_struct *mm);
> -#else
> -static inline void arch_pick_mmap_layout(struct mm_struct *mm)
> -{
> -	mm->mmap_base = TASK_UNMAPPED_BASE;
> -	mm->get_unmapped_area = arch_get_unmapped_area;
> -	mm->unmap_area = arch_unmap_area;
> -}
> -#endif
>  
>  #ifdef CONFIG_TRACING
>  extern void
> diff -puN mm/mmap.c~a mm/mmap.c
> --- a/mm/mmap.c~a
> +++ a/mm/mmap.c
> @@ -2268,3 +2268,12 @@ int install_special_mapping(struct mm_st
>  
>  	return 0;
>  }
> +
> +#ifndef HAVE_ARCH_PICK_MMAP_LAYOUT
> +void arch_pick_mmap_layout(struct mm_struct *mm)
> +{
> +	mm->mmap_base = TASK_UNMAPPED_BASE;
> +	mm->get_unmapped_area = arch_get_unmapped_area;
> +	mm->unmap_area = arch_unmap_area;
> +}
> +#endif

Nice, this seems to fix the problem.

> > and more nasty problems follow later.
> > 
> > My suggestion is to:
> > - revert commit 27ac792ca0b0a1e7e65f20342260650516c95864 and then
> > - fix all PAGE_ALIGN() instances without moving them.
> 
> Every time this patch blew up (and it did it often), fixing it resulted
> in overall improvements.
> 
> > Unifying code is a good thing, but in this case it is not worth the 
> > trouble it causes by poking into the heart of our headers mess.
> 
> Well.   If we leave it a mess, it'll stay a mess.

An interesting question is whether the PAGE_ALIGN() move makes the mess 
bigger or smaller.

Ideally, all headers should be self-contained. IOW, they should #include 
everything they use.

But TASK_UNMAPPED_BASE in asm/processor.h on some architectures uses 
PAGE_ALIGN() that got moved from asm/page.h to linux/mm.h .

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: PAGE_ALIGN() compile breakage

[Andrew Morton]

On Fri, 25 Jul 2008 12:14:55 +0300 Adrian Bunk <bunk@kernel.org> wrote:

> Ideally, all headers should be self-contained. IOW, they should #include 
> everything they use.

Yup.  And the core reason for our headers mess is that the headers do
too much stuff, and cnosequently demand a large dependency trail.

> But TASK_UNMAPPED_BASE in asm/processor.h on some architectures uses 
> PAGE_ALIGN() that got moved from asm/page.h to linux/mm.h .

Probably mm.h should be split up - put the simple things (usually
declarations) into one "early" header file and leave the more
heavyweight things (usually implementations) in mm.h.

Re: PAGE_ALIGN() compile breakage

[Adrian Bunk]

On Fri, Jul 25, 2008 at 12:14:55PM +0300, Adrian Bunk wrote:
> On Fri, Jul 25, 2008 at 01:55:37AM -0700, Andrew Morton wrote:
>...
> > pls test:
> > 
> > diff -puN include/linux/sched.h~a include/linux/sched.h
> > --- a/include/linux/sched.h~a
> > +++ a/include/linux/sched.h
> > @@ -2139,16 +2139,7 @@ static inline void set_task_cpu(struct t
> >  
> >  #endif /* CONFIG_SMP */
> >  
> > -#ifdef HAVE_ARCH_PICK_MMAP_LAYOUT
> >  extern void arch_pick_mmap_layout(struct mm_struct *mm);
> > -#else
> > -static inline void arch_pick_mmap_layout(struct mm_struct *mm)
> > -{
> > -	mm->mmap_base = TASK_UNMAPPED_BASE;
> > -	mm->get_unmapped_area = arch_get_unmapped_area;
> > -	mm->unmap_area = arch_unmap_area;
> > -}
> > -#endif
> >  
> >  #ifdef CONFIG_TRACING
> >  extern void
> > diff -puN mm/mmap.c~a mm/mmap.c
> > --- a/mm/mmap.c~a
> > +++ a/mm/mmap.c
> > @@ -2268,3 +2268,12 @@ int install_special_mapping(struct mm_st
> >  
> >  	return 0;
> >  }
> > +
> > +#ifndef HAVE_ARCH_PICK_MMAP_LAYOUT
> > +void arch_pick_mmap_layout(struct mm_struct *mm)
> > +{
> > +	mm->mmap_base = TASK_UNMAPPED_BASE;
> > +	mm->get_unmapped_area = arch_get_unmapped_area;
> > +	mm->unmap_area = arch_unmap_area;
> > +}
> > +#endif
> 
> Nice, this seems to fix the problem.
>...

Further testing revealed that you should choose a file that also gets 
compiled on MMU-less architectures:

<--  snip  -->

...
  LD      vmlinux
fs/built-in.o: In function `flush_old_exec':
(.text+0x6ae8): undefined reference to `arch_pick_mmap_layout'
fs/built-in.o: In function `flush_old_exec':
(.text+0x6cf0): undefined reference to `arch_pick_mmap_layout'
make[1]: *** [vmlinux] Error 1

<--  snip  -->

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: PAGE_ALIGN() compile breakage

[Andrew Morton]

On Fri, 25 Jul 2008 12:27:48 +0300 Adrian Bunk <bunk@kernel.org> wrote:

> Further testing revealed that you should choose a file that also gets 
> compiled on MMU-less architectures:

We don't have one :(

I guess util.c will have to do.

diff -puN include/linux/sched.h~uninline-arch_pick_mmap_layout include/linux/sched.h
--- a/include/linux/sched.h~uninline-arch_pick_mmap_layout
+++ a/include/linux/sched.h
@@ -2139,16 +2139,7 @@ static inline void set_task_cpu(struct t
 
 #endif /* CONFIG_SMP */
 
-#ifdef HAVE_ARCH_PICK_MMAP_LAYOUT
 extern void arch_pick_mmap_layout(struct mm_struct *mm);
-#else
-static inline void arch_pick_mmap_layout(struct mm_struct *mm)
-{
-	mm->mmap_base = TASK_UNMAPPED_BASE;
-	mm->get_unmapped_area = arch_get_unmapped_area;
-	mm->unmap_area = arch_unmap_area;
-}
-#endif
 
 #ifdef CONFIG_TRACING
 extern void
diff -puN mm/util.c~uninline-arch_pick_mmap_layout mm/util.c
--- a/mm/util.c~uninline-arch_pick_mmap_layout
+++ a/mm/util.c
@@ -1,3 +1,4 @@
+#include <linux/mm.h>
 #include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/module.h>
@@ -160,3 +161,12 @@ char *strndup_user(const char __user *s,
 	return p;
 }
 EXPORT_SYMBOL(strndup_user);
+
+#ifndef HAVE_ARCH_PICK_MMAP_LAYOUT
+void arch_pick_mmap_layout(struct mm_struct *mm)
+{
+	mm->mmap_base = TASK_UNMAPPED_BASE;
+	mm->get_unmapped_area = arch_get_unmapped_area;
+	mm->unmap_area = arch_unmap_area;
+}
+#endif
_


We should make arch_pick_mmap_layout __weak and nuke that ifdef.

__weak vs ifdef

[Matthew Wilcox]

On Fri, Jul 25, 2008 at 02:34:55AM -0700, Andrew Morton wrote:
> We should make arch_pick_mmap_layout __weak and nuke that ifdef.

I strongly disagree.  I find it makes it harder to follow code flow
when __weak functions are involved.  Ifdefs are ugly, no question, but
they're easier to grep for, see when they'll be defined and know which of
the arch_pick_mmap_layout() functions will be called.  __weak certainly
has its uses (eg the sys_ni_syscall is great) but I find it's becoming
overused.

My basic point here is that __weak makes the code easier to write but
harder to read, and we're supposed to be optimising for easier to read.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: __weak vs ifdef

[Andrew Morton]

On Fri, 25 Jul 2008 06:24:54 -0600 Matthew Wilcox <matthew@wil.cx> wrote:

> On Fri, Jul 25, 2008 at 02:34:55AM -0700, Andrew Morton wrote:
> > We should make arch_pick_mmap_layout __weak and nuke that ifdef.
> 
> I strongly disagree.  I find it makes it harder to follow code flow
> when __weak functions are involved.  Ifdefs are ugly, no question, but
> they're easier to grep for, see when they'll be defined and know which of
> the arch_pick_mmap_layout() functions will be called.  __weak certainly
> has its uses (eg the sys_ni_syscall is great) but I find it's becoming
> overused.
> 
> My basic point here is that __weak makes the code easier to write but
> harder to read, and we're supposed to be optimising for easier to read.
> 

If you see

void __weak arch_foo(...)

and can't immediately work out what's going on then converting it to an
ifdef maze won't save you.

Re: PAGE_ALIGN() compile breakage

[Andrea Righi]

Andrew Morton wrote:
> On Fri, 25 Jul 2008 12:14:55 +0300 Adrian Bunk <bunk@kernel.org> wrote:
> 
>> Ideally, all headers should be self-contained. IOW, they should #include 
>> everything they use.
> 
> Yup.  And the core reason for our headers mess is that the headers do
> too much stuff, and cnosequently demand a large dependency trail.
> 
>> But TASK_UNMAPPED_BASE in asm/processor.h on some architectures uses 
>> PAGE_ALIGN() that got moved from asm/page.h to linux/mm.h .
> 
> Probably mm.h should be split up - put the simple things (usually
> declarations) into one "early" header file and leave the more
> heavyweight things (usually implementations) in mm.h.

IMHO splitting mm.h is probably the best solution. If I'm not wrong
Paul (CCed) already suggested to move the stuff like PAGE_ALIGN() outside
mm.h the first time I submitted this patch.

In this way we could even include the "lightweight" mm.h (mm_define.h??)
in all the asm-*/page.h, preserving also the backward compatibility.

-Andrea

Re: __weak vs ifdef

[Linus Torvalds]

On Fri, 25 Jul 2008, Matthew Wilcox wrote:

> On Fri, Jul 25, 2008 at 02:34:55AM -0700, Andrew Morton wrote:
> > We should make arch_pick_mmap_layout __weak and nuke that ifdef.
> 
> I strongly disagree.  I find it makes it harder to follow code flow
> when __weak functions are involved.  Ifdefs are ugly, no question, but
> they're easier to grep for

Hell no, they're not.

Our use of random HAVE_ARCH_xyz or ARCH_SUPPORTS_xyz etc stuff makes 
things _totally_ impossible to grep for.

In contrast, it we did this code as

	#ifndef arch_pick_mmap_layout
	void __weak arch_pick_mmap_layout(struct mm_struct *mm)
	{
		mm->mmap_base = TASK_UNMAPPED_BASE;
		mm->get_unmapped_area = arch_get_unmapped_area;
		mm->unmap_area = arch_unmap_area;
	}
	#endif

then trying to grep for arch_pick_mmap_layout() would show EVERY SINGLE 
RELEVANT CASE! And it would show the "__weak" there too, so that once 
people get used to this convention, they'd have a really easy time 
figuring out the rules from just the output of the 'grep'.

I really think that whoever started that 'HAVE_ARCH_x'/'ARCH_HAS_x' mess 
with totally random symbols that have NOTHING WHAT-SO-EVER to do with the 
actual symbols in question (so they do _not_ show up in grep'ing for some 
use) should be shot. 

We should never _ever_ use that model. And we use it way too much.

We should generally strive for the simpler and much more obvious

	/* Generic definition */
	#ifndef symbol
	int symbol(..)
	...
	#endif

and then architecture code can do

	#define symbol(x) ...

or if they want to do a function, and you _really_ don't like the '__weak' 
part (or you want to make it an inline function and don't want the clash 
with the real declaration), then you can just do

	static inline int symbol(x)
	{
		...
	}
	#define symbol symbol

and again it all works fine WITHOUT having to introduce some idiotic new 
and unrelated element called ARCH_HAS_SYMBOL.

And now when you do 'git grep symbol' you really will see the rules. ALL 
the rules. Not some random collection of uses that don't actually explain 
why there are five different definitions of the same thing and then you 
have to figure out which one gets used.

> My basic point here is that __weak makes the code easier to write but
> harder to read, and we're supposed to be optimising for easier to read.

But your basic point is flawed. The thing you advocate is actually harder 
to read.

Yes, if you don't follow the codign style, and you write

	int __weak
	symbol(x)
	{

you are (a) a moronic rebel you never understood why the declaration 
should be on one line and (b) as a result your 'grep' won't see the __weak 
and you'll be confused about the rules.

But if we _consistently_ used

 - '#ifndef symbol' to avoid redeclaring something that the architecture 
   overrides

 - and '__weak' to allow architectures to just override functions without 
   extra work and rules

then after a while people would simply _know_ that very simple set of 
rules, and a 'grep' would work so much better than it does now.

Really. Try it. Try it with 'arch_pick_mmap_layout' (with Andrews patch in 
place). And then imagine that you'd be used to '__weak', and seeing that 
additional

	mm/util.c:#ifndef arch_pick_mmap_layout
	mm/util.c:void __weak arch_pick_mmap_layout(struct mm_struct *mm)

in the output. Be honest now - wouldn't that actually _tell_ you something 
relevant about that particular declaration? And make the fact that some 
architectures override it _less_ confusing?

IOW, you could tell directly from the grep output that it's a "default 
fallback". Which you definitely cannot tell right now, because we have 
insane models for doing it.

		Linus

[-mm patch] mm/util.c must #include <linux/sched.h>

[Adrian Bunk]

On Fri, Jul 25, 2008 at 02:34:55AM -0700, Andrew Morton wrote:
> On Fri, 25 Jul 2008 12:27:48 +0300 Adrian Bunk <bunk@kernel.org> wrote:
> 
> > Further testing revealed that you should choose a file that also gets 
> > compiled on MMU-less architectures:
> 
> We don't have one :(
> 
> I guess util.c will have to do.
>...

Requires the fix below.

cu
Adrian


<--  snip  -->


This patch fixes the following build error:

<--  snip  -->

...
  CC      mm/util.o
/home/bunk/linux/kernel-2.6/git/linux-2.6/mm/util.c: In function 'arch_pick_mmap_layout':
/home/bunk/linux/kernel-2.6/git/linux-2.6/mm/util.c:144: error: dereferencing pointer to incomplete type
/home/bunk/linux/kernel-2.6/git/linux-2.6/mm/util.c:145: error: 'arch_get_unmapped_area' undeclared (first use in this function)
/home/bunk/linux/kernel-2.6/git/linux-2.6/mm/util.c:145: error: (Each undeclared identifier is reported only once
/home/bunk/linux/kernel-2.6/git/linux-2.6/mm/util.c:145: error: for each function it appears in.)
/home/bunk/linux/kernel-2.6/git/linux-2.6/mm/util.c:146: error: 'arch_unmap_area' undeclared (first use in this function)
make[2]: *** [mm/util.o] Error 1

<--  snip  -->

Signed-off-by: Adrian Bunk <bunk@kernel.org>

---
f003405997b99d300c99f7b1eae408fe42c07126 
diff --git a/mm/util.c b/mm/util.c
index 0efd830..d8d0221 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -3,6 +3,7 @@
 #include <linux/string.h>
 #include <linux/module.h>
 #include <linux/err.h>
+#include <linux/sched.h>
 #include <asm/uaccess.h>
 
 /**

Re: __weak vs ifdef

[Grant Likely]

Reaching back into an old discussion....

On Sat, Jul 26, 2008 at 12:38 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
>
> On Fri, 25 Jul 2008, Matthew Wilcox wrote:
>
>> On Fri, Jul 25, 2008 at 02:34:55AM -0700, Andrew Morton wrote:
>> > We should make arch_pick_mmap_layout __weak and nuke that ifdef.
>>
>> I strongly disagree.  I find it makes it harder to follow code flow
>> when __weak functions are involved.  Ifdefs are ugly, no question, but
>> they're easier to grep for
>
> Hell no, they're not.
>
> Our use of random HAVE_ARCH_xyz or ARCH_SUPPORTS_xyz etc stuff makes
> things _totally_ impossible to grep for.
>
> In contrast, it we did this code as
>
>        #ifndef arch_pick_mmap_layout
>        void __weak arch_pick_mmap_layout(struct mm_struct *mm)
>        {
>                mm->mmap_base = TASK_UNMAPPED_BASE;
>                mm->get_unmapped_area = arch_get_unmapped_area;
>                mm->unmap_area = arch_unmap_area;
>        }
>        #endif
>
> then trying to grep for arch_pick_mmap_layout() would show EVERY SINGLE
> RELEVANT CASE! And it would show the "__weak" there too, so that once
> people get used to this convention, they'd have a really easy time
> figuring out the rules from just the output of the 'grep'.
[...]

Question.  If I use this pattern, and use the __weak attribute on core
code functions wrapped with a #ifndef, then how does it mesh with
EXPORT_SYMBOL*() statements?  Do both the core code, and the arch
override need to do EXPORT_SYMBOL(), or should EXPORT_SYMBOL() only
appear at the core code site?

I also assume that at the core code site, the EXPORT_SYMBOL() must
appear inside the #ifndef block so that a #define override doesn't
break things.  Correct?

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: __weak vs ifdef

[Linus Torvalds]

On Wed, 17 Feb 2010, Grant Likely wrote:
> 
> Question.  If I use this pattern, and use the __weak attribute on core
> code functions wrapped with a #ifndef, then how does it mesh with
> EXPORT_SYMBOL*() statements?

You can't. Or at least not the traditional way, which is to put the 
EXPORT_SYMBOL next to the definition of what gets exported.

If you use __weak, you need to make sure that all users of said weak 
symbol are in another file. There are some compiler and/or binutils bugs 
with using weak symbols in the same translation unit, so the rule is that 
a weak definition hes to be somewhere else from the use - including 
EXPORT_SYMBOL.

> I also assume that at the core code site, the EXPORT_SYMBOL() must
> appear inside the #ifndef block so that a #define override doesn't
> break things.  Correct?

See above. You can't put it inside the _same_ #ifndef block. You'd have to 
put it in a different file, but yes, inside an ifndef. At which point the 
linker will just pick the right definition.

However, in general, this probably gets ugly enough that the whole __weak 
thing is not great for exported stuff. It's likely mostly useful for some 
"generic" arch functionality that is always compiled in.

		Linus