* PAGE_ALIGN() compile breakage
@ 2008-07-25 8:39 Adrian Bunk
2008-07-25 8:55 ` Andrew Morton
0 siblings, 1 reply; 13+ messages in thread
From: Adrian Bunk @ 2008-07-25 8:39 UTC (permalink / raw)
To: Andrea Righi, Andrew Morton, Linus Torvalds; +Cc: linux-arch, linux-kernel
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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: PAGE_ALIGN() compile breakage 2008-07-25 8:39 PAGE_ALIGN() compile breakage Adrian Bunk @ 2008-07-25 8:55 ` Andrew Morton 2008-07-25 9:14 ` Adrian Bunk 0 siblings, 1 reply; 13+ messages in thread From: Andrew Morton @ 2008-07-25 8:55 UTC (permalink / raw) To: Adrian Bunk; +Cc: Andrea Righi, Linus Torvalds, linux-arch, linux-kernel 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: PAGE_ALIGN() compile breakage 2008-07-25 8:55 ` Andrew Morton @ 2008-07-25 9:14 ` Adrian Bunk 2008-07-25 9:25 ` Andrew Morton 2008-07-25 9:27 ` Adrian Bunk 0 siblings, 2 replies; 13+ messages in thread From: Adrian Bunk @ 2008-07-25 9:14 UTC (permalink / raw) To: Andrew Morton; +Cc: Andrea Righi, Linus Torvalds, linux-arch, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: PAGE_ALIGN() compile breakage 2008-07-25 9:14 ` Adrian Bunk @ 2008-07-25 9:25 ` Andrew Morton 2008-07-25 18:34 ` Andrea Righi 2008-07-25 9:27 ` Adrian Bunk 1 sibling, 1 reply; 13+ messages in thread From: Andrew Morton @ 2008-07-25 9:25 UTC (permalink / raw) To: Adrian Bunk; +Cc: Andrea Righi, Linus Torvalds, linux-arch, linux-kernel 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: PAGE_ALIGN() compile breakage 2008-07-25 9:25 ` Andrew Morton @ 2008-07-25 18:34 ` Andrea Righi 0 siblings, 0 replies; 13+ messages in thread From: Andrea Righi @ 2008-07-25 18:34 UTC (permalink / raw) To: Andrew Morton Cc: Adrian Bunk, Linus Torvalds, linux-arch, linux-kernel, Paul Mackerras 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: PAGE_ALIGN() compile breakage 2008-07-25 9:14 ` Adrian Bunk 2008-07-25 9:25 ` Andrew Morton @ 2008-07-25 9:27 ` Adrian Bunk 2008-07-25 9:34 ` Andrew Morton 1 sibling, 1 reply; 13+ messages in thread From: Adrian Bunk @ 2008-07-25 9:27 UTC (permalink / raw) To: Andrew Morton; +Cc: Andrea Righi, Linus Torvalds, linux-arch, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: PAGE_ALIGN() compile breakage 2008-07-25 9:27 ` Adrian Bunk @ 2008-07-25 9:34 ` Andrew Morton 2008-07-25 12:24 ` __weak vs ifdef Matthew Wilcox 2008-07-26 21:22 ` [-mm patch] mm/util.c must #include <linux/sched.h> Adrian Bunk 0 siblings, 2 replies; 13+ messages in thread From: Andrew Morton @ 2008-07-25 9:34 UTC (permalink / raw) To: Adrian Bunk; +Cc: Andrea Righi, Linus Torvalds, linux-arch, linux-kernel 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* __weak vs ifdef 2008-07-25 9:34 ` Andrew Morton @ 2008-07-25 12:24 ` Matthew Wilcox 2008-07-25 12:41 ` Andrew Morton 2008-07-26 19:38 ` Linus Torvalds 2008-07-26 21:22 ` [-mm patch] mm/util.c must #include <linux/sched.h> Adrian Bunk 1 sibling, 2 replies; 13+ messages in thread From: Matthew Wilcox @ 2008-07-25 12:24 UTC (permalink / raw) To: Andrew Morton Cc: Adrian Bunk, Andrea Righi, Linus Torvalds, linux-arch, linux-kernel 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." ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: __weak vs ifdef 2008-07-25 12:24 ` __weak vs ifdef Matthew Wilcox @ 2008-07-25 12:41 ` Andrew Morton 2008-07-26 19:38 ` Linus Torvalds 1 sibling, 0 replies; 13+ messages in thread From: Andrew Morton @ 2008-07-25 12:41 UTC (permalink / raw) To: Matthew Wilcox Cc: Adrian Bunk, Andrea Righi, Linus Torvalds, linux-arch, linux-kernel 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: __weak vs ifdef 2008-07-25 12:24 ` __weak vs ifdef Matthew Wilcox 2008-07-25 12:41 ` Andrew Morton @ 2008-07-26 19:38 ` Linus Torvalds 2010-02-18 2:07 ` Grant Likely 1 sibling, 1 reply; 13+ messages in thread From: Linus Torvalds @ 2008-07-26 19:38 UTC (permalink / raw) To: Matthew Wilcox Cc: Andrew Morton, Adrian Bunk, Andrea Righi, linux-arch, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: __weak vs ifdef 2008-07-26 19:38 ` Linus Torvalds @ 2010-02-18 2:07 ` Grant Likely 2010-02-18 2:22 ` Linus Torvalds 0 siblings, 1 reply; 13+ messages in thread From: Grant Likely @ 2010-02-18 2:07 UTC (permalink / raw) To: Linus Torvalds Cc: Matthew Wilcox, Andrew Morton, Adrian Bunk, Andrea Righi, linux-arch, linux-kernel 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: __weak vs ifdef 2010-02-18 2:07 ` Grant Likely @ 2010-02-18 2:22 ` Linus Torvalds 0 siblings, 0 replies; 13+ messages in thread From: Linus Torvalds @ 2010-02-18 2:22 UTC (permalink / raw) To: Grant Likely Cc: Matthew Wilcox, Andrew Morton, Adrian Bunk, Andrea Righi, linux-arch, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [-mm patch] mm/util.c must #include <linux/sched.h> 2008-07-25 9:34 ` Andrew Morton 2008-07-25 12:24 ` __weak vs ifdef Matthew Wilcox @ 2008-07-26 21:22 ` Adrian Bunk 1 sibling, 0 replies; 13+ messages in thread From: Adrian Bunk @ 2008-07-26 21:22 UTC (permalink / raw) To: Andrew Morton; +Cc: Andrea Righi, Linus Torvalds, linux-arch, linux-kernel 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> /** ^ permalink raw reply related [flat|nested] 13+ messages in thread
end of thread, other threads:[~2010-02-18 2:22 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-07-25 8:39 PAGE_ALIGN() compile breakage Adrian Bunk 2008-07-25 8:55 ` Andrew Morton 2008-07-25 9:14 ` Adrian Bunk 2008-07-25 9:25 ` Andrew Morton 2008-07-25 18:34 ` Andrea Righi 2008-07-25 9:27 ` Adrian Bunk 2008-07-25 9:34 ` Andrew Morton 2008-07-25 12:24 ` __weak vs ifdef Matthew Wilcox 2008-07-25 12:41 ` Andrew Morton 2008-07-26 19:38 ` Linus Torvalds 2010-02-18 2:07 ` Grant Likely 2010-02-18 2:22 ` Linus Torvalds 2008-07-26 21:22 ` [-mm patch] mm/util.c must #include <linux/sched.h> Adrian Bunk
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).