Re: [PATCH v2 4/6] compiler-gcc.h: add asm_inline definition

Re: [PATCH v2 4/6] compiler-gcc.h: add asm_inline definition

[Rasmus Villemoes]

On 05/09/2019 02.18, Nick Desaulniers wrote:
> On Fri, Aug 30, 2019 at 4:15 PM Rasmus Villemoes
> <linux@rasmusvillemoes.dk> wrote:
>>
>> This adds an asm_inline macro which expands to "asm inline" [1] when gcc
>> is new enough (>= 9.1), and just asm for older gccs and other
>> compilers.
>>
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>> ---
>>  include/linux/compiler-gcc.h   | 4 ++++
>>  include/linux/compiler_types.h | 4 ++++
>>  2 files changed, 8 insertions(+)
>>
>> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
>> index d7ee4c6bad48..544b87b41b58 100644
>> --- a/include/linux/compiler-gcc.h
>> +++ b/include/linux/compiler-gcc.h
>> @@ -172,3 +172,7 @@
>>  #endif
>>
>>  #define __no_fgcse __attribute__((optimize("-fno-gcse")))
>> +
>> +#if GCC_VERSION >= 90100
> 
> Is it too late to ask for a feature test macro? Maybe one already
> exists? 

No, not as far as I know. Perhaps something like below, though that
won't affect the already released gcc 9.1 and 9.2, of course.

gcc maintainers, WDYT? Can we add a feature test macro for asm inline()?
For context, I'm trying to add an asm_inline macro to the kernel source
that will fall back to asm when "asm inline" is not supported - see
https://lore.kernel.org/lkml/20190830231527.22304-1-linux@rasmusvillemoes.dk/
for the whole thread.

From: Rasmus Villemoes <rv@rasmusvillemoes.dk>
Subject: [PATCH] add feature test macro for "asm inline"

Allow users to check availability of "asm inline()" via a feature test
macro. If and when clang implements support for "asm inline()", it's
easier for users if they can just test __HAVE_ASM_INLINE rather than
juggling different version checks for different compilers.

Changelog:

gcc/c-family/

	* c-cppbuiltin.c (c_cpp_builtins): Add pre-define for
	__HAVE_ASM_INLINE.

gcc/

	* doc/cpp.texi: Document predefine __HAVE_ASM_INLINE.
---
 gcc/c-family/c-cppbuiltin.c | 3 +++
 gcc/doc/cpp.texi            | 5 +++++
 2 files changed, 8 insertions(+)

diff --git a/gcc/c-family/c-cppbuiltin.c b/gcc/c-family/c-cppbuiltin.c
index fc68bc4d0c4..163f3058741 100644
--- a/gcc/c-family/c-cppbuiltin.c
+++ b/gcc/c-family/c-cppbuiltin.c
@@ -1383,6 +1383,9 @@ c_cpp_builtins (cpp_reader *pfile)
   if (targetm.have_speculation_safe_value (false))
     cpp_define (pfile, "__HAVE_SPECULATION_SAFE_VALUE");

+  /* Show the availability of "asm inline()".  */
+  cpp_define (pfile, "__HAVE_ASM_INLINE");
+
 #ifdef DWARF2_UNWIND_INFO
   if (dwarf2out_do_cfi_asm ())
     cpp_define (pfile, "__GCC_HAVE_DWARF2_CFI_ASM");
diff --git a/gcc/doc/cpp.texi b/gcc/doc/cpp.texi
index e271f5180d8..98f6d625857 100644
--- a/gcc/doc/cpp.texi
+++ b/gcc/doc/cpp.texi
@@ -2386,6 +2386,11 @@ and swap operations on operands 1, 2, 4, 8 or 16
bytes in length, respectively.
 This macro is defined with the value 1 to show that this version of GCC
 supports @code{__builtin_speculation_safe_value}.

+@item __HAVE_ASM_INLINE
+This macro is defined with the value 1 to show that this version of GCC
+supports @code{asm inline()}.  @xref{Size of an asm,,, gcc, Using
+the GNU Compiler Collection (GCC)}.
+
 @item __GCC_HAVE_DWARF2_CFI_ASM
 This macro is defined when the compiler is emitting DWARF CFI directives
 to the assembler.  When this is defined, it is possible to emit those same

Re: [PATCH v2 4/6] compiler-gcc.h: add asm_inline definition

[Segher Boessenkool]

Hi Rasmus,

On Thu, Sep 05, 2019 at 01:07:11PM +0200, Rasmus Villemoes wrote:
> On 05/09/2019 02.18, Nick Desaulniers wrote:
> > Is it too late to ask for a feature test macro? Maybe one already
> > exists? 
> 
> No, not as far as I know.

[ That's not what a feature test macro is; a feature test macro allows the
  user to select some optional behaviour.  Things like _GNU_SOURCE.  ]

> Perhaps something like below, though that
> won't affect the already released gcc 9.1 and 9.2, of course.

That is one reason to not want such a predefined macro.  Another reason
is that you usually need to compile some test programs *anyway*, to see if
some bug is present for example, or to see if the exact implementation of
the feature is beneficial (or harmful) to your program in some way.

> gcc maintainers, WDYT? Can we add a feature test macro for asm inline()?

The only comparable existing predefined macro is __PRAGMA_REDEFINE_EXTNAME
it seems (no, I have no idea either).  Everything else is required by some
standard (a "standard standard" or a "vendor standard", I'm lumping
everything together here), or shows whether some target has some feature,
or how many bits there are in certain types, that kind of thing.

