Re: [PATCH] tree-profile: Don't instrument an IFUNC resolver nor its callees

["H.J. Lu" <hjl.tools@gmail.com>]

On Thu, Feb 29, 2024 at 7:06 AM Jan Hubicka <hubicka@ucw.cz> wrote:
>
> > > I am worried about scenario where ifunc selector calls function foo
> > > defined locally and foo is also used from other places possibly in hot
> > > loops.
> > > >
> > > > > So it is not really reliable fix (though I guess it will work a lot of
> > > > > common code).  I wonder what would be alternatives.  In GCC generated
> > > > > profling code we use TLS only for undirect call profiling (so there is
> > > > > no need to turn off rest of profiling).  I wonder if there is any chance
> > > > > to not make it seffault when it is done before TLS is set up?
> > > >
> > > > IFUNC selector should make minimum external calls, none is preferred.
> > >
> > > Edge porfiling only inserts (atomic) 64bit increments of counters.
> > > If target supports these operations inline, no external calls will be
> > > done.
> > >
> > > Indirect call profiling inserts the problematic TLS variable (to track
> > > caller-callee pairs). Value profiling also inserts various additional
> > > external calls to counters.
> > >
> > > I am perfectly fine with disabling instrumentation for ifunc selectors
> > > and functions only reachable from them, but I am worried about calles
> > > used also from non-ifunc path.
> >
> > Programmers need to understand not to do it.
>
> It would help to have this documented. Should we warn when ifunc
> resolver calls external function, comdat of function reachable from
> non-ifunc code?

That will be nice.

> >
> > > For example selector implemented in C++ may do some string handling to
> > > match CPU name and propagation will disable profiling for std::string
> >
> > On x86, they should use CPUID, not string functions.
> >
> > > member functions (which may not be effective if comdat section is
> > > prevailed from other translation unit).
> >
> > String functions may lead to external function calls which is dangerous.
> >
> > > > Any external calls may lead to issues at run-time.  It is a very bad idea
> > > > to profile IFUNC selector via external function call.
> > >
> > > Looking at https://sourceware.org/glibc/wiki/GNU_IFUNC
> > > there are other limitations on ifunc except for profiling, such as
> > > -fstack-protector-all.  So perhaps your propagation can be used to
> > > disable those features as well.
> >
> > So, it may not be tree-profile specific.  Where should these 2 bits
> > be added?
>
> If we want to disable other transforms too, then I think having a bit in
> cgraph_node for reachability from ifunc resolver makes sense.
> I would still do the cycle detection using on-side hash_map to avoid
> polution of the global datastructure.
>

I will see what I can do.

Thanks.

> Thanks,
> Honza
> >
> > > "Unfortunately there are actually a lot of restrictions placed on IFUNC
> > > usage which aren't entirely clear and the documentation needs to be
> > > updated." makes me wonder what other transformations are potentially
> > > dangerous.
> > >
> > > Honza
> >
> >
> > --
> > H.J.



-- 
H.J.