Re: [PATCH] be2net: fix adapter->big_page_size miscaculation

[James Y Knight]

On Mon, Jul 22, 2019 at 5:13 PM Qian Cai  wrote:
>
> On Fri, 2019-07-19 at 17:47 -0400, Qian Cai wrote:
> > On Thu, 2019-07-18 at 16:29 -0700, David Miller wrote:
> > > From: Qian Cai 
> > > Date: Thu, 18 Jul 2019 19:26:47 -0400
> > >
> > > >
> > > >
> > > > > On Jul 18, 2019, at 5:21 PM, Bill Wendling  wrote:
> > > > >
> > > > > [My previous response was marked as spam...]
> > > > >
> > > > > Top-of-tree clang says that it's const:
> > > > >
> > > > > $ gcc a.c -O2 && ./a.out
> > > > > a is a const.
> > > > >
> > > > > $ clang a.c -O2 && ./a.out
> > > > > a is a const.
> > > >
> > > >
> > > >
> > > > I used clang-7.0.1. So, this is getting worse where both GCC and clang
> > > > will
> > >
> > > start to suffer the
> > > > same problem.
> > >
> > > Then rewrite the module parameter macros such that the non-constness
> > > is evident to all compilers regardless of version.
> > >
> > > That is the place to fix this, otherwise we will just be adding hacks
> > > all over the place rather than in just one spot.
> >
> > The problem is that when the compiler is compiling be_main.o, it has no
> > knowledge about what is going to happen in load_module().  The compiler can
> > only
> > see that a "const struct kernel_param_ops" "__param_ops_rx_frag_size" at the
> > time with
> >
> > __param_ops_rx_frag_size.arg = _frag_size
> >
> > but only in load_module()->parse_args()->parse_one()->param_set_ushort(), it
> > changes "__param_ops_rx_frag_size.arg" which in-turn changes the value
> > of "rx_frag_size".
>
> Even for an obvious case, the compilers still go ahead optimizing a variable 
> as
> a constant. Maybe it is best to revert the commit d66acc39c7ce ("bitops:
> Optimise get_order()") unless some compiler experts could improve the 
> situation.
>
> #include 
>
> int a = 1;
>
> int main(void)
> {
> int *p;
>
> p = 
> *p = 2;
>
> if (__builtin_constant_p(a))
> printf("a is a const.\n");
>
> printf("a = %d\n", a);
>
> return 0;
> }
>
> # gcc -O2 const.c -o const
>
> # ./const
> a is a const.
> a = 2

This example (like the former) is showing correct behavior. At the
point of invocation of __builtin_constant_p here, the compiler knows
that 'a' is 2, because you've just assigned it (through 'p', but that
indirection trivially disappears in optimization).

Re: [GIT PULL] x86/build changes for v4.17

[James Y Knight]

On Thu, Apr 5, 2018 at 5:13 PM Linus Torvalds
 wrote:
> And btw, I hate how stupid gcc is about "constant size arrays but acts
> as a VLA because it wasn't an integer-constant-expression size"
> things.

> Your code generation example really is a sad sad example of it. A good
> optimizer should have generated the same code even if the stupid array
> again syntactically was VLA, because it damn well isn't in reality.

Unfortunately, that behavior is required by the standard, it's not up to
compiler optimization to change. Note that the optimizer in my example
_did_ determine that the VLA had a constant size, and generated
constant-size stack adjustment code. And it knew the result of the
sizeof(), and put the value "4" straight into the return register. The only
difference in the codegen from a non-VLA is the difference which was
required by the language standard -- the useless call to "function".

Note that the return value of the call is unused. And in fact, there is
literally no reason for the expression in "sizeof(expression)" to ever be
evaluated -- the result of the evaluation can _never_ be used! And, yet,
the C99 standard requires that it is evaluated, regardless, when the
resulting type of the expression is a VLA type. I have no idea why

 From C99 6.5.3.4 "The sizeof operator", paragraph 2:
"""
The sizeof operator yields the size (in bytes) of its operand, which may
be an expression or the parenthesized name of a type. The size is
determined from the type of the operand. The result is an integer. If the
type of the operand is a variable length array type, the operand is
evaluated; otherwise, the operand is not evaluated and the result is an
integer constant.
"""