Why would GCC want to have macros for all features it has?  That would be
quite a few new ones every release.  And what about bug fixes, are bug
fixes features as well?

I think you need to solve your configuration problems in your
configuration system.


Segher

Re: [PATCH v2 4/6] compiler-gcc.h: add asm_inline definition

[Rasmus Villemoes]

On 05/09/2019 15.45, Segher Boessenkool wrote:
> Hi Rasmus,
> 
> On Thu, Sep 05, 2019 at 01:07:11PM +0200, Rasmus Villemoes wrote:
>> On 05/09/2019 02.18, Nick Desaulniers wrote:
>>> Is it too late to ask for a feature test macro? Maybe one already
>>> exists? 
>>
>> No, not as far as I know.
> 
> [ That's not what a feature test macro is; a feature test macro allows the
>   user to select some optional behaviour.  Things like _GNU_SOURCE.  ]
> 
>> Perhaps something like below, though that
>> won't affect the already released gcc 9.1 and 9.2, of course.
> 
> That is one reason to not want such a predefined macro.  Another reason
> is that you usually need to compile some test programs *anyway*, to see if
> some bug is present for example, or to see if the exact implementation of
> the feature is beneficial (or harmful) to your program in some way.

OK, I think I'll just use a version check for now, and then switch to a
Kconfig test if and when clang grows support.

>> gcc maintainers, WDYT? Can we add a feature test macro for asm inline()?
> 
> 
> Why would GCC want to have macros for all features it has? 

Well, gcc has implemented __has_attribute() which is similar - one could
detect support by compiling a trivial test program. Or the same could be
said for many of the predefined macros that are conditionally defined,
e.g. __HAVE_SPECULATION_SAFE_VALUE.

But I was just throwing the question into the air, I won't pursue this
further.

Thanks,
Rasmus

Re: [PATCH v2 4/6] compiler-gcc.h: add asm_inline definition

[Segher Boessenkool]

On Thu, Sep 05, 2019 at 04:23:11PM +0200, Rasmus Villemoes wrote:
> On 05/09/2019 15.45, Segher Boessenkool wrote:
> > On Thu, Sep 05, 2019 at 01:07:11PM +0200, Rasmus Villemoes wrote:
> >> Perhaps something like below, though that
> >> won't affect the already released gcc 9.1 and 9.2, of course.
> > 
> > That is one reason to not want such a predefined macro.  Another reason
> > is that you usually need to compile some test programs *anyway*, to see if
> > some bug is present for example, or to see if the exact implementation of
> > the feature is beneficial (or harmful) to your program in some way.
> 
> OK, I think I'll just use a version check for now, and then switch to a
> Kconfig test if and when clang grows support.
> 
> >> gcc maintainers, WDYT? Can we add a feature test macro for asm inline()?
> > 
> > Why would GCC want to have macros for all features it has? 
> 
> Well, gcc has implemented __has_attribute() which is similar - one could
> detect support by compiling a trivial test program.

It is not a macro, it doesn't spill over the place, and it is for
detecting things that are already fixed strings, much easier to do :-)

> Or the same could be
> said for many of the predefined macros that are conditionally defined,
> e.g. __HAVE_SPECULATION_SAFE_VALUE.

That one happened because of the Great Security Scare of 2017/2018, it's
not a good precedent.  And, how it is set is target-specific, it can
depend on CPU model selected, target code generation options, or whatnot.

> But I was just throwing the question into the air, I won't pursue this
> further.

Maybe GCC should have a has_feature thing, it might fit in well there.
As preprocessor macros, not so much, IMO.


Segher

Re: [PATCH v2 4/6] compiler-gcc.h: add asm_inline definition

[Miguel Ojeda]

On Thu, Sep 5, 2019 at 3:45 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> [ That's not what a feature test macro is; a feature test macro allows the
>   user to select some optional behaviour.  Things like _GNU_SOURCE.  ]

Yes and no. GNU libc defines feature test macros like you say, but
C++'s feature macros are like Rasmus/Nick are saying. I think libc's
definition is weird, I would call those "feature selection macros"
instead, because the user is selecting between some features (whether
to enable or not, for instance), rather than testing for the features.

> Why would GCC want to have macros for all features it has?  That would be
> quite a few new ones every release.

Maybe GCC wouldn't, but its users, they surely would. For anything
that 1) is a new language feature, 2) breaks backwards-compatibility
with previous (or other compilers) and 3) is expected to be used by
end users, yes, it would be very useful to have.

For the same reasons C++ is adding feature test macros all over the
place nowadays and it is considered good practice (see SD-6: SG10
Feature Test Recommendations).

Cheers,
Miguel

Re: [PATCH v2 4/6] compiler-gcc.h: add asm_inline definition

[Miguel Ojeda]

On Thu, Sep 5, 2019 at 5:52 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> Yes and no. GNU libc defines feature test macros like you say, but
> C++'s feature macros are like Rasmus/Nick are saying. I think libc's
> definition is weird, I would call those "feature selection macros"
> instead, because the user is selecting between some features (whether
> to enable or not, for instance), rather than testing for the features.

By the way, this is not to criticize libc, I imagine they employed that
nomenclature since that is what some standards used, but still, the
naming is not great from the users' perspective vs. the header
writer's perspective, IMO.

Cheers,
Miguel

Re: [PATCH v2 4/6] compiler-gcc.h: add asm_inline definition

[Segher Boessenkool]

