Re: [PATCH] C++ thunk section names

[Jan Hubicka <hubicka at ucw dot cz>]

Hello,
I apologize for taking so long to get into this patch.  I ad busy time (wedding
and teaching), should be back in regular schedule now.

> Sri, can you provide examples to show why putting thunks into the same
> section as the target function when function reorder is on can be bad
> ?

C++ ABI specify that they are in the same section, but I can't think of the
case where this would break.
Hmm, I suppose it is the TARGET_USE_LOCAL_THUNK_ALIAS_P code that breaks -
you end up with code in two sections where one is accessing local comdat
of the toher. I would also like to see testcase that breaks and is fixed by
your patch.  I would expect that here, by not copying the section name,
you actually make things wose.

I think we need to deal with this later; use_tunk is done long before
profiling is read and before we decide whether code is hot/cold.  I suppose
the function reordering code may need to always walk whole comdat group and
ensure that sections are same?
I.e. pick the highest profile of a function in the group, resolve unique section
on it and then copy section names?  I had verifier checking that section names
within one comdat groups are same, perhaps it was part of the reverted patch
for AIX.  I will try to get that one back in now.

Jan
> 
> Thanks,
> 
> David
> 
> On Thu, Jun 26, 2014 at 10:29 AM, Sriraman Tallam <tmsriram@google.com> wrote:
> > Hi Honza,
> >
> >    Could you review this patch when you find time?
> >
> > Thanks
> > Sri
> >
> > On Tue, Jun 17, 2014 at 10:42 AM, Sriraman Tallam <tmsriram@google.com> wrote:
> >> Ping.
> >>
> >> On Mon, Jun 9, 2014 at 3:54 PM, Sriraman Tallam <tmsriram@google.com> wrote:
> >>> Ping.
> >>>
> >>> On Mon, May 19, 2014 at 11:25 AM, Sriraman Tallam <tmsriram@google.com> wrote:
> >>>> Ping.
> >>>>
> >>>> On Thu, Apr 17, 2014 at 10:41 AM, Sriraman Tallam <tmsriram@google.com> wrote:
> >>>>> Ping.
> >>>>>
> >>>>> On Wed, Feb 5, 2014 at 4:31 PM, Sriraman Tallam <tmsriram@google.com> wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>>   I would like this patch reviewed and considered for commit when
> >>>>>> Stage 1 is active again.
> >>>>>>
> >>>>>> Patch Description:
> >>>>>>
> >>>>>> A C++ thunk's section name is set to be the same as the original function's
> >>>>>> section name for which the thunk was created in order to place the two
> >>>>>> together.  This is done in cp/method.c in function use_thunk.
> >>>>>> However, with function reordering turned on, the original function's section
> >>>>>> name can change to something like ".text.hot.<orginal>" or
> >>>>>> ".text.unlikely.<original>" in function default_function_section in varasm.c
> >>>>>> based on the node count of that function.  The thunk function's section name
> >>>>>> is not updated to be the same as the original here and also is not always
> >>>>>> correct to do it as the original function can be hotter than the thunk.
> >>>>>>
> >>>>>> I have created a patch to not name the thunk function's section to be the same
> >>>>>> as the original function when function reordering is enabled.
> >>>>>>
> >>>>>> Thanks
> >>>>>> Sri