Re: [google] Patch to enable efficient function level instrumentation (issue 5416043)

2011-12-27 Thread Xinliang David Li 
Ok for google branches when tests are done. Update ChangeLog file properly.

David

On Tue, Dec 20, 2011 at 1:55 PM, Harshit Chopra  wrote:
>
>
> On Fri, Dec 16, 2011 at 3:10 PM,  wrote:
>>
>> Ok, Cary's explanation makes sense. Please update the comments to make
>> it clearer.
>>
>>
>>
>> http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c
>> File gcc/config/i386/i386.c (right):
>>
>>
>> http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10927
>> gcc/config/i386/i386.c:10927: +     is later renamed to ''
>> by ix86_elf_asm_named_section().  */
>> Probably better to change the comment to something like -- we emit a
>> unique section name for the back pointer section. This is needed because
>> otherwise the 'get_section' call may return an existing non-comdat
>> section with the same name, leading to references from non-comdat
>> section to comdat functions.
>
>
> Updated comment as suggested.
>
>>
>>
>> http://codereview.appspot.com/5416043/
>
>

Re: [google] Patch to enable efficient function level instrumentation (issue 5416043)

2011-12-16 Thread davidxl 

Ok, Cary's explanation makes sense. Please update the comments to make
it clearer.


http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c
File gcc/config/i386/i386.c (right):

http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10927
gcc/config/i386/i386.c:10927: + is later renamed to ''
by ix86_elf_asm_named_section().  */
Probably better to change the comment to something like -- we emit a
unique section name for the back pointer section. This is needed because
otherwise the 'get_section' call may return an existing non-comdat
section with the same name, leading to references from non-comdat
section to comdat functions.

http://codereview.appspot.com/5416043/

Re: [google] Patch to enable efficient function level instrumentation (issue 5416043)

2011-12-16 Thread ccoutant 


http://codereview.appspot.com/5416043/diff/12001/gcc/config/i386/i386.c
File gcc/config/i386/i386.c (left):

http://codereview.appspot.com/5416043/diff/12001/gcc/config/i386/i386.c#oldcode10928
gcc/config/i386/i386.c:10928: if (current_function_decl != NULL_TREE &&
If I understand correctly, this does arrange for the backpointer section
to go into a comdat section with the same group key as the function. We
have to give the section a unique name within GCC, however, in order to
keep it from combining with the non-COMDAT section that's being used for
the non-COMDAT functions. Later, in ix86_elf_asm_named_section, we can
strip off the suffix so that it gets the right name in the ELF file.

http://codereview.appspot.com/5416043/

Re: [google] Patch to enable efficient function level instrumentation (issue 5416043)

2011-12-16 Thread davidxl 


http://codereview.appspot.com/5416043/diff/12001/gcc/config/i386/i386.c
File gcc/config/i386/i386.c (left):

http://codereview.appspot.com/5416043/diff/12001/gcc/config/i386/i386.c#oldcode10928
gcc/config/i386/i386.c:10928: if (current_function_decl != NULL_TREE &&
I am not sure how the hack you have here makes the linker warning go
away (and besides the section name suffix will be stripped right after
it is set when switch_section is called).  The right solution might be
to set the comdat group of the label address section to be the same as
the group of the comdat function.

Cary, what is your opinion?

http://codereview.appspot.com/5416043/

Re: [google] Patch to enable efficient function level instrumentation (issue 5416043)

2011-12-02 Thread davidxl 


http://codereview.appspot.com/5416043/diff/6001/gcc/config/i386/i386.c
File gcc/config/i386/i386.c (right):

http://codereview.appspot.com/5416043/diff/6001/gcc/config/i386/i386.c#newcode10881
gcc/config/i386/i386.c:10881: +   '_function_patch_epilogue'. The
backpointer section can be used to navigate
Is it strictly necessary? If most of the functions are instrumented, can
the runtime instrumenter just recognize functions need patching via
pattern matching -- as least for function prologue?

http://codereview.appspot.com/5416043/

Re: [google] Patch to enable efficient function level instrumentation (issue 5416043)

2011-12-02 Thread davidxl 

Have you uploaded the revised patch?

David


http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c
File gcc/config/i386/i386.c (right):

http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10927
gcc/config/i386/i386.c:10927: + is later renamed to ''
by ix86_elf_asm_named_section().  */
On 2011/12/02 01:57:17, harshit wrote:

On 2011/11/28 22:16:27, davidxl wrote:
> Explain more on the comdat handling.



I have limited knowledge about comdat sections, so can't give a

detailed

explanation on why the assembler emits an error.


What does the assembler error look like? An example file would be
helpful.

http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10970
gcc/config/i386/i386.c:10970: +
sizeof("_function_patch_prologue") - 1;
Define a macro for the prologue and epilogue name.

http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10974
gcc/config/i386/i386.c:10974: +
section_name_length) == 0)
The two section name length happen to be the same, but it is not good to
share the same value here. sizeof (..) will be evaluated at compile
time.

