Gold bug in ordering text sections.

Gold bug in ordering text sections.

[Sriraman Tallam]

[-- Attachment #1: Type: text/plain, Size: 1340 bytes --]

Hi,

    This patch to gold : http://sourceware.org/ml/binutils/2012-12/msg00227.html
broke option --section-ordering-file and plugin based reordering. This
patch treats text sections as reorderable when special prefixes like
".text.hot" are seen. However, when --section-ordering-file (or
plugins) is also used then it cannot override the default sorting as
the code in output.cc to do sorting checks for
"must_sort_attached_input_sections()" first and this is always true
for .text sections with those prefixes.

   I have created the following patch to fix this. I have basically
disabled default sorting of .text sections in the presence of
--section-ordering-file or plugins. I think this makes sense because
this is also disabled in the presence of linker scripts.

  One other alternative is to unset
"must_sort_attached_input_sections" for ".text" if
section-ordering-file or plugins do reorder ".text".  This can sort
.text sections by default if --section-ordering-file does not touch
any .text sections. I think this is overkill though.

   This went unnoticed because none of the test cases generated the
special prefix for input text sections. I have modified the plugin
based test case to catch this problem. This test case will fail if the
patch to layout.cc is not present.

  Please let me know what you think.

Thanks
Sri

[-- Attachment #2: section_ordering_bug.txt --]
[-- Type: text/plain, Size: 2387 bytes --]

	* layout.cc (Layout::layout): Do not do default sorting for
	text sections when section ordering is specified.
	(make_output_section): Ditto.
	* testsuite/plugin_final_layout.cc: Name the sections for the
	functions to catch reordering issues.


Index: layout.cc
===================================================================
RCS file: /cvs/src/src/gold/layout.cc,v
retrieving revision 1.242
diff -u -u -p -r1.242 layout.cc
--- layout.cc	21 Dec 2012 06:24:31 -0000	1.242
+++ layout.cc	16 Jan 2013 22:13:38 -0000
@@ -1150,6 +1150,7 @@ Layout::layout(Sized_relobj_file<size, b
   // By default the GNU linker sorts some special text sections ahead
   // of others.  We are compatible.
   if (!this->script_options_->saw_sections_clause()
+      && !this->is_section_ordering_specified()
       && !parameters->options().relocatable()
       && Layout::special_ordering_of_input_section(name) >= 0)
     os->set_must_sort_attached_input_sections();
@@ -1646,6 +1647,7 @@ Layout::make_output_section(const char* 
   // need to know that this might happen before we attach any input
   // sections.
   if (!this->script_options_->saw_sections_clause()
+      && !this->is_section_ordering_specified()
       && !parameters->options().relocatable()
       && strcmp(name, ".text") == 0)
     os->set_may_sort_attached_input_sections();
cvs diff: Diffing po
cvs diff: Diffing testsuite
Index: testsuite/plugin_final_layout.cc
===================================================================
RCS file: /cvs/src/src/gold/testsuite/plugin_final_layout.cc,v
retrieving revision 1.1
diff -u -u -p -r1.1 plugin_final_layout.cc
--- testsuite/plugin_final_layout.cc	29 Sep 2011 23:45:57 -0000	1.1
+++ testsuite/plugin_final_layout.cc	16 Jan 2013 22:13:38 -0000
@@ -21,16 +21,22 @@
 // MA 02110-1301, USA.
 
 // The goal of this program is to verify if section ordering
-// via plugins happens correctly.
+// via plugins happens correctly.  Also, test is plugin based ordering
+// overrides default text section ordering where ".text.hot" sections
+// are grouped.  The plugin does not want foo and baz next to each other.
+// Plugin section order is foo() followed by bar() and then baz().
 
+__attribute__ ((section(".text._Z3barv")))
 void bar ()
 {
 }
 
+__attribute__ ((section(".text.hot._Z3bazv")))
 void baz ()
 {
 }
 
+__attribute__ ((section(".text.hot._Z3foov")))
 void foo ()
 {
 }

Re: Gold bug in ordering text sections.

[Ian Lance Taylor]

On Wed, Jan 16, 2013 at 2:27 PM, Sriraman Tallam <tmsriram@google.com> wrote:
>
>     This patch to gold : http://sourceware.org/ml/binutils/2012-12/msg00227.html
> broke option --section-ordering-file and plugin based reordering. This
> patch treats text sections as reorderable when special prefixes like
> ".text.hot" are seen. However, when --section-ordering-file (or
> plugins) is also used then it cannot override the default sorting as
> the code in output.cc to do sorting checks for
> "must_sort_attached_input_sections()" first and this is always true
> for .text sections with those prefixes.
>
>    I have created the following patch to fix this. I have basically
> disabled default sorting of .text sections in the presence of
> --section-ordering-file or plugins. I think this makes sense because
> this is also disabled in the presence of linker scripts.
>
>   One other alternative is to unset
> "must_sort_attached_input_sections" for ".text" if
> section-ordering-file or plugins do reorder ".text".  This can sort
> .text sections by default if --section-ordering-file does not touch
> any .text sections. I think this is overkill though.
>
>    This went unnoticed because none of the test cases generated the
> special prefix for input text sections. I have modified the plugin
> based test case to catch this problem. This test case will fail if the
> patch to layout.cc is not present.


This patch is OK.

Thanks.

Ian

Re: Gold bug in ordering text sections.

[Sriraman Tallam]

On Wed, Jan 16, 2013 at 2:33 PM, Ian Lance Taylor <iant@google.com> wrote:
> On Wed, Jan 16, 2013 at 2:27 PM, Sriraman Tallam <tmsriram@google.com> wrote:
>>
>>     This patch to gold : http://sourceware.org/ml/binutils/2012-12/msg00227.html
>> broke option --section-ordering-file and plugin based reordering. This
>> patch treats text sections as reorderable when special prefixes like
>> ".text.hot" are seen. However, when --section-ordering-file (or
>> plugins) is also used then it cannot override the default sorting as
>> the code in output.cc to do sorting checks for
>> "must_sort_attached_input_sections()" first and this is always true
>> for .text sections with those prefixes.
>>
>>    I have created the following patch to fix this. I have basically
>> disabled default sorting of .text sections in the presence of
>> --section-ordering-file or plugins. I think this makes sense because
>> this is also disabled in the presence of linker scripts.
>>
>>   One other alternative is to unset
>> "must_sort_attached_input_sections" for ".text" if
>> section-ordering-file or plugins do reorder ".text".  This can sort
>> .text sections by default if --section-ordering-file does not touch
>> any .text sections. I think this is overkill though.
>>
>>    This went unnoticed because none of the test cases generated the
>> special prefix for input text sections. I have modified the plugin
>> based test case to catch this problem. This test case will fail if the
>> patch to layout.cc is not present.
>
>
> This patch is OK.
>
> Thanks.

Thanks, this patch is now committed.

Sri

>
> Ian

Re: Gold bug in ordering text sections.

[Sriraman Tallam]

Committed this simple patch to fix a comment in testsuite/plugin_final_layout.cc

===================================================================
RCS file: /cvs/src/src/gold/testsuite/plugin_final_layout.cc,v
retrieving revision 1.2
diff -u -r1.2 plugin_final_layout.cc
--- testsuite/plugin_final_layout.cc 16 Jan 2013 22:47:14 -0000 1.2
+++ testsuite/plugin_final_layout.cc 16 Jan 2013 23:59:25 -0000
@@ -21,7 +21,7 @@
 // MA 02110-1301, USA.

 // The goal of this program is to verify if section ordering
-// via plugins happens correctly.  Also, test is plugin based ordering
+// via plugins happens correctly.  Also, test if plugin based ordering
 // overrides default text section ordering where ".text.hot" sections
 // are grouped.  The plugin does not want foo and baz next to each other.
 // Plugin section order is foo() followed by bar() and then baz().



Thanks
Sri

On Wed, Jan 16, 2013 at 2:47 PM, Sriraman Tallam <tmsriram@google.com> wrote:
> On Wed, Jan 16, 2013 at 2:33 PM, Ian Lance Taylor <iant@google.com> wrote:
>> On Wed, Jan 16, 2013 at 2:27 PM, Sriraman Tallam <tmsriram@google.com> wrote:
>>>
>>>     This patch to gold : http://sourceware.org/ml/binutils/2012-12/msg00227.html
>>> broke option --section-ordering-file and plugin based reordering. This
>>> patch treats text sections as reorderable when special prefixes like
>>> ".text.hot" are seen. However, when --section-ordering-file (or
>>> plugins) is also used then it cannot override the default sorting as
>>> the code in output.cc to do sorting checks for
>>> "must_sort_attached_input_sections()" first and this is always true
>>> for .text sections with those prefixes.
>>>
>>>    I have created the following patch to fix this. I have basically
>>> disabled default sorting of .text sections in the presence of
>>> --section-ordering-file or plugins. I think this makes sense because
>>> this is also disabled in the presence of linker scripts.
>>>
>>>   One other alternative is to unset
>>> "must_sort_attached_input_sections" for ".text" if
>>> section-ordering-file or plugins do reorder ".text".  This can sort
>>> .text sections by default if --section-ordering-file does not touch
>>> any .text sections. I think this is overkill though.
>>>
>>>    This went unnoticed because none of the test cases generated the
>>> special prefix for input text sections. I have modified the plugin
>>> based test case to catch this problem. This test case will fail if the
>>> patch to layout.cc is not present.
>>
>>
>> This patch is OK.
>>
>> Thanks.
>
> Thanks, this patch is now committed.
>
> Sri
>
>>
>> Ian