This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On Wed, Feb 6, 2013 at 6:18 AM, Alexander Ivchenko <aivchenk@gmail.com> wrote:
> 2013/2/5 Sriraman Tallam <tmsriram@google.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.
>
> Oops, thanks.
>
>>  // 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.
>
> Done.
>
>> You also need test cases.
>
> The test case is added.
>
> I did not include sorting of .sdata and .sbss sections for compatibility with
> GNU ld (I don't know why they are not sorted by it...). Is this patch ok?

It looks fine to me, some minor comments


+  // Let's warn if the user wants text sections to be sorted,
+  // but at the same time --no-text-reorder is given.
+  if (strcmp(this->sort_section(), "name") == 0
+      && !this->text_reorder())
+    gold_warning(_("ignoring --sort-section=name since
--no-text-reorder is specified"));
+

You are not completely ignoring --sort-section=name here since you
will sort .data and .bss. Maybe change the warning or get rid of it
and specify  in the option help message that --no-text-reorder will
override --sort-section=name for .text

+  DEFINE_enum(sort_section, options::TWO_DASHES, '\0', "none",
+      N_("Sort text sections by name"),
+      N_("[none,name]"),
+      {"none", "name"});
+

You have to change the help string since you are also sorting .bss and .data

Otherwise it looks good to me. Please remember that you need Ian's
approval for submission.

Thanks
Sri

>
>
> thank you,
> Alexander
>
>
>
>
>
>> 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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]