On Thu, Sep 05, 2019 at 05:52:44PM +0200, Miguel Ojeda wrote:
> On Thu, Sep 5, 2019 at 3:45 PM Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
> >
> > [ That's not what a feature test macro is; a feature test macro allows the
> >   user to select some optional behaviour.  Things like _GNU_SOURCE.  ]
> 
> Yes and no. GNU libc defines feature test macros like you say, but
> C++'s feature macros are like Rasmus/Nick are saying. I think libc's

I can't find anything with "feature" and "macros" in the C++ standard,
it's "predefined macros" there I guess?  In C, it is also "predefined
macros" in general, and there is "conditional feature macros".

> definition is weird, I would call those "feature selection macros"
> instead, because the user is selecting between some features (whether
> to enable or not, for instance), rather than testing for the features.

Sure.  But the name is traditional, many decades old, it predates glibc.
Using an established name to mean pretty much the opposite of what it
normally does is a bit confusing, never mind if that usage makes much
sense ;-)


Segher

Re: [PATCH v2 4/6] compiler-gcc.h: add asm_inline definition

[Miguel Ojeda]

On Fri, Sep 6, 2019 at 2:23 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> I can't find anything with "feature" and "macros" in the C++ standard,
> it's "predefined macros" there I guess?  In C, it is also "predefined
> macros" in general, and there is "conditional feature macros".

They are introduced in C++20, but they have been added for a lot of
older features in both the language (see [cpp.predefined]p1, around 50
of them) and the library (see [support.limits.general]p3, ~100):

    http://eel.is/c++draft/cpp.predefined#tab:cpp.predefined.ft
    http://eel.is/c++draft/support.limits#tab:support.ft

> Sure.  But the name is traditional, many decades old, it predates glibc.
> Using an established name to mean pretty much the opposite of what it
> normally does is a bit confusing, never mind if that usage makes much
> sense ;-)

Agreed on principle :-) However, I wouldn't say it is the opposite. I
would say they are the same, but from different perspectives: one says
"I want to test if the user enabled the feature", the other says "I
want to test if the vendor implemented the feature". Which is fine,
but for users the meaning is inverted as you say: in the first case
they want to say "I want to enable this feature in this library" --
they don't want to "test" anything. And since most people will be
users, not vendors writing standard libraries, I think the user
perspective would have been better.

Cheers,
Miguel

Re: [PATCH v2 4/6] compiler-gcc.h: add asm_inline definition

[Segher Boessenkool]

On Fri, Sep 06, 2019 at 05:13:54PM +0200, Miguel Ojeda wrote:
> On Fri, Sep 6, 2019 at 2:23 PM Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
> > I can't find anything with "feature" and "macros" in the C++ standard,
> > it's "predefined macros" there I guess?  In C, it is also "predefined
> > macros" in general, and there is "conditional feature macros".
> 
> They are introduced in C++20,

(Which isn't the C++ standard yet, okay).

> but they have been added for a lot of
> older features in both the language (see [cpp.predefined]p1, around 50
> of them) and the library (see [support.limits.general]p3, ~100):
> 
>     http://eel.is/c++draft/cpp.predefined#tab:cpp.predefined.ft
>     http://eel.is/c++draft/support.limits#tab:support.ft

And they spell it "feature-test" there.  Lovely :-/

> > Sure.  But the name is traditional, many decades old, it predates glibc.
> > Using an established name to mean pretty much the opposite of what it
> > normally does is a bit confusing, never mind if that usage makes much
> > sense ;-)
> 
> Agreed on principle :-) However, I wouldn't say it is the opposite. I
> would say they are the same, but from different perspectives: one says
> "I want to test if the user enabled the feature", the other says "I
> want to test if the vendor implemented the feature".

No, that is not what it does.  A user defines such a macro, and that
makes the library change behaviour.

As the GNU C Library manual explains:

     This system exists to allow the library to conform to multiple
  standards.  Although the different standards are often described as
  supersets of each other, they are usually incompatible because larger
  standards require functions with names that smaller ones reserve to the
  user program.  This is not mere pedantry -- it has been a problem in
  practice.  For instance, some non-GNU programs define functions named
  'getline' that have nothing to do with this library's 'getline'.  They
  would not be compilable if all features were enabled indiscriminately.

https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html


Segher

Re: [PATCH v2 4/6] compiler-gcc.h: add asm_inline definition

[Jakub Jelinek]