(C11 says the same thing.)

Re: [GIT PULL] x86/build changes for v4.17

[James Y Knight]

On Thu, Apr 5, 2018 at 5:13 PM Linus Torvalds
 wrote:
> And btw, I hate how stupid gcc is about "constant size arrays but acts
> as a VLA because it wasn't an integer-constant-expression size"
> things.

> Your code generation example really is a sad sad example of it. A good
> optimizer should have generated the same code even if the stupid array
> again syntactically was VLA, because it damn well isn't in reality.

Unfortunately, that behavior is required by the standard, it's not up to
compiler optimization to change. Note that the optimizer in my example
_did_ determine that the VLA had a constant size, and generated
constant-size stack adjustment code. And it knew the result of the
sizeof(), and put the value "4" straight into the return register. The only
difference in the codegen from a non-VLA is the difference which was
required by the language standard -- the useless call to "function".

Note that the return value of the call is unused. And in fact, there is
literally no reason for the expression in "sizeof(expression)" to ever be
evaluated -- the result of the evaluation can _never_ be used! And, yet,
the C99 standard requires that it is evaluated, regardless, when the
resulting type of the expression is a VLA type. I have no idea why

 From C99 6.5.3.4 "The sizeof operator", paragraph 2:
"""
The sizeof operator yields the size (in bytes) of its operand, which may
be an expression or the parenthesized name of a type. The size is
determined from the type of the operand. The result is an integer. If the
type of the operand is a variable length array type, the operand is
evaluated; otherwise, the operand is not evaluated and the result is an
integer constant.
"""

(C11 says the same thing.)

Re: [GIT PULL] x86/build changes for v4.17

[James Y Knight]

On Thu, Apr 5, 2018 at 2:06 PM Linus Torvalds
<torva...@linux-foundation.org>
wrote:
> On Thu, Apr 5, 2018 at 10:46 AM, James Y Knight <jykni...@google.com>
wrote:
> >
> > GCC, however, mixes up the concept of a C "constant expression" with the
> > results of running optimization passes such as inlining for its
> > definition/implementation of __builtin_constant_p. Clang does not, and
quite
> > likely will not ever, do that.

> Nothing has ever said that "__builtin_constant_p(x)" means "is x an
> integer constant expression"

I had actually meant that the __builtin_constant_p **itself** had to be a
constant expression, not that its *argument* must be an I-C-E for
__builtin_constant_p to return true.

But after spending some time on further investigating in order to show an
example of how this matters, I must take back my words. I was mistaken
about GCC's semantics.

Take this example:
===
int function(void);
void useval(int*);

int f() {
 int v = 1 + 2;
 int array[2][__builtin_constant_p(v) ? 1 : 100];
 useval(array[0]);
 return sizeof(array[function()]) / sizeof(array[0]);
}
===

Build with "gcc -O -std=c99":
===
f:
 subq$24, %rsp
 leaq8(%rsp), %rdi
 calluseval
 callfunction
 movl$4, %eax
 addq$24, %rsp
 ret
===

Note the fact that "function" is actually *called* indicates that 'array'
is a VLA (...and that C99's sizeof(VLA) semantics are bonkers, but that's
another story...).

Which means that __builtin_constant_p(v) was _not_ evaluated as an integer
constant expression by GCC. Instead, it was left as an expression. And, the
stack frame being only 24 bytes indicates that the __bcp eventually
evaluated to true.

I actually think this actually _is_ something clang can implement. Thanks
for making me try to prove myself. :)

Re: [GIT PULL] x86/build changes for v4.17

[James Y Knight]

On Thu, Apr 5, 2018 at 2:06 PM Linus Torvalds

wrote:
> On Thu, Apr 5, 2018 at 10:46 AM, James Y Knight 
wrote:
> >
> > GCC, however, mixes up the concept of a C "constant expression" with the
> > results of running optimization passes such as inlining for its
> > definition/implementation of __builtin_constant_p. Clang does not, and
quite
> > likely will not ever, do that.

