[PATCH] arm64/debug: don't duplicate filenames.

[PATCH] arm64/debug: don't duplicate filenames.

[Jamie Iles]

Rather than explicitly pushing the filename into .rodata.str, use a
compiler generated string literal and use the address of that as an
input constraint to the inline assembly.  This allows the compiler to
emit only one version of the string without relying on the linker to
deduplicate.

Cc: Dave P Martin <Dave.Martin@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Signed-off-by: Jamie Iles <jamie.iles@oracle.com>
---
 arch/arm64/include/asm/bug.h | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/include/asm/bug.h b/arch/arm64/include/asm/bug.h
index a02a57186f56..a7b05625ef63 100644
--- a/arch/arm64/include/asm/bug.h
+++ b/arch/arm64/include/asm/bug.h
@@ -23,12 +23,8 @@
 #ifdef CONFIG_DEBUG_BUGVERBOSE
 #define _BUGVERBOSE_LOCATION(file, line) __BUGVERBOSE_LOCATION(file, line)
 #define __BUGVERBOSE_LOCATION(file, line)				\
-		".pushsection .rodata.str,\"aMS\", at progbits,1\n"	\
-	"2:	.string \"" file "\"\n\t"				\
-		".popsection\n\t"					\
-									\
-		".long 2b - 0b\n\t"					\
-		".short " #line "\n\t"
+		".long %[file] - 0b\n\t"				\
+		".short %[line]\n\t"
 #else
 #define _BUGVERBOSE_LOCATION(file, line)
 #endif
@@ -50,7 +46,9 @@ _BUGVERBOSE_LOCATION(__FILE__, __LINE__)		\
 #define __BUG_FLAGS(flags)				\
 	asm volatile (					\
 		__BUG_ENTRY(flags)			\
-		"brk %[imm]" :: [imm] "i" (BUG_BRK_IMM)	\
+		"brk %[imm]" :: [imm] "i" (BUG_BRK_IMM),\
+			        [line] "i" (__LINE__),  \
+			        [file] "i" (__FILE__)   \
 	);
 
 
-- 
2.15.0

[PATCH] arm64/debug: don't duplicate filenames.

[Dave Martin]

On Thu, Nov 09, 2017 at 12:14:40PM +0000, Jamie Iles wrote:
> Rather than explicitly pushing the filename into .rodata.str, use a
> compiler generated string literal and use the address of that as an
> input constraint to the inline assembly.  This allows the compiler to
> emit only one version of the string without relying on the linker to
> deduplicate.

But if the linker does deduplicate, why does it matter?

Or is the linker not removing duplicates that some from the same source
file?

Cheers
---Dave

> 
> Cc: Dave P Martin <Dave.Martin@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Jamie Iles <jamie.iles@oracle.com>
> ---
>  arch/arm64/include/asm/bug.h | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/bug.h b/arch/arm64/include/asm/bug.h
> index a02a57186f56..a7b05625ef63 100644
> --- a/arch/arm64/include/asm/bug.h
> +++ b/arch/arm64/include/asm/bug.h
> @@ -23,12 +23,8 @@
>  #ifdef CONFIG_DEBUG_BUGVERBOSE
>  #define _BUGVERBOSE_LOCATION(file, line) __BUGVERBOSE_LOCATION(file, line)
>  #define __BUGVERBOSE_LOCATION(file, line)				\
> -		".pushsection .rodata.str,\"aMS\", at progbits,1\n"	\
> -	"2:	.string \"" file "\"\n\t"				\
> -		".popsection\n\t"					\
> -									\
> -		".long 2b - 0b\n\t"					\
> -		".short " #line "\n\t"
> +		".long %[file] - 0b\n\t"				\
> +		".short %[line]\n\t"
>  #else
>  #define _BUGVERBOSE_LOCATION(file, line)
>  #endif
> @@ -50,7 +46,9 @@ _BUGVERBOSE_LOCATION(__FILE__, __LINE__)		\
>  #define __BUG_FLAGS(flags)				\
>  	asm volatile (					\
>  		__BUG_ENTRY(flags)			\
> -		"brk %[imm]" :: [imm] "i" (BUG_BRK_IMM)	\
> +		"brk %[imm]" :: [imm] "i" (BUG_BRK_IMM),\
> +			        [line] "i" (__LINE__),  \
> +			        [file] "i" (__FILE__)   \
>  	);
>  
>  
> -- 
> 2.15.0
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] arm64/debug: don't duplicate filenames.

[Jamie Iles]

Hi Dave,

On Thu, Nov 09, 2017 at 12:35:04PM +0000, Dave Martin wrote:
> On Thu, Nov 09, 2017 at 12:14:40PM +0000, Jamie Iles wrote:
> > Rather than explicitly pushing the filename into .rodata.str, use a
> > compiler generated string literal and use the address of that as an
> > input constraint to the inline assembly.  This allows the compiler to
> > emit only one version of the string without relying on the linker to
> > deduplicate.
> 
> But if the linker does deduplicate, why does it matter?

It's not broken, but at the moment we have multiple copies in the object 
file then we're relying on the linker.  If we use the same pattern as 
X86, we can let GCC assign sections+attributes for us and only get one 
copy in the object file.

> Or is the linker not removing duplicates that some from the same source
> file?

In my testing, yes, the linker is merging them.

Jamie

[PATCH] arm64/debug: don't duplicate filenames.

[Dave Martin]

On Thu, Nov 09, 2017 at 01:08:51PM +0000, Jamie Iles wrote:
> Hi Dave,
> 
> On Thu, Nov 09, 2017 at 12:35:04PM +0000, Dave Martin wrote:
> > On Thu, Nov 09, 2017 at 12:14:40PM +0000, Jamie Iles wrote:
> > > Rather than explicitly pushing the filename into .rodata.str, use a
> > > compiler generated string literal and use the address of that as an
> > > input constraint to the inline assembly.  This allows the compiler to
> > > emit only one version of the string without relying on the linker to
> > > deduplicate.
> > 
> > But if the linker does deduplicate, why does it matter?
> 
> It's not broken, but at the moment we have multiple copies in the object 
> file then we're relying on the linker.  If we use the same pattern as 
> X86, we can let GCC assign sections+attributes for us and only get one 
> copy in the object file.
> 
> > Or is the linker not removing duplicates that some from the same source
> > file?
> 
> In my testing, yes, the linker is merging them.

So, playing devil's advocate here, what is this fixing?

It doesn't really matter where in the toolchain the duplicates get
removed, just that it happens somewhere...

It looks like ld -r doesn't do the deduplication, so there may be a fair
amount of duplicates before the final linux of vmlinux -- which I guess
could be an issue if BUG() is used in macros that get expanded all over
the place.

Cheers
---Dave

[PATCH] arm64/debug: don't duplicate filenames.

[Jamie Iles]

On Thu, Nov 09, 2017 at 02:31:22PM +0000, Dave Martin wrote:
> On Thu, Nov 09, 2017 at 01:08:51PM +0000, Jamie Iles wrote:
> > Hi Dave,
> > 
> > On Thu, Nov 09, 2017 at 12:35:04PM +0000, Dave Martin wrote:
> > > On Thu, Nov 09, 2017 at 12:14:40PM +0000, Jamie Iles wrote:
> > > > Rather than explicitly pushing the filename into .rodata.str, use a
> > > > compiler generated string literal and use the address of that as an
> > > > input constraint to the inline assembly.  This allows the compiler to
> > > > emit only one version of the string without relying on the linker to
> > > > deduplicate.
> > > 
> > > But if the linker does deduplicate, why does it matter?
> > 
> > It's not broken, but at the moment we have multiple copies in the object 
> > file then we're relying on the linker.  If we use the same pattern as 
> > X86, we can let GCC assign sections+attributes for us and only get one 
> > copy in the object file.
> > 
> > > Or is the linker not removing duplicates that some from the same source
> > > file?
> > 
> > In my testing, yes, the linker is merging them.
> 
> So, playing devil's advocate here, what is this fixing?

It's caused some confusion looking at object files, it's certainly not a 
critical bug!

> It doesn't really matter where in the toolchain the duplicates get
> removed, just that it happens somewhere...
> 
> It looks like ld -r doesn't do the deduplication, so there may be a fair
> amount of duplicates before the final linux of vmlinux -- which I guess
> could be an issue if BUG() is used in macros that get expanded all over
> the place.

Ah, interesting.  Testing with a loadable module, this means that there 
are duplicates in modules.  With mainline:

  strings ./fs/ext4/ext4.ko | grep -c "inode\.c"
  61

and with my patch:

  strings ./fs/ext4/ext4.ko | grep -c "inode\.c"
  4

Thanks,

Jamie

[PATCH] arm64/debug: don't duplicate filenames.

[Dave Martin]