On Fri, Sep 06, 2019 at 11:30:28AM -0500, Segher Boessenkool wrote:
> On Fri, Sep 06, 2019 at 05:13:54PM +0200, Miguel Ojeda wrote:
> > On Fri, Sep 6, 2019 at 2:23 PM Segher Boessenkool
> > <segher@kernel.crashing.org> wrote:
> > > I can't find anything with "feature" and "macros" in the C++ standard,
> > > it's "predefined macros" there I guess?  In C, it is also "predefined
> > > macros" in general, and there is "conditional feature macros".
> > 
> > They are introduced in C++20,
> 
> (Which isn't the C++ standard yet, okay).

Well, they have been required by SD-6 before being added to C++20, so we
have tons of the predefined macros for C++ already starting with gcc 4.9 or
so, but it is something required by the standard so we have to do that.
Most of them depend also on compiler options, so can't be easily replaced
with a simple __GNUC__ version check.

What I'd like to add is that each predefined macro isn't without cost,
while adding one predefined macro might not be measurable (though, for
some predefined macros (the floating point values) it was very measurable
and we had to resort to lazy evaluation of the macros), adding hundreds of
predefined macros is measurable, affects the speed of empty compiler run,
adds size of -g3 produced debug info, increases size of -E -dD output etc.

	Jakub

Re: [PATCH v2 4/6] compiler-gcc.h: add asm_inline definition

[Nick Desaulniers via gcc-patches]

On Fri, Sep 6, 2019 at 9:39 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Fri, Sep 06, 2019 at 11:30:28AM -0500, Segher Boessenkool wrote:
> > On Fri, Sep 06, 2019 at 05:13:54PM +0200, Miguel Ojeda wrote:
> > > On Fri, Sep 6, 2019 at 2:23 PM Segher Boessenkool
> > > <segher@kernel.crashing.org> wrote:
> > > > I can't find anything with "feature" and "macros" in the C++ standard,
> > > > it's "predefined macros" there I guess?  In C, it is also "predefined
> > > > macros" in general, and there is "conditional feature macros".
> > >
> > > They are introduced in C++20,
> >
> > (Which isn't the C++ standard yet, okay).
>
> Well, they have been required by SD-6 before being added to C++20, so we
> have tons of the predefined macros for C++ already starting with gcc 4.9 or
> so, but it is something required by the standard so we have to do that.
> Most of them depend also on compiler options, so can't be easily replaced
> with a simple __GNUC__ version check.
>
> What I'd like to add is that each predefined macro isn't without cost,
> while adding one predefined macro might not be measurable (though, for
> some predefined macros (the floating point values) it was very measurable
> and we had to resort to lazy evaluation of the macros), adding hundreds of
> predefined macros is measurable, affects the speed of empty compiler run,
> adds size of -g3 produced debug info, increases size of -E -dD output etc.
>
>         Jakub

Here's the case that I think is perfect:
https://developers.redhat.com/blog/2016/02/25/new-asm-flags-feature-for-x86-in-gcc-6/

Specifically the feature test preprocessor define __GCC_ASM_FLAG_OUTPUTS__.

See exactly how we handle it in the kernel:
- https://github.com/ClangBuiltLinux/linux/blob/0445971000375859008414f87e7c72fa0d809cf8/arch/x86/include/asm/asm.h#L112-L118
- https://github.com/ClangBuiltLinux/linux/blob/0445971000375859008414f87e7c72fa0d809cf8/arch/x86/include/asm/rmwcc.h#L14-L30

Feature detection of the feature makes it trivial to detect when the
feature is supported, rather than brittle compiler version checks.
Had it been a GCC version check, it wouldn't work for clang out of the
box when clang added support for __GCC_ASM_FLAG_OUTPUTS__.  But since
we had the helpful __GCC_ASM_FLAG_OUTPUTS__, and wisely based our use
of the feature on that preprocessor define, the code ***just worked***
for compilers that didn't support the feature ***and*** compilers when
they did support the feature ***without changing any of the source
code*** being compiled.

All I'm asking for is that when GCC adds a new GNU C extension (such
as `asm inline`), define a new preprocessor symbol (like has already
been done w/ __GCC_ASM_FLAG_OUTPUTS__), so that we don't have to use
version checking (or reimplementing autoconf) and use feature
detection instead.  This simplifies use of this feature even between
codebases supporting multiple versions of GCC.

(Also, I'm guessing the cost of another preprocessor define is near
zero compared to parsing comments for -Wimplicit-fallthrough)
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2 4/6] compiler-gcc.h: add asm_inline definition

[Segher Boessenkool]

On Fri, Sep 06, 2019 at 11:14:08AM -0700, Nick Desaulniers wrote:
> Here's the case that I think is perfect:
> https://developers.redhat.com/blog/2016/02/25/new-asm-flags-feature-for-x86-in-gcc-6/
> 
> Specifically the feature test preprocessor define __GCC_ASM_FLAG_OUTPUTS__.
> 
> See exactly how we handle it in the kernel:
> - https://github.com/ClangBuiltLinux/linux/blob/0445971000375859008414f87e7c72fa0d809cf8/arch/x86/include/asm/asm.h#L112-L118
> - https://github.com/ClangBuiltLinux/linux/blob/0445971000375859008414f87e7c72fa0d809cf8/arch/x86/include/asm/rmwcc.h#L14-L30
> 
> Feature detection of the feature makes it trivial to detect when the
> feature is supported, rather than brittle compiler version checks.
> Had it been a GCC version check, it wouldn't work for clang out of the
> box when clang added support for __GCC_ASM_FLAG_OUTPUTS__.  But since
> we had the helpful __GCC_ASM_FLAG_OUTPUTS__, and wisely based our use
> of the feature on that preprocessor define, the code ***just worked***
> for compilers that didn't support the feature ***and*** compilers when
> they did support the feature ***without changing any of the source
> code*** being compiled.

And if instead you tested whether the actual feature you need works as
you need it to, it would even work fine if there was a bug we fixed that
breaks things for the kernel.  Without needing a new compiler.

Or as another example, if we added support for some other flags. (x86
has only a few flags; many other archs have many more, and in some cases
newer hardware actually has more flags than older).

With the "macro" scheme we would need to add new macros in all these
cases.  And since those are target-specific macros, that quickly expands
beyond reasonable bounds.

If you want to know if you can do X in some environment, just try to do X.


Segher

Re: [PATCH v2 4/6] compiler-gcc.h: add asm_inline definition

[Nick Desaulniers via gcc-patches]

