On Thu, Mar 25, 2021 at 3:32 PM Jakub Jelinek <ja...@redhat.com> wrote:
>
> On Thu, Mar 25, 2021 at 03:24:38PM +0100, Richard Biener wrote:
> > Well, but in all of those cases the program was invalid (and is diagnosed).
> > So it simply means "always inline". That we abuse the always-inline
> > (error) diagnostic to tell users about possible problems (and in HJs case
> > and the fortify case reject valid programs) is IMHO a bug.
>
> No, we don't diagnose the cases where we chose to implement the intrinsics
> e.g. using generic vectors anymore with H.J.'s patch.
> It is fine to use generic vectors directly, but invalid and non-portable
> to call e.g. AVX2 intrinsics from SSE2 functions even if they are
> implemented using generic vectors.
>
> > We perform always-inline inlining early even with -fno-early-inlining. But
> > I
> > think we don't reliably diagnose indirect calls or address-taking of
> > always-inline
> > functions and will emit them out-of-line if they end up "used". That's a
> > bug
> > I think.
> >
> > static inline __attribute__((always_inline)) void f () {}
> > void (*p)() = f;
> >
> > is not diagnosed and 'f' is emitted out of line.
>
> I think we can't do such a change, too much code in the wild relies on
> always_inline not diagnosing the indirect call. Including all of glibc
> headers.
> Doing
> int (*p) (const char *, int, ...) = open;
> is perfectly valid C and we must not diagnose that, even when for
> fortification glibc has an always_inline wrapper around it.
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.
> Yes, it is unfortunate that we in the past called the attribute
> always_inline when we really didn't mean (or perhaps some of us meant but
> didn't implement) always, but at this point we'd need a new attribute to
> mean diagnose non-indirect calls/taking address of except in direct calls as
> errors. And IMHO we need a different attribute to mean inline despite
> target/optimization option mismatches.
> That way users can choose what exactly they want on a case by case basis.
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?
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?
Richard.
> Jakub
>