http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.opt
File gcc/config/i386/i386.opt (right):

http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.opt#newcode570
gcc/config/i386/i386.opt:570: Minimum number of instructions in the
function without loop before the function is qualified for patching for
instrumentation (for use with -mpatch-functions-for-instrumentation)
in params.def

On 2011/12/02 01:57:17, harshit wrote:

On 2011/11/28 22:16:27, davidxl wrote:
> It may be better to define PARAM for it.



Param for this option? How to do it in options file? I looked at the

docs, but

didn't find anything on it.


http://codereview.appspot.com/5416043/

Re: [google] Patch to enable efficient function level instrumentation (issue 5416043)

2011-11-28 Thread davidxl 

Please also explain the need for backpointer section.

David


http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c
File gcc/config/i386/i386.c (right):

http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10801
gcc/config/i386/i386.c:10801: +static bool
Add an empty line between comment and function.

http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10811
gcc/config/i386/i386.c:10811: +  if (!patch_functions_ignore_loops)
What exactly does this option try to do here?

http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10822
gcc/config/i386/i386.c:10822: +return true;
should it have a an else return false here?

http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10841
gcc/config/i386/i386.c:10841: +++num_insns;
NON_DEBUG_INSN_P (insn)

http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10859
gcc/config/i386/i386.c:10859: +  patch_current_function_p =
check_should_patch_current_function();
Why do you need a static variable for this?

http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10874
gcc/config/i386/i386.c:10874: +   '_function_patch_epilogue'.
Explain why a backpointer section is needed.

http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10888
gcc/config/i386/i386.c:10888: +  unsigned int num_actual_nops =
num_remaining_nops - 8;
hard code of 8?

http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10897
gcc/config/i386/i386.c:10897: +return false;
Can this be guarded in the caller of this function?

http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.c#newcode10927
gcc/config/i386/i386.c:10927: + is later renamed to ''
by ix86_elf_asm_named_section().  */
Explain more on the comdat handling.

http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.opt
File gcc/config/i386/i386.opt (right):

http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.opt#newcode570
gcc/config/i386/i386.opt:570: Minimum number of instructions in the
function without loop before the function is qualified for patching for
instrumentation (for use with -mpatch-functions-for-instrumentation)
It may be better to define PARAM for it.

http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.opt#newcode574
gcc/config/i386/i386.opt:574: Ignore loops when deciding whether to
patch a function for instrumentation (for use with
-mpatch-functions-for-instrumentation).
What is the motivation for this option?

http://codereview.appspot.com/5416043/diff/1/gcc/config/i386/i386.opt#newcode578
gcc/config/i386/i386.opt:578: Treat 'main' as any other function and
only patch it if it meets the criteria for loops and minimum number of
instructions (for use with -mpatch-functions-for-instrumentation).
What is the motivation for this option?

http://codereview.appspot.com/5416043/

Re: [google] Patch to enable efficient function level instrumentation

2011-11-28 Thread Harshit Chopra 
Ping!

--
Harshit