On Fri, Sep 6, 2019 at 3:03 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Fri, Sep 06, 2019 at 11:14:08AM -0700, Nick Desaulniers wrote:
> > Here's the case that I think is perfect:
> > https://developers.redhat.com/blog/2016/02/25/new-asm-flags-feature-for-x86-in-gcc-6/
> >
> > Specifically the feature test preprocessor define __GCC_ASM_FLAG_OUTPUTS__.
> >
> > See exactly how we handle it in the kernel:
> > - https://github.com/ClangBuiltLinux/linux/blob/0445971000375859008414f87e7c72fa0d809cf8/arch/x86/include/asm/asm.h#L112-L118
> > - https://github.com/ClangBuiltLinux/linux/blob/0445971000375859008414f87e7c72fa0d809cf8/arch/x86/include/asm/rmwcc.h#L14-L30
> >
> > Feature detection of the feature makes it trivial to detect when the
> > feature is supported, rather than brittle compiler version checks.
> > Had it been a GCC version check, it wouldn't work for clang out of the
> > box when clang added support for __GCC_ASM_FLAG_OUTPUTS__.  But since
> > we had the helpful __GCC_ASM_FLAG_OUTPUTS__, and wisely based our use
> > of the feature on that preprocessor define, the code ***just worked***
> > for compilers that didn't support the feature ***and*** compilers when
> > they did support the feature ***without changing any of the source
> > code*** being compiled.
>
> And if instead you tested whether the actual feature you need works as
> you need it to, it would even work fine if there was a bug we fixed that
> breaks things for the kernel.  Without needing a new compiler.

That assumes a feature is broken out of the gate and is putting the
cart before the horse.  If a feature is available, it should work.  If
you later find it to be unsatisfactory, sure go out of your way to add
ugly compiler-specific version checks or upgrade your minimally
supported toolchain; until then feature detection is much cleaner (see
again __GCC_ASM_FLAG_OUTPUTS__).

>
> Or as another example, if we added support for some other flags. (x86
> has only a few flags; many other archs have many more, and in some cases
> newer hardware actually has more flags than older).

I think compiler flags are orthogonal to GNU C extensions we're discussing here.

>
> With the "macro" scheme we would need to add new macros in all these
> cases.  And since those are target-specific macros, that quickly expands
> beyond reasonable bounds.

I don't think so.  Can you show me an example codebase that proves me wrong?

>
> If you want to know if you can do X in some environment, just try to do X.

That's a very autoconf centric viewpoint.  Why doesn't the kernel take
that approach for __GCC_ASM_FLAG_OUTPUTS__?  Why not repeatedly invoke
$CC to find if every compiler __attribute__ is supported?  Do you
think it's faster for the C preprocessor to check for a few #ifdefs,
or to repeatedly invoke $CC at build or compile time to detect new
features?
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2 4/6] compiler-gcc.h: add asm_inline definition

[Segher Boessenkool]

On Fri, Sep 06, 2019 at 03:35:02PM -0700, Nick Desaulniers wrote:
> On Fri, Sep 6, 2019 at 3:03 PM Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
> > And if instead you tested whether the actual feature you need works as
> > you need it to, it would even work fine if there was a bug we fixed that
> > breaks things for the kernel.  Without needing a new compiler.
> 
> That assumes a feature is broken out of the gate and is putting the
> cart before the horse.  If a feature is available, it should work.

GCC currently has 91696 bug reports.

> > Or as another example, if we added support for some other flags. (x86
> > has only a few flags; many other archs have many more, and in some cases
> > newer hardware actually has more flags than older).
> 
> I think compiler flags are orthogonal to GNU C extensions we're discussing here.

No, I am talking exactly about what you brought up.  The flags output
for inline assembler, using the =@ syntax.  If I had implemented that
for Power when it first came up, I would by now have had to add support
for another flag (the CA32 flag).  Oh, and I would not have implemented
support for OV or SO at all originally, but perhaps they are useful, so
let's add that as well.  And there is OV32 now as well.

> > With the "macro" scheme we would need to add new macros in all these
> > cases.  And since those are target-specific macros, that quickly expands
> > beyond reasonable bounds.
> 
> I don't think so.  Can you show me an example codebase that proves me wrong?

No, of course not, because we aren't silly enough to implement something
like that.

> > If you want to know if you can do X in some environment, just try to do X.
> 
> That's a very autoconf centric viewpoint.  Why doesn't the kernel take
> that approach for __GCC_ASM_FLAG_OUTPUTS__?

Ask them, not me.


Segher

Re: [PATCH v2 4/6] compiler-gcc.h: add asm_inline definition

[Nick Desaulniers via gcc-patches]

On Fri, Sep 6, 2019 at 3:56 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Fri, Sep 06, 2019 at 03:35:02PM -0700, Nick Desaulniers wrote:
> > On Fri, Sep 6, 2019 at 3:03 PM Segher Boessenkool
> > <segher@kernel.crashing.org> wrote:
> > > And if instead you tested whether the actual feature you need works as
> > > you need it to, it would even work fine if there was a bug we fixed that
> > > breaks things for the kernel.  Without needing a new compiler.
> >
> > That assumes a feature is broken out of the gate and is putting the
> > cart before the horse.  If a feature is available, it should work.
>
> GCC currently has 91696 bug reports.

Fair.

>
> > > Or as another example, if we added support for some other flags. (x86
> > > has only a few flags; many other archs have many more, and in some cases
> > > newer hardware actually has more flags than older).
> >
> > I think compiler flags are orthogonal to GNU C extensions we're discussing here.
>
> No, I am talking exactly about what you brought up.  The flags output
> for inline assembler, using the =@ syntax.  If I had implemented that
> for Power when it first came up, I would by now have had to add support
> for another flag (the CA32 flag).  Oh, and I would not have implemented
> support for OV or SO at all originally, but perhaps they are useful, so
> let's add that as well.  And there is OV32 now as well.

