On Thu, Mar 25, 2021 at 7:37 PM H.J. Lu <hjl.to...@gmail.com> wrote:
>
> On Thu, Mar 25, 2021 at 7:54 AM Jakub Jelinek via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > On Thu, Mar 25, 2021 at 03:40:51PM +0100, Richard Biener wrote:
> > > I think the "proper" way to do this is to have 'open' above end up
> > > refering to the out-of-line 'open' in the DSO, _not_ to emit the
> > > fortification wrapper out-of-line. But then, yes, it shouldn't
> > > be always-inline then. It should be like the former extern inline
> > > extension.
> >
> > It is extern inline __attribute__((gnu_inline, always_inline, artificial))
> > I think. But the always_inline is completely intentional there,
> > we don't want the decision whether to inline it or not being done based on
> > its size, amount of functions already inlined into the caller before,
> > whether the call is cold or hot, etc. It is a matter of security.
> > If it is taking address, we want the library routine in that case, sure.
> >
> > > But we have existing issues with [target] options differing and existing
> > > old
> > > uses of always_inline (like the fortification wrappers). Adding a new
> > > attribute
> > > will not fix those issues. Do you propose to not fix them and instead
> > > only
> > > fix the new general_regs_only always-inline function glibc wants to add?
> >
> > Yes.
> > Basically solve the problem for the fortification wrappers and rdtsc or
> > whatever other always inlines don't really require any specific
> > target/optimize options.
> >
> > > IMHO we have to fix the existing always_inline and we need a _new_
> > > attribute to get the desired diagnostics on intrinsics. Something
> > > like __attribute__((need_target("avx"))) for AVX intrinsics?
> >
> > Or, if we go this route in addition to adding
> > at least a new attributes for the "diagnose taking address without
> > direct call", we'd need probably not just that,
> > but also pragma way to specify it for a lot of functions together,
> > otherwise it would be a maintainance nightmare.
> >
>
> How can we move forward with it? I'd like to resolve it in GCC 11.
So I looked closer and we handle target attribute mismatches different
from optimization attribute mismatches (the latter are validated in
can_inline_edge_by_limits_p, the former in can_inline_edge_p).
For optimize attribute differences we're ignoring all (even semantic
differences):
/* Until GCC 4.9 we did not check the semantics-altering flags
below and inlined across optimization boundaries.
Enabling checks below breaks several packages by refusing
to inline library always_inline functions. See PR65873.
Disable the check for early inlining for now until better solution
is found. */
if (always_inline && early)
;
/* There are some options that change IL semantics which means
we cannot inline in these cases for correctness reason.
Not even for always_inline declared functions. */
else if (check_match (flag_wrapv)
...
/* gcc.dg/pr43564.c. Apply user-forced inline even at -O0. */
else if (always_inline)
;
/* When user added an attribute to the callee honor it. */
else if (lookup_attribute ("optimize", DECL_ATTRIBUTES (callee->decl))
&& opts_for_fn (caller->decl) != opts_for_fn (callee->decl))
{
e->inline_failed = CIF_OPTIMIZATION_MISMATCH;
inlinable = false;
}
so the original intent was to do things "correctly" but then as now seen
with target attribute mismatches we run into problems. Thus now we
allow all always-inlines.
I suppose diagnosing
static inline void __attribute__((target("avx"),always_inline))
foo_avx_optimized () {...}
void bar()
{
if (cpu_supports ("avx"))
foo_avx_optimized ();
}
for the missed optimization because of the always-inline
(foo_avx_optimized will inherit
the callers target flags and _not_ be avx optimized) might be nice,
but well, at least this
kind of inlining will not generate wrong code.
Thus we IMHO can do sth like
diff --git a/gcc/ipa-inline.c b/gcc/ipa-inline.c
index f15c4828958..d4d4ac366c8 100644
--- a/gcc/ipa-inline.c
+++ b/gcc/ipa-inline.c
@@ -374,9 +374,14 @@ can_inline_edge_p (struct cgraph_edge *e, bool report,
e->inline_failed = CIF_UNSPECIFIED;
inlinable = false;
}
- /* Check compatibility of target optimization options. */
- else if (!targetm.target_option.can_inline_p (caller->decl,
- callee->decl))
+ /* Check compatibility of target optimization options. Be consistent with
+ handling of early always-inlines and optimize attribute differences
+ handled in can_inline_edge_by_limits_p. */
+ else if ((!early
+ || !DECL_DISREGARD_INLINE_LIMITS (callee->decl)
+ || !lookup_attribute ("always_inline",
+ DECL_ATTRIBUTES (callee->decl)))
+ && !targetm.target_option.can_inline_p (caller->decl, callee->decl))
{
e->inline_failed = CIF_TARGET_OPTION_MISMATCH;
inlinable = false;
alternatively, if we want to give targets the opportunity to disallow some very
specific bad attribute differences, push this logic into all target
(and the default)
hooks (maybe as extra flag to the hook).
Refactoring can_inline_edge_p and can_inline_edge_by_limits_p to put the
option checks next to each other would also be good, this avoids differences
like above.
That said, IMHO always-inline should be what it says. If we want sth for some
useful diagnostics then that should be a new attribute (need_target
("...") or so).
And to fix possible wrong-code/ICE issues we'd need support for different
options on different code regions more fine grained than function level. For
optimize attributes this means all semantic changing options should reflect
themselves in the IL, for target options I have no good idea.
Richard.
> Thanks.
>
> --
> H.J.
