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] |
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] |