Re: [PATCH v5] add -fprolog-pad=N,M option

[Jakub Jelinek <jakub at redhat dot com>]

On Wed, Feb 08, 2017 at 12:18:05PM +0100, Torsten Duwe wrote:
> On Mon, Jan 23, 2017 at 05:25:45PM +0100, Bernd Schmidt wrote:
> > There's still a a few details that need addressing, and some questions I
> > have. Also Ccing Jakub to have another pair of eyes on the name of the
> > section - I don't know if we want some sort of .gnu.something name.
> 
> So Jakub, Richard E., what's the outcome?
> 
> Is this all the feedback the "gcc developers" will provide? Bernd, don't
> get me wrong, I also consider it important to watch the coding style in a 
> project, I've seen cases were nobody cared ... not good.
> 
> But OTOH some of the code you rightfully criticised in the previous run
> just went away after some smart feedback from Richard Biener (thanks!);
> I'd rather sort out the design issues first before I polish the code.
> 
> So, can I please have a statement from a responsible maintainer (if such
> a person really exists for this area) about whether this feature would be
> acceptable at all?

First of all, GCC is in stage4 and this isn't a fix for a regression, so it
is not acceptable at this point.  Next stage1 will start in April or so.
Second thing that is questionable is what units are the arguments of the
option or attribute.
+  if (record_p)                                                                                                                                   
+    fprintf (file, "1:");                                                                                                                         
+                                                                                                                                                  
+  unsigned i;                                                                                                                                     
+  for (i = 0; i < pad_size; ++i)                                                                                                                  
+    fprintf (file, "\tnop\n");                                                                                                                    
+                                                                                                                                                  
+  if (record_p)                                                                                                                                   
+    {                                                                                                                                             
+      fprintf (file, "\t.section __prolog_pads_loc, \"a\",@progbits\n");                                                                          
+      fprintf (file, "\t.quad 1b\n");                                                                                                             
+      fprintf (file, "\t.previous\n");                                                                                                            
+    }                                                                                                                                             
suggests that it is at least by default the number of nops, whatever that
means.  The length of nop varies greatly across different architectures,
some architectures have different spelling for it (e.g.
ia64, s390{,x} use nop 0 and mmix uses swym 0), some architectures have
tons of different nops with different sizes (see e.g. x86_64/i686), for some
longer sizes it is often beneficial to emit a jump around the rest of nops
instead of just tons of nops.

Even if it is counted always in nops and only nops are emitted (not really
efficient, and I guess kernel cares about efficiency), the above is
definitely not acceptable for a generic hook implementation, it contains too
many ELF specific details as well as 64-bit target specific details etc.
For the label, you should use something like:
  static int ppad_no;
  char tmp_label[...];
  ASM_GENERATE_INTERNAL_LABEL (tmp_label, "LPPAD", ppad_no);
  ppad_no++;
  ASM_OUTPUT_LABEL (file, tmp_label);
the "a",@progbits is not generic enough, you want
  switch_to_section (get_section ("__prolog_pads_loc", 0, NULL));
or so.  .previous doesn't work on many targets, you need to actuall switch
to the previously current section.  .quad is not supported by many
assemblers, you want assemble_integer, with the appropriate size (say
derived from size of pointer), decide whether in that section everything is
aligned or unaligned, emit aligning directive if the former.

> I thought that was obvious, sorry. Instrumentation / profiling will need
> some registers, and live patching (the current use case) may allocate
> different registers for the replacement functions. You will probably want
> to replace the NOPs with a standardised code snippet etc. Conclusion: do not
> make any assumptions about callee's register utilisation and strictly stick
> to the ABI! I remember to have discussed this, maybe not here.

There are many other ways in which the ABI can check, e.g.
see how i386.c (ix86_function_regparm) can change ABI if
          cgraph_local_info *i = &target->local;
          if (i && i->local && i->can_change_signature)
So you'd e.g. need to make sure that functions you emit the padding for
can't change signature.  Bet you won't be able to handle e.g. IPA cp or many
other IPA optimizations that change signature too, while the function from
the source may still be around, perhaps nothing will call it because the
callers have been changed to call its clone with different arguments.
Or say IPA-ICF, if you want to live-patch only some function and not another
function which happens to have identical bogy in the end.

	Jakub