Oh, I misunderstood.  I see your point.  Define the symbol as a number
for what level of output flags you support (ie. the __cplusplus
macro).

>
> > > With the "macro" scheme we would need to add new macros in all these
> > > cases.  And since those are target-specific macros, that quickly expands
> > > beyond reasonable bounds.
> >
> > I don't think so.  Can you show me an example codebase that proves me wrong?
>
> No, of course not, because we aren't silly enough to implement something
> like that.

I still don't think feature detection is worse than version detection
(until you find your feature broken in a way that forces the use of
version detection).

Just to prove my point about version checks being brittle, it looks
like Rasmus' version check isn't even right.  GCC supported `asm
inline` back in the 8.3 release, not 9.1 as in this patch:
https://godbolt.org/z/1woitS .  So users of gcc-8.2 and gcc-8.3
wouldn't be able to take advantage of `asm inline` even though their
compiler supported it.

Or was it "broken" until 9.1?  Lord knows, as `asm inline` wasn't in
any release notes or bug reports I can find:
8: https://gcc.gnu.org/gcc-8/changes.html
9: https://gcc.gnu.org/gcc-9/changes.html
Bug tracker query:
https://gcc.gnu.org/bugzilla/buglist.cgi?bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=SUSPENDED&bug_status=WAITING&bug_status=REOPENED&cf_known_to_fail_type=allwords&cf_known_to_work_type=allwords&query_format=advanced&short_desc=asm%20inline&short_desc_type=anywordssubstr

Ah, here it is:
https://github.com/gcc-mirror/gcc/commit/6de46ad5326fc5e6b730a2feb8c62d09c1561f92
Which looks like the qualifier was added to this page:
https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html

I like many of the GNU C extensions, and I want to help support them
in Clang so that they can be used in more places, but the current
process (and questions I have when I implement some of them) leaves me
with the sense that there's probably room for improvement on some of
these things before they go out the door.

Segher, next time there's discussion about new C extensions for the
kernel, can you please include me in the discussions?
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2 4/6] compiler-gcc.h: add asm_inline definition

[Segher Boessenkool]

On Fri, Sep 06, 2019 at 04:42:58PM -0700, Nick Desaulniers via gcc-patches wrote:
> On Fri, Sep 6, 2019 at 3:56 PM Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
> Oh, I misunderstood.  I see your point.  Define the symbol as a number
> for what level of output flags you support (ie. the __cplusplus
> macro).

That works if history is linear.  I guess with enough effort we can make
that work in most cases (for backports it is a problem, if you want to
support a new feature -- or bugfix! -- you need to support all previous
ones as well...  Distros are going to hate us, too ;-) )

> > > I don't think so.  Can you show me an example codebase that proves me wrong?
> >
> > No, of course not, because we aren't silly enough to implement something
> > like that.
> 
> I still don't think feature detection is worse than version detection
> (until you find your feature broken in a way that forces the use of
> version detection).

*You* bring up version detection.  I don't.  You want some halfway thing,
with some macros that say what version some part of your compiler is.

I say to just detect the feature you want, and if it actually works the
way you want it, etc.

> Just to prove my point about version checks being brittle, it looks
> like Rasmus' version check isn't even right.  GCC supported `asm
> inline` back in the 8.3 release, not 9.1 as in this patch:

Yes, I backported it so that it is available in 7.5, 8.3, and 9.1, so
that more users will have this available sooner.  (7.5 has not been
released yet, but asm inline has been supported in GCC 7 since Jan 2
this year).

> Or was it "broken" until 9.1?  Lord knows, as `asm inline` wasn't in
> any release notes or bug reports I can find:

https://gcc.gnu.org/ml/gcc-patches/2019-02/msg01143.html

It never was accepted, and I dropped the ball.

> Ah, here it is:
> https://github.com/gcc-mirror/gcc/commit/6de46ad5326fc5e6b730a2feb8c62d09c1561f92
> Which looks like the qualifier was added to this page:
> https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html

Sure, it is part of the documentation just fine.  And it works just fine
too, it is a *very* simple feature.  By design.

> I like many of the GNU C extensions, and I want to help support them
> in Clang so that they can be used in more places, but the current
> process (and questions I have when I implement some of them) leaves me
> with the sense that there's probably room for improvement on some of
> these things before they go out the door.
> 
> Segher, next time there's discussion about new C extensions for the
> kernel, can you please include me in the discussions?

You can lurk on gcc-patches@ and/or gcc@?  But I'll try to remember, sure.
Not that I am involved in all such discussions myself, mind.


Segher

Re: [PATCH v2 4/6] compiler-gcc.h: add asm_inline definition

[Nick Desaulniers via gcc-patches]

On Fri, Sep 6, 2019 at 5:14 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Fri, Sep 06, 2019 at 04:42:58PM -0700, Nick Desaulniers via gcc-patches wrote:
> > Just to prove my point about version checks being brittle, it looks
> > like Rasmus' version check isn't even right.  GCC supported `asm
> > inline` back in the 8.3 release, not 9.1 as in this patch:
>
> Yes, I backported it so that it is available in 7.5, 8.3, and 9.1, so
> that more users will have this available sooner.  (7.5 has not been
> released yet, but asm inline has been supported in GCC 7 since Jan 2
> this year).

Ah, ok that makes sense.

