Provide a means of aligning the __mcount_loc section.
Signed-off-by: Matt Fleming <[email protected]>
---
scripts/recordmcount.pl | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
index 6b9fe3e..2d0bfa1 100755
--- a/scripts/recordmcount.pl
+++ b/scripts/recordmcount.pl
@@ -134,6 +134,7 @@ my $section_regex; # Find the start of a section
my $function_regex; # Find the name of a function
# (return offset and func name)
my $mcount_regex; # Find the call site to mcount (return offset)
+my $alignment; # The .align value to use for $mcount_section
if ($arch eq "x86") {
if ($bits == 64) {
@@ -148,6 +149,7 @@ if ($arch eq "x86_64") {
$function_regex = "^([0-9a-fA-F]+)\\s+<(.*?)>:";
$mcount_regex = "^\\s*([0-9a-fA-F]+):.*\\smcount([+-]0x[0-9a-zA-Z]+)?\$";
$type = ".quad";
+ $alignment = 8;
# force flags for this arch
$ld .= " -m elf_x86_64";
@@ -160,6 +162,7 @@ if ($arch eq "x86_64") {
$function_regex = "^([0-9a-fA-F]+)\\s+<(.*?)>:";
$mcount_regex = "^\\s*([0-9a-fA-F]+):.*\\smcount\$";
$type = ".long";
+ $alignment = 4;
# force flags for this arch
$ld .= " -m elf_i386";
@@ -288,6 +291,7 @@ sub update_funcs
open(FILE, ">$mcount_s") || die "can't create $mcount_s\n";
$opened = 1;
print FILE "\t.section $mcount_section,\"a\",\@progbits\n";
+ print FILE "\t.align $alignment\n";
}
printf FILE "\t%s %s + %d\n", $type, $ref_func, $offsets[$i] - $offset;
}
--
1.5.6.4
On Fri, 2008-11-07 at 13:12 +0000, Matt Fleming wrote:
> Provide a means of aligning the __mcount_loc section.
Not that I object to the patch, but this changelog needs work.
Its wrong, it doesn't provide means, it plain does.
It doesn't tell us why.
> Signed-off-by: Matt Fleming <[email protected]>
> ---
> scripts/recordmcount.pl | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
> index 6b9fe3e..2d0bfa1 100755
> --- a/scripts/recordmcount.pl
> +++ b/scripts/recordmcount.pl
> @@ -134,6 +134,7 @@ my $section_regex; # Find the start of a section
> my $function_regex; # Find the name of a function
> # (return offset and func name)
> my $mcount_regex; # Find the call site to mcount (return offset)
> +my $alignment; # The .align value to use for $mcount_section
>
> if ($arch eq "x86") {
> if ($bits == 64) {
> @@ -148,6 +149,7 @@ if ($arch eq "x86_64") {
> $function_regex = "^([0-9a-fA-F]+)\\s+<(.*?)>:";
> $mcount_regex = "^\\s*([0-9a-fA-F]+):.*\\smcount([+-]0x[0-9a-zA-Z]+)?\$";
> $type = ".quad";
> + $alignment = 8;
>
> # force flags for this arch
> $ld .= " -m elf_x86_64";
> @@ -160,6 +162,7 @@ if ($arch eq "x86_64") {
> $function_regex = "^([0-9a-fA-F]+)\\s+<(.*?)>:";
> $mcount_regex = "^\\s*([0-9a-fA-F]+):.*\\smcount\$";
> $type = ".long";
> + $alignment = 4;
>
> # force flags for this arch
> $ld .= " -m elf_i386";
> @@ -288,6 +291,7 @@ sub update_funcs
> open(FILE, ">$mcount_s") || die "can't create $mcount_s\n";
> $opened = 1;
> print FILE "\t.section $mcount_section,\"a\",\@progbits\n";
> + print FILE "\t.align $alignment\n";
> }
> printf FILE "\t%s %s + %d\n", $type, $ref_func, $offsets[$i] - $offset;
> }
> --
> 1.5.6.4
2008/11/7 Peter Zijlstra <[email protected]>:
> On Fri, 2008-11-07 at 13:12 +0000, Matt Fleming wrote:
>> Provide a means of aligning the __mcount_loc section.
>
> Not that I object to the patch, but this changelog needs work.
>
> Its wrong, it doesn't provide means, it plain does.
> It doesn't tell us why.
>
Would you like me to resubmit the patch with a better changelog?
On Fri, 7 Nov 2008, Peter Zijlstra wrote:
> On Fri, 2008-11-07 at 13:12 +0000, Matt Fleming wrote:
> > Provide a means of aligning the __mcount_loc section.
>
> Not that I object to the patch, but this changelog needs work.
>
> Its wrong, it doesn't provide means, it plain does.
> It doesn't tell us why.
Also, please tell us what broke. Did you have a compiler that did
not align it properly? As long as the alignment is less than or equal to
the word size, it is fine. If the alignment is larger than the word size,
then there will be holes in the mcount_loc section and that will break
ftrace. Heck, an alignment of 1 should work for everyone ;-) All the
sections are packed together in an aligned space when it is pulled in by
the linker. If that is not true, then the linker script needs to be fixed.
-- Steve
On Fri, 7 Nov 2008, Matt Fleming wrote:
> 2008/11/7 Peter Zijlstra <[email protected]>:
> > On Fri, 2008-11-07 at 13:12 +0000, Matt Fleming wrote:
> >> Provide a means of aligning the __mcount_loc section.
> >
> > Not that I object to the patch, but this changelog needs work.
> >
> > Its wrong, it doesn't provide means, it plain does.
> > It doesn't tell us why.
> >
>
> Would you like me to resubmit the patch with a better changelog?
Yes, because I'm not convinced that the change is needed.
-- Steve
2008/11/7 Steven Rostedt <[email protected]>:
>
> On Fri, 7 Nov 2008, Peter Zijlstra wrote:
>
>> On Fri, 2008-11-07 at 13:12 +0000, Matt Fleming wrote:
>> > Provide a means of aligning the __mcount_loc section.
>>
>> Not that I object to the patch, but this changelog needs work.
>>
>> Its wrong, it doesn't provide means, it plain does.
>> It doesn't tell us why.
>
> Also, please tell us what broke. Did you have a compiler that did
> not align it properly? As long as the alignment is less than or equal to
> the word size, it is fine. If the alignment is larger than the word size,
> then there will be holes in the mcount_loc section and that will break
> ftrace. Heck, an alignment of 1 should work for everyone ;-) All the
> sections are packed together in an aligned space when it is pulled in by
> the linker. If that is not true, then the linker script needs to be fixed.
>
I've tried to explain the bug better in the attached patch. The issue
is not that there were gaps in __mcount_loc, rather that the addresses
in __mcount_loc were not aligned on a 4-byte boundary by the
assembler. Unaligned accesses are not supported by our architecture
and an alignment of 1-byte will likely lead to performance loss on
architectures that do support them.
>From 4161c8b2a1e5c93146fc6e9a638c018a14648c6b Mon Sep 17 00:00:00 2001
From: Matt Fleming <[email protected]>
Date: Fri, 7 Nov 2008 13:26:25 +0000
Subject: [PATCH] ftrace: Align __mcount_loc sections
Align the __mcount_loc sections so that architectures with strict
alignment requirements need not worry about performing unaligned
accesses.
This fixes an issue where I was seeing unaligned accesses, which are not
supported on our architecture (the results of an unaligned access are
undefined).
Signed-off-by: Matt Fleming <[email protected]>
---
scripts/recordmcount.pl | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
index 6b9fe3e..2d0bfa1 100755
--- a/scripts/recordmcount.pl
+++ b/scripts/recordmcount.pl
@@ -134,6 +134,7 @@ my $section_regex; # Find the start of a section
my $function_regex; # Find the name of a function
# (return offset and func name)
my $mcount_regex; # Find the call site to mcount (return offset)
+my $alignment; # The .align value to use for $mcount_section
if ($arch eq "x86") {
if ($bits == 64) {
@@ -148,6 +149,7 @@ if ($arch eq "x86_64") {
$function_regex = "^([0-9a-fA-F]+)\\s+<(.*?)>:";
$mcount_regex = "^\\s*([0-9a-fA-F]+):.*\\smcount([+-]0x[0-9a-zA-Z]+)?\$";
$type = ".quad";
+ $alignment = 8;
# force flags for this arch
$ld .= " -m elf_x86_64";
@@ -160,6 +162,7 @@ if ($arch eq "x86_64") {
$function_regex = "^([0-9a-fA-F]+)\\s+<(.*?)>:";
$mcount_regex = "^\\s*([0-9a-fA-F]+):.*\\smcount\$";
$type = ".long";
+ $alignment = 4;
# force flags for this arch
$ld .= " -m elf_i386";
@@ -288,6 +291,7 @@ sub update_funcs
open(FILE, ">$mcount_s") || die "can't create $mcount_s\n";
$opened = 1;
print FILE "\t.section $mcount_section,\"a\",\@progbits\n";
+ print FILE "\t.align $alignment\n";
}
printf FILE "\t%s %s + %d\n", $type, $ref_func, $offsets[$i] - $offset;
}
--
1.5.6.4
On Fri, 7 Nov 2008, Matt Fleming wrote:
>
> I've tried to explain the bug better in the attached patch. The issue
> is not that there were gaps in __mcount_loc, rather that the addresses
> in __mcount_loc were not aligned on a 4-byte boundary by the
> assembler. Unaligned accesses are not supported by our architecture
> and an alignment of 1-byte will likely lead to performance loss on
> architectures that do support them.
This is what I was looking for ;-)
Yes, I was just telling Peter on IRC, that this might just be an
architecture request. Which I find as a legitimate reason. But because
there was no mention of that in the change log, I would have to NACK it.
Ingo,
I'll pull this in my tree and test it first. Then I'll pass it off to you.
-- Steve