[PATCH] x86: Skip ISA check for always_inline in system headers

[Richard Biener]

On Thu, Mar 25, 2021 at 2:39 PM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Thu, Mar 25, 2021 at 02:21:21PM +0100, Richard Biener wrote:
> > Err, but _which_ mismatches do you ignore with such new attribute?
>
> We'd need to define it.
>
> > If I have __rdtsc and compile that into a -mno-rdtsc unit/function would
> > that be OK?
>
> There is no -mno-rdtsc or -mrdtsc, rdtsc insn is always available.
> So from that POV, having the new attribute on __rdtsc is fine.
>
> The problem with H.J.'s patch is that we'll now accept a lot of code that
> should be rejected, we relied for years on the _mm* intrinsics implemented
> as always_inline and for it being rejected if users try to use it from
> incompatible callers.
> Some of them will be still rejected but with different diagnostics (because
> users can use the (unsupported) builtins directly, we don't have a guarantee
> they aren't used in code with arbitrary ISA flags), some of them will ICE
> (if we didn't catch such uses of unsupported builtins, something to be fixed
> for sure), and others that are implemented without builtins will be
> accepted, which means people will have broken code in the wild that might
> then not compile with clang or icc but will compile with gcc.
>
> always_inline simply sometimes means always inline except when it is address
> taken and not called directly and at other times ... or when there is a
> target or optimization specific option mismatch.

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.

>
> Perhaps a way to make it work most of the time would be to amend
> H.J.'s patch to do it only if the callee target options are the default
> ones.
> That would mean we keep the existing behavior for the
> #pragma GCC push_options
> #pragma GCC target("avx2")
> inline __attribute__((always_inline)) ...
> #pragma GCC pop_options
> cases and not for the
> __fortify_function int
> open (const char *__path, int __oflag, ...)
> {
> ...
> }
> cases.  But right now that wouldn't work reliably, because we add those
> push_options/target ... only if the currently selected target (e.g. from
> command line options) doesn't already include those.
> So, we would let the
> __attribute__((target ("no-sse3")))
> void bar (void) { _mm256_*(); }
> cases through.
>
> Another question is what H.J.'s patch will do for LTO with
> -fno-early-inlining.  If the always_inline functions are only inlined
> during LTO inlining, == comparisons with default target option node are just
> weird.

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.

That said, I think comparing with some "default" options isn't a good
way either.

Richard.

>
>         Jakub
>