How would you even write a version check for that?

Which looks better?

#if __GNU_MAJOR__ > 9 || __GNU_MAJOR__ == 8 && __GNU_MINOR__ >= 3 ||
__GNU_MAJOR__ == 7 && __GNU_MINOR__ >= 5 || __CLANG_MAJOR__ == 42
// make use of `asm inline`
#endif

or

#ifdef __CC_HAS_ASM_INLINE__
// make use of `asm inline`
#endif

>
> > Or was it "broken" until 9.1?  Lord knows, as `asm inline` wasn't in
> > any release notes or bug reports I can find:
>
> https://gcc.gnu.org/ml/gcc-patches/2019-02/msg01143.html
>
> It never was accepted, and I dropped the ball.

Ah, ok, that's fine, so documentation was at least written.  Tracking
when and where patches land (or don't) is difficult when patch files
are emailed around.  I try to keep track of when and where our kernel
patches land, but I frequently drop the ball there.

> > Segher, next time there's discussion about new C extensions for the
> > kernel, can you please include me in the discussions?
>
> You can lurk on gcc-patches@ and/or gcc@?

Please "interrupt" me when you're aware of such discussions, rather
than me "polling" a mailing list.  (I will buy you a tasty beverage of
your preference).  I'm already subscribed to more mailing lists than I
have time to read.

> But I'll try to remember, sure.
> Not that I am involved in all such discussions myself, mind.

But you _did_ implement `asm inline`. ;)
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2 4/6] compiler-gcc.h: add asm_inline definition

[Segher Boessenkool]

On Fri, Sep 06, 2019 at 06:04:54PM -0700, Nick Desaulniers wrote:
> On Fri, Sep 6, 2019 at 5:14 PM Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
> > On Fri, Sep 06, 2019 at 04:42:58PM -0700, Nick Desaulniers via gcc-patches wrote:
> > > Just to prove my point about version checks being brittle, it looks
> > > like Rasmus' version check isn't even right.  GCC supported `asm
> > > inline` back in the 8.3 release, not 9.1 as in this patch:
> >
> > Yes, I backported it so that it is available in 7.5, 8.3, and 9.1, so
> > that more users will have this available sooner.  (7.5 has not been
> > released yet, but asm inline has been supported in GCC 7 since Jan 2
> > this year).
> 
> Ah, ok that makes sense.
> 
> How would you even write a version check for that?

I wouldn't.  Please stop using that straw man.  I'm not saying version
checks are good, or useful for most things.  I am saying they are not.

Predefined compiler symbols to do version checking (of a feature) is
just a lesser instance of the same problem though.  (And it causes its
own more or less obvious problems as well).

> > > Or was it "broken" until 9.1?  Lord knows, as `asm inline` wasn't in
> > > any release notes or bug reports I can find:
> >
> > https://gcc.gnu.org/ml/gcc-patches/2019-02/msg01143.html
> >
> > It never was accepted, and I dropped the ball.
> 
> Ah, ok, that's fine, so documentation was at least written.  Tracking
> when and where patches land (or don't) is difficult when patch files
> are emailed around.  I try to keep track of when and where our kernel
> patches land, but I frequently drop the ball there.

I keep track of most things just fine...  But the release notes are part
of our web content, which is in a separate CVS repository (still nicer
than SVN :-) ), and since I don't use it very often it falls outside of
all my normal procedures.

> your preference).  I'm already subscribed to more mailing lists than I
> have time to read.
> 
> > But I'll try to remember, sure.
> > Not that I am involved in all such discussions myself, mind.
> 
> But you _did_ implement `asm inline`. ;)

That started as just

+       /* If this asm is asm inline, count anything as minimum size.  */
+       if (gimple_asm_inline_p (as_a <gasm *> (stmt)))
+         count = MIN (1, count);

(in estimate_num_insns) but then things ballooned.  Like such things do.


Segher

Re: [PATCH v2 4/6] compiler-gcc.h: add asm_inline definition

[Miguel Ojeda]

On Sat, Sep 7, 2019 at 3:11 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> I wouldn't.  Please stop using that straw man.  I'm not saying version
> checks are good, or useful for most things.  I am saying they are not.
>
> Predefined compiler symbols to do version checking (of a feature) is
> just a lesser instance of the same problem though.  (And it causes its
> own more or less obvious problems as well).

That is fair enough, but what are you suggesting we use, then?

Because "testing to do X to know if you can do X" cannot work with
source code alone and implies each project has to redo this work in
its build system for each compiler, plus each project ends up with
different names for these macros. The C++20 approach of having
standardized macros for major features is way better (whether we have
one macro per feature or a __has_feature(X) macro). Note this does not
mean we need to take this to the extreme and have a feature-test macro
for *every* feature, bugfix or behavior change as a macro.

Cheers,
Miguel

Re: [PATCH v2 4/6] compiler-gcc.h: add asm_inline definition

[Nick Desaulniers via gcc-patches]

On Sat, Sep 7, 2019 at 6:11 AM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Fri, Sep 06, 2019 at 06:04:54PM -0700, Nick Desaulniers wrote:
> > On Fri, Sep 6, 2019 at 5:14 PM Segher Boessenkool
> > <segher@kernel.crashing.org> wrote:
> > > On Fri, Sep 06, 2019 at 04:42:58PM -0700, Nick Desaulniers via gcc-patches wrote:
> > > > Just to prove my point about version checks being brittle, it looks
> > > > like Rasmus' version check isn't even right.  GCC supported `asm
> > > > inline` back in the 8.3 release, not 9.1 as in this patch:
> > >
> > > Yes, I backported it so that it is available in 7.5, 8.3, and 9.1, so
> > > that more users will have this available sooner.  (7.5 has not been
> > > released yet, but asm inline has been supported in GCC 7 since Jan 2
> > > this year).
> >
> > Ah, ok that makes sense.
> >
> > How would you even write a version check for that?
>
> I wouldn't.  Please stop using that straw man.  I'm not saying version
> checks are good, or useful for most things.  I am saying they are not.

Then please help Rasmus with a suggestion on how best to detect and
safely make use of the feature you implemented.  As is, the patch in
question is using version checks.

>
> Predefined compiler symbols to do version checking (of a feature) is
> just a lesser instance of the same problem though.  (And it causes its
> own more or less obvious problems as well).
>
> > > > Or was it "broken" until 9.1?  Lord knows, as `asm inline` wasn't in
> > > > any release notes or bug reports I can find:
> > >
> > > https://gcc.gnu.org/ml/gcc-patches/2019-02/msg01143.html
> > >
> > > It never was accepted, and I dropped the ball.
> >
> > Ah, ok, that's fine, so documentation was at least written.  Tracking
> > when and where patches land (or don't) is difficult when patch files
> > are emailed around.  I try to keep track of when and where our kernel
> > patches land, but I frequently drop the ball there.
>
> I keep track of most things just fine...  But the release notes are part
> of our web content, which is in a separate CVS repository (still nicer
> than SVN :-) ), and since I don't use it very often it falls outside of
> all my normal procedures.
>
> > your preference).  I'm already subscribed to more mailing lists than I
> > have time to read.
> >
> > > But I'll try to remember, sure.
> > > Not that I am involved in all such discussions myself, mind.
> >
> > But you _did_ implement `asm inline`. ;)
>
> That started as just
>
> +       /* If this asm is asm inline, count anything as minimum size.  */
> +       if (gimple_asm_inline_p (as_a <gasm *> (stmt)))
> +         count = MIN (1, count);
>
> (in estimate_num_insns) but then things ballooned.  Like such things do.

So I'm not convinced this GNU C extension solves the problem it's
described to be used for.  I agree that current implementations in
multiple compilers is imprecise, and leads to developer headaches.  I
think `asm inline` will help in cases where vanilla `asm`
overestimates the size of inline assembly, but I also think it will be
just as bad as vanilla `asm` in cases where the size is
underestimated.

I have seen instances where instruction selection fails to select the
appropriate way to branch when inline asm size is misjudged, resulting
in un-encodeable jumps (as in the branch target is too far to be
encoded in the number of bits of a single jump/branch instruction).
And the use of .pushsection/.popsection assembler directives and
__attribute__((section())) attributes complicates the accounting
further, as you can then place code from the inline assembly in a
different section than the function itself (so that inline assembly
doesn't affect the function's size, and the implications on inlining
the function).  That would cause vanilla `asm` to overestimate size.
(I suspect variable length encoded instruction sets also suffer from
misaccounting).

Short of invoking the assembler itself, and then matching the byte
size of generated code section that matches the function's section,
can you accurately describe the size of inline assembly.  .macro and
.rept assembler directives really complicate estimates and can cause
vanilla `asm` to underestimate size.

I agree that current implementations in multiple compilers is
imprecise, and leads to developer headaches.  Rather than give
developers the ability to choose between 2 different heuristics that
are both imprecise, why not make the existing heuristic (ie. vanilla
`asm`) more precise in its measure?
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2 4/6] compiler-gcc.h: add asm_inline definition

[Rasmus Villemoes]

On 12/09/2019 23.54, Nick Desaulniers wrote:
> On Sat, Sep 7, 2019 at 6:11 AM Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
>>
>> On Fri, Sep 06, 2019 at 06:04:54PM -0700, Nick Desaulniers wrote:
>>
>>> How would you even write a version check for that?
>>
>> I wouldn't.  Please stop using that straw man.  I'm not saying version
>> checks are good, or useful for most things.  I am saying they are not.
> 
> Then please help Rasmus with a suggestion on how best to detect and
> safely make use of the feature you implemented.  As is, the patch in
> question is using version checks.

I was just about to send out an updated version. I'm just going to do
the check in Kconfig - I didn't realize how easy it had become to do
that kind of thing until Masahiro pointed me at his RFC patch from December.

Rasmus

Re: [PATCH v2 4/6] compiler-gcc.h: add asm_inline definition

[Segher Boessenkool]

On Thu, Sep 12, 2019 at 02:54:50PM -0700, Nick Desaulniers wrote:
> I have seen instances where instruction selection fails to select the
> appropriate way to branch when inline asm size is misjudged, resulting
> in un-encodeable jumps (as in the branch target is too far to be
> encoded in the number of bits of a single jump/branch instruction).

"asm inline" *only* influences the estimated size *for inlining
purposes*.  Not for anything else.  Everything else still uses
conservative estimates.


Segher

Re: [PATCH v2 4/6] compiler-gcc.h: add asm_inline definition

[Miguel Ojeda]

On Fri, Sep 6, 2019 at 6:30 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> (Which isn't the C++ standard yet, okay).

At this stage, it pretty much is. It is basically bug fixing at this point.

> No, that is not what it does.  A user defines such a macro, and that
> makes the library change behaviour.

That is what I have said:

  "I want to test if the user enabled the feature"

means the *library* tests if the user enabled the feature before
including the library. But the user does not want to test anything.

Cheers,
Miguel