Re: [gold] enable sorting of text sections with the same prefix

[Sriraman Tallam <tmsriram at google dot com>]

@@ -3527,8 +3528,44 @@
Output_section::Input_section_sort_section_name_special_ordering_compare
  return o1 < o2;
     }

+  if (strcmp(parameters->options().sort_section(), "name") == 0)
+    {
+      // Within each prefix we sort by name.
+      int compare = s1.section_name().compare(s2.section_name());
+      if (compare != 0)
+ return compare < 0;
+    }
+

Why do you still need this? Maybe you forgot to remove this.

 // This updates the section order index of input sections according to the
@@ -3600,8 +3637,14 @@ Output_section::sort_attached_input_sections()
         std::sort(sort_list.begin(), sort_list.end(),
           Input_section_sort_init_fini_compare());
       else if (strcmp(this->name(), ".text") == 0)
-        std::sort(sort_list.begin(), sort_list.end(),
-          Input_section_sort_section_name_special_ordering_compare());
+ {
+  if (strcmp(parameters->options().sort_section(), "name") == 0)
+    std::sort(sort_list.begin(), sort_list.end(),
+              Input_section_sort_section_name_compare());
+  else
+    std::sort(sort_list.begin(), sort_list.end(),
+              Input_section_sort_section_prefix_special_ordering_compare());
+ }


So, you are using Input_section_sort_section_name_compare only for
.text, why is that? What about other sections. You can do something
like this:

if (strcmp(parameters->options().sort_section(), "name") == 0))
  std::sort(sort_list.begin(), sort_list.end(),
              Input_section_sort_section_name_compare());
else if  (strcmp(this->name(), ".text") == 0)
  std::sort(sort_list.begin(), sort_list.end(),
              Input_section_sort_section_prefix_special_ordering_compare());

This will override the default sorting of ".text" with name sorting if
--sort-section=name is specified and will sort sections by name for
other sections like .data.

You also need test cases.


On Tue, Feb 5, 2013 at 5:30 AM, Alexander Ivchenko <aivchenk@gmail.com> wrote:
> You are right: strcmp in a sorting function is not a good idea..
> I did what you proposed in the first paragraph. Could you please take a look
>
> thanks,
> Alexander
>
> 2013/2/4 Sriraman Tallam <tmsriram@google.com>:
>> @@ -3527,8 +3528,16 @@
>> Output_section::Input_section_sort_section_name_special_ordering_compare
>>   return o1 < o2;
>>      }
>>
>> +  if (strcmp(parameters->options().sort_section(), "name") == 0)
>> +    {
>> +      // Within each prefix we sort by name.
>> +      int compare = s1.section_name().compare(s2.section_name());
>> +      if (compare != 0)
>> + return compare < 0;
>> +    }
>> +
>>
>> I think you can create a new compare function for this. You can rename
>> the existing function to something like
>> "Input_section_sort_section_prefix_special_ordering_compare" and make
>> a new "Input_section_sort_section_name_compare". Sorting by section
>> name also ensures the grouping of functions with prefixes like
>> ".text.hot", only the entire .text.hot could land up somewhere in the
>> middle of all functions.
>>
>> However, if you do want both special ordering and sorting by section
>> name, I think you should move the strcmp out of the compare function
>> as the compare function is called many times. A simple bool function
>> somewhere in layout would be fine I think.
>>
>> Thanks
>> Sri
>>
>> On Mon, Feb 4, 2013 at 4:15 AM, Alexander Ivchenko <aivchenk@gmail.com> wrote:
>>> Hi,
>>>
>>> The attached patch implements --sort-section=name that sorts text
>>> sections by name within each
>>> prefix and also sorts .data and .sdata sections by name (as BFD does).
>>> That is enough for closing
>>> that http://sourceware.org/bugzilla/show_bug.cgi?id=14948
>>>
>>> thank you,
>>> Alexander
>>>
>>> 2013/1/29 Sriraman Tallam <tmsriram@google.com>:
>>>> On Tue, Jan 29, 2013 at 9:55 AM, Alexander Ivchenko <aivchenk@gmail.com> wrote:
>>>>> Hello Sri,
>>>>>
>>>>> thank you for your input!
>>>>> Please, look at
>>>>> http://sourceware.org/bugzilla/show_bug.cgi?id=14948
>>>>
>>>> So, why not do this guarded by --sort-section=name just like BFD ld?
>>>> That way, it does not affect the default. Just my two cents.
>>>>
>>>> Thanks
>>>> Sri
>>>>
>>>>>
>>>>> thank you,
>>>>> Alexander
>>>>>
>>>>> 2013/1/29 Sriraman Tallam <tmsriram@google.com>:
>>>>>> Hi,
>>>>>>
>>>>>> On Tue, Jan 29, 2013 at 7:28 AM, Alexander Ivchenko <aivchenk@gmail.com> wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>>    This patch to gold: http://sourceware.org/ml/binutils/2013-01/msg00335.html
>>>>>>> disabled sorting of text sections with the same prefix like
>>>>>>> .text.hot0001, .text.hot0002
>>>>>>> which is very desirable.
>>>>>>
>>>>>> Sorting by section names in general for text sections turns out to be
>>>>>> a bad idea as we have seen many performance issues. That is the reason
>>>>>> why the patch you reference was created.
>>>>>>
>>>>>> The reason why some text sections are grouped to begin with is to
>>>>>> mimic the default GNU ld behaviour. I dont think GNU ld  groups these
>>>>>> two sections.  AFAIK, ".text.hot." is the prefix for hot text
>>>>>> sections. How were these two sections you mention created?Which
>>>>>> compiler is generating these two sections? Or, did you explicitly use
>>>>>> a section attribute?
>>>>>>
>>>>>> Thanks
>>>>>> Sri
>>>>>>
>>>>>> The attached patch fix this.
>>>>>>>
>>>>>>> OK for trunk?
>>>>>>>
>>>>>>> thank you,
>>>>>>> Alexander