> Nothing has ever said that "__builtin_constant_p(x)" means "is x an
> integer constant expression"

I had actually meant that the __builtin_constant_p **itself** had to be a
constant expression, not that its *argument* must be an I-C-E for
__builtin_constant_p to return true.

But after spending some time on further investigating in order to show an
example of how this matters, I must take back my words. I was mistaken
about GCC's semantics.

Take this example:
===
int function(void);
void useval(int*);

int f() {
 int v = 1 + 2;
 int array[2][__builtin_constant_p(v) ? 1 : 100];
 useval(array[0]);
 return sizeof(array[function()]) / sizeof(array[0]);
}
===

Build with "gcc -O -std=c99":
===
f:
 subq$24, %rsp
 leaq8(%rsp), %rdi
 calluseval
 callfunction
 movl$4, %eax
 addq$24, %rsp
 ret
===

Note the fact that "function" is actually *called* indicates that 'array'
is a VLA (...and that C99's sizeof(VLA) semantics are bonkers, but that's
another story...).

Which means that __builtin_constant_p(v) was _not_ evaluated as an integer
constant expression by GCC. Instead, it was left as an expression. And, the
stack frame being only 24 bytes indicates that the __bcp eventually
evaluated to true.

I actually think this actually _is_ something clang can implement. Thanks
for making me try to prove myself. :)

Re: [GIT PULL] x86/build changes for v4.17

[James Y Knight]

I think maybe you're confused; those functions do not appear to use
__builtin_constant_p, which is the issue at hand. Clang's optimizer is of
course not a complete joke...it can perfectly well optimize functions after
inlining in order to not generate "shit code gen".

GCC, however, mixes up the concept of a C "constant expression" with the
results of running optimization passes such as inlining for its
definition/implementation of __builtin_constant_p. Clang does not, and
quite likely will not ever, do that.

That said, I do believe there are ongoing discussions as to how to best
provide a useful alternative which is less semantically strange, and not
too difficult for to conditionally adopt for a gcc/clang-compatible
codebase such as the kernel.

On Thu, Apr 5, 2018 at 3:20 AM Peter Zijlstra  wrote:

> On Wed, Apr 04, 2018 at 04:31:11PM -0700, Matthias Kaehlcke wrote:

> > From some experiments it looks like clang, in difference to gcc, does
> > not treat constant values passed as parameters to inline function as
> > constants.

> Then you're also missing a heap of optimizations in code like
> rb_erase_augmented() which is specifically constructed to take advantage
> of constant propagation like that.

> Other sites where we expect that to happen is __mutex_lock_common(),
> __update_load_sum() and a bunch of others. There isn't strictly a bug
> here, but not doing that constant propagation will still result in shit
> code gen.

Re: [GIT PULL] x86/build changes for v4.17

[James Y Knight]

I think maybe you're confused; those functions do not appear to use
__builtin_constant_p, which is the issue at hand. Clang's optimizer is of
course not a complete joke...it can perfectly well optimize functions after
inlining in order to not generate "shit code gen".

GCC, however, mixes up the concept of a C "constant expression" with the
results of running optimization passes such as inlining for its
definition/implementation of __builtin_constant_p. Clang does not, and
quite likely will not ever, do that.

That said, I do believe there are ongoing discussions as to how to best
provide a useful alternative which is less semantically strange, and not
too difficult for to conditionally adopt for a gcc/clang-compatible
codebase such as the kernel.

On Thu, Apr 5, 2018 at 3:20 AM Peter Zijlstra  wrote:

> On Wed, Apr 04, 2018 at 04:31:11PM -0700, Matthias Kaehlcke wrote:

> > From some experiments it looks like clang, in difference to gcc, does
> > not treat constant values passed as parameters to inline function as
> > constants.

> Then you're also missing a heap of optimizations in code like
> rb_erase_augmented() which is specifically constructed to take advantage
> of constant propagation like that.

> Other sites where we expect that to happen is __mutex_lock_common(),
> __update_load_sum() and a bunch of others. There isn't strictly a bug
> here, but not doing that constant propagation will still result in shit
> code gen.

