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


thank you for your help, Sri. I fixed help string and deleted warning.

Ian, could you please take a look at the attached patch?

Alexander


2013/2/6 Sriraman Tallam <tmsriram@google.com>:
> 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

Attachment: enable_sorting_sections_by_name_6.patch
Description: Binary data


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