Message ID | 1248740260.30993.26.camel@pasglop (mailing list archive) |
---|---|
State | Accepted, archived |
Headers | show |
On Tue, Jul 28, 2009 at 10:17:40AM +1000, Benjamin Herrenschmidt wrote: > On Mon, 2009-07-27 at 12:11 -0700, Linus Torvalds wrote: > > On Thu, 23 Jul 2009, Benjamin Herrenschmidt wrote: > > > > > > Hrm... my powerpc-next branch will contain stuff that depend on it, so > > > I'll probably have to pull it in though, unless I tell all my > > > sub-maintainers to also pull from that other branch first :-) > > > > Ok, I'll just apply the patch. It does look obvious enough. > > There seem to be a MIPS and SH breakage as a result but I can't see > how my patch would have broken it, ie, it looks like the bug was > already in those two archs. The error is that it complains about a > duplicate definition of __pmd_free_tlb() between those arch pgalloc.h > and pgtable-nopmd.h > > For MIPS, when CONFIG_32BIT is set, asm/pgalloc.h redefines > __pmd_free_tlb despite the fact that it's already defined by > asm-generic/pgtable-nopmd.h (via via pgtable.h via linux/mm.h). > > I -suspect- what happens is that the compiler, before, would ignore the > double definition (or maybe just warn) due to the definition being > strictly identical. With the new argument added, it's no longer the case > as it's called "a" in asm-generic and "addr" in mips... oops. > > In any case, can Ralf and Paul check if the following patch is correct ? > Yup, that seems to be what happened. I've never seen a warning about this with any compiler version, otherwise we would have caught this much earlier. As soon as the addr -> a rename took place it blew up immediately as a redefinition. Is there a magical gcc flag we can turn on to warn on identical definitions, even if just for testing? > >From 41928c7945d855ae0eb053eadad590ab6876847e Mon Sep 17 00:00:00 2001 > From: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Date: Tue, 28 Jul 2009 10:16:48 +1000 > Subject: [PATCH] mm: Remove duplicate definitions in MIPS and SH > > Those definitions are already provided by asm-generic > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> Builds and boots fine, thanks. Acked-by: Paul Mundt <lethal@linux-sh.org>
On Tue, 28 Jul 2009, Paul Mundt wrote: > > Yup, that seems to be what happened. I've never seen a warning about this > with any compiler version, otherwise we would have caught this much > earlier. As soon as the addr -> a rename took place it blew up > immediately as a redefinition. Is there a magical gcc flag we can turn on > to warn on identical definitions, even if just for testing? No, this is actually defined C behavior - identical macro redefinitions are ok. That's very much on purpose, and allows different header files to use an identical #define to define some common macro. Strictly speaking, this is a "safety feature", in that you obviously _could_ just always do a #undef+#define, but such a case would be able to redefine a macro even if the new definition didn't match the old one. So the C pre-processor rules is that you can safely re-define something if you re-define it identically. Of course, we could make the rules for the kernel be stricter, but I don't know if there are any flags to warn about it, since it's such a standard C feature: the lack of warning is _not_ an accident. It would be trivial to teach sparse to warn about it, of course. Look at sparse/pre-process.c, function do_handle_define(). Notice how it literally checks that any previous #define is identical in both expansion and argument list, with: if (token_list_different(sym->expansion, expansion) || token_list_different(sym->arglist, arglist)) { and just make token_list_different() always return true (this is the only use of that function). I haven't checked if such a change would actually result in a lot of warnings. Linus
diff --git a/arch/mips/include/asm/pgalloc.h b/arch/mips/include/asm/pgalloc.h index f705735..3738f4b 100644 --- a/arch/mips/include/asm/pgalloc.h +++ b/arch/mips/include/asm/pgalloc.h @@ -104,17 +104,6 @@ do { \ tlb_remove_page((tlb), pte); \ } while (0) -#ifdef CONFIG_32BIT - -/* - * allocating and freeing a pmd is trivial: the 1-entry pmd is - * inside the pgd, so has no extra memory associated with it. - */ -#define pmd_free(mm, x) do { } while (0) -#define __pmd_free_tlb(tlb, x, addr) do { } while (0) - -#endif - #ifdef CONFIG_64BIT static inline pmd_t *pmd_alloc_one(struct mm_struct *mm, unsigned long address) diff --git a/arch/sh/include/asm/pgalloc.h b/arch/sh/include/asm/pgalloc.h index 89a4827..63ca37b 100644 --- a/arch/sh/include/asm/pgalloc.h +++ b/arch/sh/include/asm/pgalloc.h @@ -79,14 +79,6 @@ do { \ tlb_remove_page((tlb), (pte)); \ } while (0) -/* - * allocating and freeing a pmd is trivial: the 1-entry pmd is - * inside the pgd, so has no extra memory associated with it. - */ - -#define pmd_free(mm, x) do { } while (0) -#define __pmd_free_tlb(tlb,x,addr) do { } while (0) - static inline void check_pgt_cache(void) { quicklist_trim(QUICK_PGD, NULL, 25, 16);