On Thu, Nov 09, 2017 at 02:41:36PM +0000, Jamie Iles wrote:
> On Thu, Nov 09, 2017 at 02:31:22PM +0000, Dave Martin wrote:
> > On Thu, Nov 09, 2017 at 01:08:51PM +0000, Jamie Iles wrote:
> > > Hi Dave,
> > > 
> > > On Thu, Nov 09, 2017 at 12:35:04PM +0000, Dave Martin wrote:

[...]

> > > > Or is the linker not removing duplicates that some from the same source
> > > > file?
> > > 
> > > In my testing, yes, the linker is merging them.
> > 
> > So, playing devil's advocate here, what is this fixing?
> 
> It's caused some confusion looking at object files, it's certainly not a 
> critical bug!
> 
> > It doesn't really matter where in the toolchain the duplicates get
> > removed, just that it happens somewhere...
> > 
> > It looks like ld -r doesn't do the deduplication, so there may be a fair
> > amount of duplicates before the final linux of vmlinux -- which I guess
> > could be an issue if BUG() is used in macros that get expanded all over
> > the place.
> 
> Ah, interesting.  Testing with a loadable module, this means that there 
> are duplicates in modules.  With mainline:
> 
>   strings ./fs/ext4/ext4.ko | grep -c "inode\.c"
>   61
> 
> and with my patch:
> 
>   strings ./fs/ext4/ext4.ko | grep -c "inode\.c"
>   4
> 
> Thanks,
> 
> Jamie

That's a more convincing argument...  I had forgotten that modules
weren't fully linked.

So this does look like a good idea.  I have some comments, but I'll
reply to the original patch (where there is code).

Cheers
---Dave

[PATCH] arm64/debug: don't duplicate filenames.

[Dave Martin]

On Thu, Nov 09, 2017 at 12:14:40PM +0000, Jamie Iles wrote:
> Rather than explicitly pushing the filename into .rodata.str, use a
> compiler generated string literal and use the address of that as an
> input constraint to the inline assembly.  This allows the compiler to
> emit only one version of the string without relying on the linker to
> deduplicate.
> 
> Cc: Dave P Martin <Dave.Martin@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Jamie Iles <jamie.iles@oracle.com>
> ---
>  arch/arm64/include/asm/bug.h | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/bug.h b/arch/arm64/include/asm/bug.h
> index a02a57186f56..a7b05625ef63 100644
> --- a/arch/arm64/include/asm/bug.h
> +++ b/arch/arm64/include/asm/bug.h
> @@ -23,12 +23,8 @@
>  #ifdef CONFIG_DEBUG_BUGVERBOSE
>  #define _BUGVERBOSE_LOCATION(file, line) __BUGVERBOSE_LOCATION(file, line)
>  #define __BUGVERBOSE_LOCATION(file, line)				\
> -		".pushsection .rodata.str,\"aMS\", at progbits,1\n"	\
> -	"2:	.string \"" file "\"\n\t"				\
> -		".popsection\n\t"					\
> -									\
> -		".long 2b - 0b\n\t"					\
> -		".short " #line "\n\t"
> +		".long %[file] - 0b\n\t"				\
> +		".short %[line]\n\t"
>  #else
>  #define _BUGVERBOSE_LOCATION(file, line)
>  #endif
> @@ -50,7 +46,9 @@ _BUGVERBOSE_LOCATION(__FILE__, __LINE__)		\
>  #define __BUG_FLAGS(flags)				\
>  	asm volatile (					\
>  		__BUG_ENTRY(flags)			\
> -		"brk %[imm]" :: [imm] "i" (BUG_BRK_IMM)	\
> +		"brk %[imm]" :: [imm] "i" (BUG_BRK_IMM),\
> +			        [line] "i" (__LINE__),  \
> +			        [file] "i" (__FILE__)   \

Please remove the extra level of macro nesting.  It's only there to
ensure that __LINE__ is fully expanded before stringifying it with
#line.  Your version doesn't rely on stringification, so this shouldn't
be needed any more.

Also, I'm not 100% sure that "i" is the right constraint to use, but
I can't come up with a better answer.


You should check to see whether anything else in the kernel is using a
similar technique.

A quick grep for 'section.*MS' suggests that arm, arm64, s390 and
are all doing something similar.  It would be good to fix all of them
if they're affected.

firmware/Makefile also has an instance, though it may not matter as
much.  I've not spent the time to figure out what it's doing.

Cheers
---Dave