[PATCH 0/3] ARM: unwind extension

[PATCH 0/3] ARM: unwind extension

[Phil Carmody]

The first two patches are simply preparation for the third, making it
effectively trivial, even though it's the only one with a concrete 
change in behaviour.

The origins of this patchset are the discovery that unwind and kmemleak
don't always cooperate well with each other - any allocation within 
an exit or devexit function causes kmemleak to look up symbols that 
aren't in any unwind table. This of course means that all WARN_ONs and
BUGs will suffer the same fate.

It could certainly be said that with a typical system the linked list
has grown too large to be practical as a container, and some improvements
could be made in that direction in the future.

Cheers,
Phil

[PATCH 1/3] ARM: module - simplify code with temporaries

[Phil Carmody]

Less to read.

Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
---
 arch/arm/kernel/module.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
index c628bdf..d531a63 100644
--- a/arch/arm/kernel/module.c
+++ b/arch/arm/kernel/module.c
@@ -71,17 +71,19 @@ int module_frob_arch_sections(Elf_Ehdr *hdr,
 	Elf_Shdr *s, *sechdrs_end = sechdrs + hdr->e_shnum;
 
 	for (s = sechdrs; s < sechdrs_end; s++) {
-		if (strcmp(".ARM.exidx.init.text", secstrings + s->sh_name) == 0)
+		char const *secname = secstrings + s->sh_name;
+
+		if (strcmp(".ARM.exidx.init.text", secname) == 0)
 			mod->arch.unw_sec_init = s;
-		else if (strcmp(".ARM.exidx.devinit.text", secstrings + s->sh_name) == 0)
+		else if (strcmp(".ARM.exidx.devinit.text", secname) == 0)
 			mod->arch.unw_sec_devinit = s;
-		else if (strcmp(".ARM.exidx", secstrings + s->sh_name) == 0)
+		else if (strcmp(".ARM.exidx", secname) == 0)
 			mod->arch.unw_sec_core = s;
-		else if (strcmp(".init.text", secstrings + s->sh_name) == 0)
+		else if (strcmp(".init.text", secname) == 0)
 			mod->arch.sec_init_text = s;
-		else if (strcmp(".devinit.text", secstrings + s->sh_name) == 0)
+		else if (strcmp(".devinit.text", secname) == 0)
 			mod->arch.sec_devinit_text = s;
-		else if (strcmp(".text", secstrings + s->sh_name) == 0)
+		else if (strcmp(".text", secname) == 0)
 			mod->arch.sec_core_text = s;
 	}
 #endif
-- 
1.6.0.4

[PATCH 2/3] ARM: module - simplify unwind table handling

[Phil Carmody]

The various sections are all dealt with similarly, so factor out
that common behaviour.

Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
---
 arch/arm/include/asm/module.h |   29 +++++++++++++++----------
 arch/arm/kernel/module.c      |   46 +++++++++++++++++------------------------
 2 files changed, 36 insertions(+), 39 deletions(-)

diff --git a/arch/arm/include/asm/module.h b/arch/arm/include/asm/module.h
index e4dfa69..2164061 100644
--- a/arch/arm/include/asm/module.h
+++ b/arch/arm/include/asm/module.h
@@ -7,20 +7,25 @@
 
 struct unwind_table;
 
-struct mod_arch_specific
-{
 #ifdef CONFIG_ARM_UNWIND
-	Elf_Shdr *unw_sec_init;
-	Elf_Shdr *unw_sec_devinit;
-	Elf_Shdr *unw_sec_core;
-	Elf_Shdr *sec_init_text;
-	Elf_Shdr *sec_devinit_text;
-	Elf_Shdr *sec_core_text;
-	struct unwind_table *unwind_init;
-	struct unwind_table *unwind_devinit;
-	struct unwind_table *unwind_core;
-#endif
+struct arm_unwind_mapping {
+	Elf_Shdr *unw_sec;
+	Elf_Shdr *sec_text;
+	struct unwind_table *unwind;
+};
+enum {
+	ARM_SEC_INIT,
+	ARM_SEC_DEVINIT,
+	ARM_SEC_CORE,
+	ARM_SEC_MAX,
 };
+struct mod_arch_specific {
+	struct arm_unwind_mapping map[ARM_SEC_MAX];
+};
+#else
+struct mod_arch_specific {
+}
+#endif
 
 /*
  * Include the ARM architecture version.
diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
index d531a63..bcf928e 100644
--- a/arch/arm/kernel/module.c
+++ b/arch/arm/kernel/module.c
@@ -69,22 +69,23 @@ int module_frob_arch_sections(Elf_Ehdr *hdr,
 {
 #ifdef CONFIG_ARM_UNWIND
 	Elf_Shdr *s, *sechdrs_end = sechdrs + hdr->e_shnum;
+	struct arm_unwind_mapping *maps = mod->arch.map;
 
 	for (s = sechdrs; s < sechdrs_end; s++) {
 		char const *secname = secstrings + s->sh_name;
 
 		if (strcmp(".ARM.exidx.init.text", secname) == 0)
-			mod->arch.unw_sec_init = s;
+			maps[ARM_SEC_INIT].unw_sec = s;
 		else if (strcmp(".ARM.exidx.devinit.text", secname) == 0)
-			mod->arch.unw_sec_devinit = s;
+			maps[ARM_SEC_DEVINIT].unw_sec = s;
 		else if (strcmp(".ARM.exidx", secname) == 0)
-			mod->arch.unw_sec_core = s;
+			maps[ARM_SEC_CORE].unw_sec = s;
 		else if (strcmp(".init.text", secname) == 0)
-			mod->arch.sec_init_text = s;
+			maps[ARM_SEC_INIT].sec_text = s;
 		else if (strcmp(".devinit.text", secname) == 0)
-			mod->arch.sec_devinit_text = s;
+			maps[ARM_SEC_DEVINIT].sec_text = s;
 		else if (strcmp(".text", secname) == 0)
-			mod->arch.sec_core_text = s;
+			maps[ARM_SEC_CORE].sec_text = s;
 	}
 #endif
 	return 0;
@@ -260,31 +261,22 @@ apply_relocate_add(Elf32_Shdr *sechdrs, const char *strtab,
 #ifdef CONFIG_ARM_UNWIND
 static void register_unwind_tables(struct module *mod)
 {
-	if (mod->arch.unw_sec_init && mod->arch.sec_init_text)
-		mod->arch.unwind_init =
-			unwind_table_add(mod->arch.unw_sec_init->sh_addr,
-					 mod->arch.unw_sec_init->sh_size,
-					 mod->arch.sec_init_text->sh_addr,
-					 mod->arch.sec_init_text->sh_size);
-	if (mod->arch.unw_sec_devinit && mod->arch.sec_devinit_text)
-		mod->arch.unwind_devinit =
-			unwind_table_add(mod->arch.unw_sec_devinit->sh_addr,
-					 mod->arch.unw_sec_devinit->sh_size,
-					 mod->arch.sec_devinit_text->sh_addr,
-					 mod->arch.sec_devinit_text->sh_size);
-	if (mod->arch.unw_sec_core && mod->arch.sec_core_text)
-		mod->arch.unwind_core =
-			unwind_table_add(mod->arch.unw_sec_core->sh_addr,
-					 mod->arch.unw_sec_core->sh_size,
-					 mod->arch.sec_core_text->sh_addr,
-					 mod->arch.sec_core_text->sh_size);
+	int i;
+	for (i = 0; i < ARM_SEC_MAX; ++i) {
+		struct arm_unwind_mapping *map = &mod->arch.map[i];
+		if (map->unw_sec && map->sec_text)
+			map->unwind = unwind_table_add(map->unw_sec->sh_addr,
+						       map->unw_sec->sh_size,
+						       map->sec_text->sh_addr,
+						       map->sec_text->sh_size);
+	}
 }
 
 static void unregister_unwind_tables(struct module *mod)
 {
-	unwind_table_del(mod->arch.unwind_init);
-	unwind_table_del(mod->arch.unwind_devinit);
-	unwind_table_del(mod->arch.unwind_core);
+	int i = ARM_SEC_MAX;
+	while (--i >= 0)
+		unwind_table_del(mod->arch.map[i].unwind);
 }
 #else
 static inline void register_unwind_tables(struct module *mod) { }
-- 
1.6.0.4

[PATCH 3/3] ARM: module - additional unwind tables for exit/devexit sections

[Phil Carmody]

Without these, exit functions cannot be stack-traced, so to speak.
This implies that module unloads that perform allocations (don't
laugh) will cause noisy warnings on the console when kmemleak is
enabled, as it presumes that all code's call chains are traceable.
Similarly, BUGs and WARN_ONs will give additional console spam.

Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
---
 arch/arm/include/asm/module.h |    2 ++
 arch/arm/kernel/module.c      |    8 ++++++++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/module.h b/arch/arm/include/asm/module.h
index 2164061..fcff0d2 100644
--- a/arch/arm/include/asm/module.h
+++ b/arch/arm/include/asm/module.h
@@ -17,6 +17,8 @@ enum {
 	ARM_SEC_INIT,
 	ARM_SEC_DEVINIT,
 	ARM_SEC_CORE,
+	ARM_SEC_EXIT,
+	ARM_SEC_DEVEXIT,
 	ARM_SEC_MAX,
 };
 struct mod_arch_specific {
diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
index bcf928e..8eaff1c 100644
--- a/arch/arm/kernel/module.c
+++ b/arch/arm/kernel/module.c
@@ -80,12 +80,20 @@ int module_frob_arch_sections(Elf_Ehdr *hdr,
 			maps[ARM_SEC_DEVINIT].unw_sec = s;
 		else if (strcmp(".ARM.exidx", secname) == 0)
 			maps[ARM_SEC_CORE].unw_sec = s;
+		else if (strcmp(".ARM.exidx.exit.text", secname) == 0)
+			maps[ARM_SEC_EXIT].unw_sec = s;
+		else if (strcmp(".ARM.exidx.devexit.text", secname) == 0)
+			maps[ARM_SEC_DEVEXIT].unw_sec = s;
 		else if (strcmp(".init.text", secname) == 0)
 			maps[ARM_SEC_INIT].sec_text = s;
 		else if (strcmp(".devinit.text", secname) == 0)
 			maps[ARM_SEC_DEVINIT].sec_text = s;
 		else if (strcmp(".text", secname) == 0)
 			maps[ARM_SEC_CORE].sec_text = s;
+		else if (strcmp(".exit.text", secname) == 0)
+			maps[ARM_SEC_EXIT].sec_text = s;
+		else if (strcmp(".devexit.text", secname) == 0)
+			maps[ARM_SEC_DEVEXIT].sec_text = s;
 	}
 #endif
 	return 0;
-- 
1.6.0.4

Re: [PATCH 0/3] ARM: unwind extension

[Phil Carmody]

On 03/06/10 14:17 +0200, Carmody Phil.2 (EXT-Ixonos/Helsinki) wrote:
> 
> The first two patches are simply preparation for the third, making it
> effectively trivial, even though it's the only one with a concrete 
> change in behaviour.
> 
> The origins of this patchset are the discovery that unwind and kmemleak
> don't always cooperate well with each other - any allocation within 
> an exit or devexit function causes kmemleak to look up symbols that 
> aren't in any unwind table. This of course means that all WARN_ONs and
> BUGs will suffer the same fate.
> 
> It could certainly be said that with a typical system the linked list
> has grown too large to be practical as a container, and some improvements
> could be made in that direction in the future.

Catalin, 

Have you had a chance to look at these yet? The linked-list efficiency
issue I mention in the final paragraph above is a no-brainer; I have a 
1-line tweak that improves the real-world efficiency so much that on 
average there are only 2 linked list operations rather than (on a 50+
module system) 70. However, that patch is orthogonal to the above set, 
so I'll not mix the two.

Phil

Re: [PATCH 1/3] ARM: module - simplify code with temporaries

[Catalin Marinas]

On Thu, 2010-06-03 at 13:17 +0100, Phil Carmody wrote:
> Less to read.
> 
> Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

-- 
Catalin

Re: [PATCH 2/3] ARM: module - simplify unwind table handling

[Catalin Marinas]

On Thu, 2010-06-03 at 13:17 +0100, Phil Carmody wrote:
> The various sections are all dealt with similarly, so factor out
> that common behaviour.
> 
> Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

-- 
Catalin

Re: [PATCH 3/3] ARM: module - additional unwind tables for exit/devexit sections

[Catalin Marinas]

On Thu, 2010-06-03 at 13:17 +0100, Phil Carmody wrote:
> Without these, exit functions cannot be stack-traced, so to speak.
> This implies that module unloads that perform allocations (don't
> laugh) will cause noisy warnings on the console when kmemleak is
> enabled, as it presumes that all code's call chains are traceable.
> Similarly, BUGs and WARN_ONs will give additional console spam.
> 
> Signed-off-by: Phil Carmody <ext-phil.2.carmody@nokia.com>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

-- 
Catalin

Re: [PATCH 2/3] ARM: module - simplify unwind table handling

[Russell King - ARM Linux]

On Thu, Jun 03, 2010 at 03:17:21PM +0300, Phil Carmody wrote:
>  #ifdef CONFIG_ARM_UNWIND
...
> +#else
> +struct mod_arch_specific {
> +}
> +#endif

I assume no one tried building these with UNWIND disabled?  linux-next
reports most ARM builds are broken by this.

Dropped all three patches out of my git tree until it can be fixed.

Re: [PATCH 2/3] ARM: module - simplify unwind table handling

[Catalin Marinas]

On Thu, 2010-06-24 at 10:05 +0100, Russell King - ARM Linux wrote:
> On Thu, Jun 03, 2010 at 03:17:21PM +0300, Phil Carmody wrote:
> >  #ifdef CONFIG_ARM_UNWIND
> ...
> > +#else
> > +struct mod_arch_specific {
> > +}
> > +#endif
> 
> I assume no one tried building these with UNWIND disabled?  linux-next
> reports most ARM builds are broken by this.
> 
> Dropped all three patches out of my git tree until it can be fixed.

It looks like a patch for fixing the build was posted on the 16th of
June, though not sure whether it went to your patch system or not.

-- 
Catalin