Re: [PATCH] C++ thunk section names

[Sriraman Tallam <tmsriram at google dot com>]

On Mon, Jul 7, 2014 at 11:48 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> 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.

Here is an example where the thunk and the original function get
placed in different sections.

class base_class_1
{
public:
  virtual void vfn () {}
};

class base_class_2
{
public:
  virtual void vfn () {}
};
void foo();
class need_thunk_class : public base_class_1, public base_class_2
{
public:
  virtual void vfn () {
    for (int i = 0; i < 100000; ++i)
      foo();
  }
};

int main (int argc, char *argv[])
{
  base_class_1 *bc1 = new need_thunk_class ();
  bc1->vfn();
  return 0;
}

int glob = 0;
__attribute__((noinline))
void foo()
{
  glob++;
}


I am making the function that needs thunk hot. Now,

$ g++ thunkex.cc  -O2 -fno-reorder-blocks-and-partition
-fprofile-generate -ffunction-sections
$ a.out
$ g++ thunkex.cc  -O2 -fno-reorder-blocks-and-partition -fprofile-use
-ffunction-sections -c
$ objdump -d thunkex.o

Disassembly of section .text.hot._ZN16need_thunk_class3vfnEv:

0000000000000000 <_ZN16need_thunk_class3vfnEv>:
   0:   53                      push   %rbx
   1:   bb a0 86 01 00          mov    $0x186a0,%ebx
   ...

Disassembly of section .text._ZN16need_thunk_class3vfnEv:

0000000000000000 <_ZThn8_N16need_thunk_class3vfnEv>:
   0:   48 83 ef 08             sub    $0x8,%rdi
   ....

When the original function gets moved to .text.hot, the thunk does
not.  It is not always the case that the thunk should either.

Thanks
Sri




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