Re: [GIT PULL] x86/build changes for v4.17

[James Y Knight]

On Thu, Apr 5, 2018 at 3:08 AM Peter Zijlstra <pet...@infradead.org> wrote:

> On Wed, Apr 04, 2018 at 10:21:05PM +0000, James Y Knight wrote:
> > But allowing random pointer arithmetic, and pointer arithmetic
wraparound,
> > is still different than asserting that an object _field access_ can
> > overflow. Clang does not believe that can happen -- it assumes that an
> > object will still be contiguous. And that's why the llist stuff used to
be
> > broken, before it was corrected to do simply do math on a uintptr_t
(which
> > is a nice and simple and sane fix!).

> That 'fix' wasn't anything simple, I recently ran into that
> member_address_is_nonnull() trainwreck and had to think real hard wtf it
> was about.

I agree the comment there could be clearer. You could replace it with
something like this (apologies: this patch is likely going to be mangled by
gmail's plaintext mode hard-wrapping it.)

diff --git a/include/linux/llist.h b/include/linux/llist.h
index 85abc2915e8d..04e972a0bbe8 100644
--- a/include/linux/llist.h
+++ b/include/linux/llist.h
@@ -99,12 +99,15 @@ static inline void init_llist_head(struct llist_head
*list)
   *
   * This macro is conceptually the same as
   * >member != NULL
- * but it works around the fact that compilers can decide that taking a
member
- * address is never a NULL pointer.
- *
- * Real objects that start at a high address and have a member at NULL are
- * unlikely to exist, but such pointers may be returned e.g. by the
- * container_of() macro.
+ * except that it uses addition on a uintptr_t instead of member
+ * access syntax. This avoids running into a compiler assumption that
+ * objects must be contiguous in memory, and therefore that member
+ * address lookup cannot wrap, and therefore that a field with a
+ * positive offset within an object can never be at address 0.
+ *
+ * Real objects which start at a high address and have a member at
+ * NULL do not exist, but such a pointer is the result of applying
+ * container_of() to NULL, which llist_for_each_entry does.
   */
  #define member_address_is_nonnull(ptr, member) \
   ((uintptr_t)(ptr) + offsetof(typeof(*(ptr)), member) != 0)

Re: [GIT PULL] x86/build changes for v4.17

[James Y Knight]

On Thu, Apr 5, 2018 at 3:08 AM Peter Zijlstra  wrote:

> On Wed, Apr 04, 2018 at 10:21:05PM +0000, James Y Knight wrote:
> > But allowing random pointer arithmetic, and pointer arithmetic
wraparound,
> > is still different than asserting that an object _field access_ can
> > overflow. Clang does not believe that can happen -- it assumes that an
> > object will still be contiguous. And that's why the llist stuff used to
be
> > broken, before it was corrected to do simply do math on a uintptr_t
(which
> > is a nice and simple and sane fix!).

> That 'fix' wasn't anything simple, I recently ran into that
> member_address_is_nonnull() trainwreck and had to think real hard wtf it
> was about.

I agree the comment there could be clearer. You could replace it with
something like this (apologies: this patch is likely going to be mangled by
gmail's plaintext mode hard-wrapping it.)

diff --git a/include/linux/llist.h b/include/linux/llist.h
index 85abc2915e8d..04e972a0bbe8 100644
--- a/include/linux/llist.h
+++ b/include/linux/llist.h
@@ -99,12 +99,15 @@ static inline void init_llist_head(struct llist_head
*list)
   *
   * This macro is conceptually the same as
   * >member != NULL
- * but it works around the fact that compilers can decide that taking a
member
- * address is never a NULL pointer.
- *
- * Real objects that start at a high address and have a member at NULL are
- * unlikely to exist, but such pointers may be returned e.g. by the
- * container_of() macro.
+ * except that it uses addition on a uintptr_t instead of member
+ * access syntax. This avoids running into a compiler assumption that
+ * objects must be contiguous in memory, and therefore that member
+ * address lookup cannot wrap, and therefore that a field with a
+ * positive offset within an object can never be at address 0.
+ *
+ * Real objects which start at a high address and have a member at
+ * NULL do not exist, but such a pointer is the result of applying
+ * container_of() to NULL, which llist_for_each_entry does.
   */
  #define member_address_is_nonnull(ptr, member) \
   ((uintptr_t)(ptr) + offsetof(typeof(*(ptr)), member) != 0)

Re: [GIT PULL] x86/build changes for v4.17

[James Y Knight]

On Wed, Apr 4, 2018 at 3:42 PM Linus Torvalds

wrote:
> So we'd definitely want that "-fno-strict-overflow" to affect pointer
> arithmetic too (or have a separate flag for the pointer equivalent of
> "we play games that may temporarily wrap pointer values around"..

The -fno-strict-overflow flag in clang does do that. You can subtract any
two random pointers you have, whether they're in the same object or not,
even if the math overflows. And you can later add things back together, and
end up back where you started, and load the value. No problem, even though
subtracting pointers from unrelated objects is illegal according to C.

That's why container_of as it's written in the kernel does work in clang --
wrapping arithmetic when given a NULL pointer and everything. (as a
sidenote...it would be a readability improvement to make container_of do
its math with a uintptr_t instead of a void*, since it would be more
*obviously* correct...)

But allowing random pointer arithmetic, and pointer arithmetic wraparound,
is still different than asserting that an object _field access_ can
overflow. Clang does not believe that can happen -- it assumes that an
object will still be contiguous. And that's why the llist stuff used to be
broken, before it was corrected to do simply do math on a uintptr_t (which
is a nice and simple and sane fix!).

Re: [GIT PULL] x86/build changes for v4.17

[James Y Knight]

On Wed, Apr 4, 2018 at 3:42 PM Linus Torvalds

wrote:
> So we'd definitely want that "-fno-strict-overflow" to affect pointer
> arithmetic too (or have a separate flag for the pointer equivalent of
> "we play games that may temporarily wrap pointer values around"..

The -fno-strict-overflow flag in clang does do that. You can subtract any
two random pointers you have, whether they're in the same object or not,
even if the math overflows. And you can later add things back together, and
end up back where you started, and load the value. No problem, even though
subtracting pointers from unrelated objects is illegal according to C.

That's why container_of as it's written in the kernel does work in clang --
wrapping arithmetic when given a NULL pointer and everything. (as a
sidenote...it would be a readability improvement to make container_of do
its math with a uintptr_t instead of a void*, since it would be more
*obviously* correct...)

But allowing random pointer arithmetic, and pointer arithmetic wraparound,
is still different than asserting that an object _field access_ can
overflow. Clang does not believe that can happen -- it assumes that an
object will still be contiguous. And that's why the llist stuff used to be
broken, before it was corrected to do simply do math on a uintptr_t (which
is a nice and simple and sane fix!).

Re: [GIT PULL] x86/build changes for v4.17

[James Y Knight]

On Wed, Apr 4, 2018 at 12:59 PM Greg KH  wrote:
> Here is another horrible work around that was needed to get clang to
> stop generating invalid code, beaec533fc27 ("llist: clang: introduce
> member_address_is_nonnull()")  That one caused a lot of odd failures by
> users, I wonder what else is lurking in that same code pattern.  It's a
> hard one to debug...

I would note that this issue is an entirely different issue from the
null-pointer-deref-is-undefined-behavior optimizations, even though it may
seem superficially similar. For _this_ issue, the behavior at question is
that the compiler assumes that objects are contiguous in memory, and thus
that "_pointer->member_at_nonzero_offset != NULL" is always true. I
don't really see clang ever getting a flag to stop assuming that objects
are contiguous.

Note that clang does actually emit an on-by-default warning for a situation
analogous to this:
   warning: comparison of address of 'p->sub' not equal to a null pointer is
always true [-Wtautological-pointer-compare]
   if (>sub != NULL) {
~~~^~~
...but unfortunately that warning is suppressed when it occurs in a
macro-expansion, so the llist_for_each_entry error was silent.

(OTOH, I _do_ expect clang to eventually gain support for a flag to treat
NULL-pointer deref as a legal operation instead of UB. I'm not really sure
it makes sense for the linux kernel, but it's a useful feature for the
compiler to provide in any case, so why not...)

Re: [GIT PULL] x86/build changes for v4.17

[James Y Knight]

On Wed, Apr 4, 2018 at 12:59 PM Greg KH  wrote:
> Here is another horrible work around that was needed to get clang to
> stop generating invalid code, beaec533fc27 ("llist: clang: introduce
> member_address_is_nonnull()")  That one caused a lot of odd failures by
> users, I wonder what else is lurking in that same code pattern.  It's a
> hard one to debug...

I would note that this issue is an entirely different issue from the
null-pointer-deref-is-undefined-behavior optimizations, even though it may
seem superficially similar. For _this_ issue, the behavior at question is
that the compiler assumes that objects are contiguous in memory, and thus
that "_pointer->member_at_nonzero_offset != NULL" is always true. I
don't really see clang ever getting a flag to stop assuming that objects
are contiguous.

Note that clang does actually emit an on-by-default warning for a situation
analogous to this:
   warning: comparison of address of 'p->sub' not equal to a null pointer is
always true [-Wtautological-pointer-compare]
   if (>sub != NULL) {
~~~^~~
...but unfortunately that warning is suppressed when it occurs in a
macro-expansion, so the llist_for_each_entry error was silent.

(OTOH, I _do_ expect clang to eventually gain support for a flag to treat
NULL-pointer deref as a legal operation instead of UB. I'm not really sure
it makes sense for the linux kernel, but it's a useful feature for the
compiler to provide in any case, so why not...)

Re: clang asm-goto support (Was Re: [PATCH v2] x86/retpoline: Add clang support)

[James Y Knight]

I'd be definitely in favor having clang support asm goto. I wouldn't
want to exclude having other conversations about how to more directly
provide compiler features that the linux kernel could use, ALSO, but I
do not think that conversation should block implementing asm-goto.

IMO, inline asm is, generally, a valuable feature to provide in the
compiler as an escape hatch, and asm goto is a relatively sane
extension of it. Supporting outgoing edges from an inline asm block is
a reasonable thing for users to desire, and as far as anyone's said so
far, seems like it ought to be fairly easily implementable, without
causing bad side-effects in the compiler.

Of course, we generally do want to minimize the need for users to use
inline asm, by providing appropriate compiler support for the features
people would otherwise be forced to implement using asm. But I don't
see that as really any more important for asm-goto than any other
inline asm. There will always be a desire for escape hatches, to do
weird and unique things which aren't supported directly in the
compiler. (Also, the kernel is a pretty special case in terms of its
requirements, it seems exceedingly unlikely that we could ever provide
enough compiler support to let it eliminate inline asm.).

On Wed, Feb 14, 2018 at 3:33 AM, Yatsina, Marina
 wrote:
>
> Hi Kees,
>
> When I raised the question of whether we want to add support for "asm goto" 
> in llvm I got some feedback from the community that "asm goto" might not be 
> the best solution for the problem it was invented for (optimizing support for 
> tracepoints), so I stopped perusing this issue.
> I'm CC-ing the developers that participated in the original thread and a few 
> developers that might be interested in adding support of "asm goto".
> I'm also adding the llvm-dev mailing list, in case there are additional 
> parties interested in voicing their opinion.
>
> I hope this will give this issue a push forward and we will find a solution 
> that will not prevent llvm from compiling linux kernel.
>
> Thanks,
> Marina
>
> -Original Message-
> From: Kees Cook [mailto:keesc...@google.com]
> Sent: Wednesday, February 14, 2018 02:29
> To: David Woodhouse ; Chandler Carruth 
> ; Yatsina, Marina 
> Cc: Guenter Roeck ; X86 ML ; LKML 
> ; Alan Cox ; Rik 
> van Riel ; Andi Kleen ; Josh Poimboeuf 
> ; Tom Lendacky ; Peter Zijlstra 
> ; Linus Torvalds ; Jiri 
> Kosina ; Andy Lutomirski ; Hansen, 
> Dave ; Tim Chen ; Greg 
> Kroah-Hartman ; Paul Turner ; 
> Stephen Hines ; Nick Desaulniers 
> Subject: clang asm-goto support (Was Re: [PATCH v2] x86/retpoline: Add clang 
> support)
>
> On Tue, Feb 13, 2018 at 4:10 PM, David Woodhouse  wrote:
> > We also need to resolve the asm-goto thing.
>
> Yes, this is becoming much more urgent, assuming we'll be raising the minimum 
> GCC version soon and drop support for lacking asm-goto...
>
> Do you happen to know who the right people are to include to move the 
> discussion forward? I know various kernel folks that are passionate about it, 
> but I'm still getting to know who to talk with from llvm.
>
> I see an earlier thread here:
> http://lists.llvm.org/pipermail/llvm-dev/2017-April/111748.html
>
> It seems to end there? I'm still coming up to speed on it, so I'm likely 
> missing other context.
>
> -Kees
>
> --
> Kees Cook
> Pixel Security
> -
> Intel Israel (74) Limited
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.

Re: clang asm-goto support (Was Re: [PATCH v2] x86/retpoline: Add clang support)

[James Y Knight]

I'd be definitely in favor having clang support asm goto. I wouldn't
want to exclude having other conversations about how to more directly
provide compiler features that the linux kernel could use, ALSO, but I
do not think that conversation should block implementing asm-goto.

IMO, inline asm is, generally, a valuable feature to provide in the
compiler as an escape hatch, and asm goto is a relatively sane
extension of it. Supporting outgoing edges from an inline asm block is
a reasonable thing for users to desire, and as far as anyone's said so
far, seems like it ought to be fairly easily implementable, without
causing bad side-effects in the compiler.

Of course, we generally do want to minimize the need for users to use
inline asm, by providing appropriate compiler support for the features
people would otherwise be forced to implement using asm. But I don't
see that as really any more important for asm-goto than any other
inline asm. There will always be a desire for escape hatches, to do
weird and unique things which aren't supported directly in the
compiler. (Also, the kernel is a pretty special case in terms of its
requirements, it seems exceedingly unlikely that we could ever provide
enough compiler support to let it eliminate inline asm.).

On Wed, Feb 14, 2018 at 3:33 AM, Yatsina, Marina
 wrote:
>
> Hi Kees,
>
> When I raised the question of whether we want to add support for "asm goto" 
> in llvm I got some feedback from the community that "asm goto" might not be 
> the best solution for the problem it was invented for (optimizing support for 
> tracepoints), so I stopped perusing this issue.
> I'm CC-ing the developers that participated in the original thread and a few 
> developers that might be interested in adding support of "asm goto".
> I'm also adding the llvm-dev mailing list, in case there are additional 
> parties interested in voicing their opinion.
>
> I hope this will give this issue a push forward and we will find a solution 
> that will not prevent llvm from compiling linux kernel.
>
> Thanks,
> Marina
>
> -Original Message-
> From: Kees Cook [mailto:keesc...@google.com]
> Sent: Wednesday, February 14, 2018 02:29
> To: David Woodhouse ; Chandler Carruth 
> ; Yatsina, Marina 
> Cc: Guenter Roeck ; X86 ML ; LKML 
> ; Alan Cox ; Rik 
> van Riel ; Andi Kleen ; Josh Poimboeuf 
> ; Tom Lendacky ; Peter Zijlstra 
> ; Linus Torvalds ; Jiri 
> Kosina ; Andy Lutomirski ; Hansen, 
> Dave ; Tim Chen ; Greg 
> Kroah-Hartman ; Paul Turner ; 
> Stephen Hines ; Nick Desaulniers 
> Subject: clang asm-goto support (Was Re: [PATCH v2] x86/retpoline: Add clang 
> support)
>
> On Tue, Feb 13, 2018 at 4:10 PM, David Woodhouse  wrote:
> > We also need to resolve the asm-goto thing.
>
> Yes, this is becoming much more urgent, assuming we'll be raising the minimum 
> GCC version soon and drop support for lacking asm-goto...
>
> Do you happen to know who the right people are to include to move the 
> discussion forward? I know various kernel folks that are passionate about it, 
> but I'm still getting to know who to talk with from llvm.
>
> I see an earlier thread here:
> http://lists.llvm.org/pipermail/llvm-dev/2017-April/111748.html
>
> It seems to end there? I'm still coming up to speed on it, so I'm likely 
> missing other context.
>
> -Kees
>
> --
> Kees Cook
> Pixel Security
> -
> Intel Israel (74) Limited
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.

Re: [llvm-dev] [isocpp-parallel] Proposal for new memory_order_consume definition

[James Y Knight]

No, you really don't need undefined behavior in the standard in order
to enable bug-finding.

The standard could've (and still could...) make signed integer
overflow "implementation-defined" rather than "undefined". Compilers
would thus be required to have *some documented meaning* for it (e.g.
wrap 2's-complement, wrap 1's-complement, saturate to min/max, trap,
or whatever...), but must not have the current "Anything goes! I can
set your cat on fire if the optimizer feels like it today!" behavior.

Such a change to the standard would not reduce any ability to do error
checking, as compilers that want to be helpful could perfectly-well
define it to trap at runtime when given certain compiler flags, and
perfectly well warn you of your dependence upon unportable
implementation-defined behavior (or, that your program is going to
trap), at build-time.

[Sending again as a plain-text email, since a bunch of mailing lists
apparently hate on multipart messages that even contain a text/html
part...]

On Mon, Feb 29, 2016 at 2:38 PM, Lawrence Crowl via llvm-dev
 wrote:
> On 2/28/16, Linus Torvalds  wrote:
>> The fact is, undefined compiler behavior is never a good idea. Not for
>> serious projects.
>
> Actually, undefined behavior is essential for serious projects, but
> not for the reasons mentioned.
>
> If the language has no undefined behavior, then from the compiler's view,
> there is no such thing as a bad program.  All programs will compile and
> enter functional debug (possibly after shipping to customer).  On the
> other hand, a language with undefined behavior makes it possible for
> compilers (and their run-time support) to identify a program as wrong.
>
> The problem with the latest spate of compiler optimizations was not the
> optimization, but the lack of warnings about exploiting undefined behavior.
>
> --
> Lawrence Crowl
> ___
> LLVM Developers mailing list
> llvm-...@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

Re: [llvm-dev] [isocpp-parallel] Proposal for new memory_order_consume definition

[James Y Knight]

No, you really don't need undefined behavior in the standard in order
to enable bug-finding.

The standard could've (and still could...) make signed integer
overflow "implementation-defined" rather than "undefined". Compilers
would thus be required to have *some documented meaning* for it (e.g.
wrap 2's-complement, wrap 1's-complement, saturate to min/max, trap,
or whatever...), but must not have the current "Anything goes! I can
set your cat on fire if the optimizer feels like it today!" behavior.

Such a change to the standard would not reduce any ability to do error
checking, as compilers that want to be helpful could perfectly-well
define it to trap at runtime when given certain compiler flags, and
perfectly well warn you of your dependence upon unportable
implementation-defined behavior (or, that your program is going to
trap), at build-time.

[Sending again as a plain-text email, since a bunch of mailing lists
apparently hate on multipart messages that even contain a text/html
part...]

On Mon, Feb 29, 2016 at 2:38 PM, Lawrence Crowl via llvm-dev
 wrote:
> On 2/28/16, Linus Torvalds  wrote:
>> The fact is, undefined compiler behavior is never a good idea. Not for
>> serious projects.
>
> Actually, undefined behavior is essential for serious projects, but
> not for the reasons mentioned.
>
> If the language has no undefined behavior, then from the compiler's view,
> there is no such thing as a bad program.  All programs will compile and
> enter functional debug (possibly after shipping to customer).  On the
> other hand, a language with undefined behavior makes it possible for
> compilers (and their run-time support) to identify a program as wrong.
>
> The problem with the latest spate of compiler optimizations was not the
> optimization, but the lack of warnings about exploiting undefined behavior.
>
> --
> Lawrence Crowl
> ___
> LLVM Developers mailing list
> llvm-...@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev