linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] genksyms: Ignore module scoped _Static_assert()
@ 2020-12-01 15:20 Marco Elver
  2020-12-01 16:14 ` Christoph Hellwig
  2020-12-01 19:59 ` Nick Desaulniers
  0 siblings, 2 replies; 14+ messages in thread
From: Marco Elver @ 2020-12-01 15:20 UTC (permalink / raw)
  To: elver; +Cc: linux-kernel, kasan-dev, masahiroy, ndesaulniers, joe

The C11 _Static_assert() keyword may be used at module scope, and we
need to teach genksyms about it to not abort with an error. We currently
have a growing number of static_assert() (but also direct usage of
_Static_assert()) users at module scope:

	git grep -E '^_Static_assert\(|^static_assert\(' | grep -v '^tools' | wc -l
	135

More recently, when enabling CONFIG_MODVERSIONS with CONFIG_KCSAN, we
observe a number of warnings:

	WARNING: modpost: EXPORT symbol "<..all kcsan symbols..>" [vmlinux] [...]

When running a preprocessed source through 'genksyms -w' a number of
syntax errors point at usage of static_assert()s. In the case of
kernel/kcsan/encoding.h, new static_assert()s had been introduced which
used expressions that appear to cause genksyms to not even be able to
recover from the syntax error gracefully (as it appears was the case
previously).

Therefore, make genksyms ignore all _Static_assert() and the contained
expression. With the fix, usage of _Static_assert() no longer cause
"syntax error" all over the kernel, and the above modpost warnings for
KCSAN are gone, too.

Signed-off-by: Marco Elver <elver@google.com>
---
 scripts/genksyms/keywords.c |  3 +++
 scripts/genksyms/lex.l      | 27 ++++++++++++++++++++++++++-
 scripts/genksyms/parse.y    |  7 +++++++
 3 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/scripts/genksyms/keywords.c b/scripts/genksyms/keywords.c
index 057c6cabad1d..b85e0979a00c 100644
--- a/scripts/genksyms/keywords.c
+++ b/scripts/genksyms/keywords.c
@@ -32,6 +32,9 @@ static struct resword {
 	{ "restrict", RESTRICT_KEYW },
 	{ "asm", ASM_KEYW },
 
+	// c11 keywords that can be used at module scope
+	{ "_Static_assert", STATIC_ASSERT_KEYW },
+
 	// attribute commented out in modutils 2.4.2.  People are using 'attribute' as a
 	// field name which breaks the genksyms parser.  It is not a gcc keyword anyway.
 	// KAO. },
diff --git a/scripts/genksyms/lex.l b/scripts/genksyms/lex.l
index e265c5d96861..ae76472efc43 100644
--- a/scripts/genksyms/lex.l
+++ b/scripts/genksyms/lex.l
@@ -118,7 +118,7 @@ yylex(void)
 {
   static enum {
     ST_NOTSTARTED, ST_NORMAL, ST_ATTRIBUTE, ST_ASM, ST_TYPEOF, ST_TYPEOF_1,
-    ST_BRACKET, ST_BRACE, ST_EXPRESSION,
+    ST_BRACKET, ST_BRACE, ST_EXPRESSION, ST_STATIC_ASSERT,
     ST_TABLE_1, ST_TABLE_2, ST_TABLE_3, ST_TABLE_4,
     ST_TABLE_5, ST_TABLE_6
   } lexstate = ST_NOTSTARTED;
@@ -201,6 +201,11 @@ repeat:
 
 		  case EXPORT_SYMBOL_KEYW:
 		      goto fini;
+
+		  case STATIC_ASSERT_KEYW:
+		    lexstate = ST_STATIC_ASSERT;
+		    count = 0;
+		    goto repeat;
 		  }
 	      }
 	    if (!suppress_type_lookup)
@@ -401,6 +406,26 @@ repeat:
 	}
       break;
 
+    case ST_STATIC_ASSERT:
+      APP;
+      switch (token)
+	{
+	case '(':
+	  ++count;
+	  goto repeat;
+	case ')':
+	  if (--count == 0)
+	    {
+	      lexstate = ST_NORMAL;
+	      token = STATIC_ASSERT_PHRASE;
+	      break;
+	    }
+	  goto repeat;
+	default:
+	  goto repeat;
+	}
+      break;
+
     case ST_TABLE_1:
       goto repeat;
 
diff --git a/scripts/genksyms/parse.y b/scripts/genksyms/parse.y
index e22b42245bcc..8e9b5e69e8f0 100644
--- a/scripts/genksyms/parse.y
+++ b/scripts/genksyms/parse.y
@@ -80,6 +80,7 @@ static void record_compound(struct string_list **keyw,
 %token SHORT_KEYW
 %token SIGNED_KEYW
 %token STATIC_KEYW
+%token STATIC_ASSERT_KEYW
 %token STRUCT_KEYW
 %token TYPEDEF_KEYW
 %token UNION_KEYW
@@ -97,6 +98,7 @@ static void record_compound(struct string_list **keyw,
 %token BRACE_PHRASE
 %token BRACKET_PHRASE
 %token EXPRESSION_PHRASE
+%token STATIC_ASSERT_PHRASE
 
 %token CHAR
 %token DOTS
@@ -130,6 +132,7 @@ declaration1:
 	| function_definition
 	| asm_definition
 	| export_definition
+	| static_assert
 	| error ';'				{ $$ = $2; }
 	| error '}'				{ $$ = $2; }
 	;
@@ -493,6 +496,10 @@ export_definition:
 		{ export_symbol((*$3)->string); $$ = $5; }
 	;
 
+/* Ignore any module scoped _Static_assert(...) */
+static_assert:
+	STATIC_ASSERT_PHRASE ';'			{ $$ = $2; }
+	;
 
 %%
 
-- 
2.29.2.454.gaff20da3a2-goog


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] genksyms: Ignore module scoped _Static_assert()
  2020-12-01 15:20 [PATCH] genksyms: Ignore module scoped _Static_assert() Marco Elver
@ 2020-12-01 16:14 ` Christoph Hellwig
  2020-12-01 17:04   ` Marco Elver
  2020-12-01 19:59 ` Nick Desaulniers
  1 sibling, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2020-12-01 16:14 UTC (permalink / raw)
  To: Marco Elver; +Cc: linux-kernel, kasan-dev, masahiroy, ndesaulniers, joe

Why not use the kernels own BUILD_BUG_ON instead of this idiom?

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] genksyms: Ignore module scoped _Static_assert()
  2020-12-01 16:14 ` Christoph Hellwig
@ 2020-12-01 17:04   ` Marco Elver
  2020-12-01 19:56     ` Nick Desaulniers
  0 siblings, 1 reply; 14+ messages in thread
From: Marco Elver @ 2020-12-01 17:04 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-kernel, kasan-dev, masahiroy, ndesaulniers, joe

On Tue, Dec 01, 2020 at 04:14PM +0000, Christoph Hellwig wrote:
> Why not use the kernels own BUILD_BUG_ON instead of this idiom?

BUILD_BUG_ON() was conceived before there was builtin compiler-support
in the form of _Static_assert() (static_assert()), which has several
advantages (compile-time performance, optional message) but most
importantly, that it can be used at module/global scope (which
BUILD_BUG_ON() cannot).

From include/linux/build_bug:

	/**
	 * static_assert - check integer constant expression at build time
	 *
	 [...]
	 *
	 * Contrary to BUILD_BUG_ON(), static_assert() can be used at global
	 * scope, but requires the expression to be an integer constant
	 * expression (i.e., it is not enough that __builtin_constant_p() is
	 * true for expr).
	 [...]

.. and there are plenty of global/module scoped users of it already.

Thanks,
-- Marco

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] genksyms: Ignore module scoped _Static_assert()
  2020-12-01 17:04   ` Marco Elver
@ 2020-12-01 19:56     ` Nick Desaulniers
  2020-12-01 21:01       ` Rasmus Villemoes
  0 siblings, 1 reply; 14+ messages in thread
From: Nick Desaulniers @ 2020-12-01 19:56 UTC (permalink / raw)
  To: Marco Elver
  Cc: Christoph Hellwig, LKML, kasan-dev, Masahiro Yamada, Joe Perches,
	George Burgess, Rasmus Villemoes

On Tue, Dec 1, 2020 at 9:04 AM Marco Elver <elver@google.com> wrote:
>
> On Tue, Dec 01, 2020 at 04:14PM +0000, Christoph Hellwig wrote:
> > Why not use the kernels own BUILD_BUG_ON instead of this idiom?
>
> BUILD_BUG_ON() was conceived before there was builtin compiler-support
> in the form of _Static_assert() (static_assert()), which has several
> advantages (compile-time performance, optional message) but most
> importantly, that it can be used at module/global scope (which
> BUILD_BUG_ON() cannot).
>
> From include/linux/build_bug:
>
>         /**
>          * static_assert - check integer constant expression at build time
>          *
>          [...]
>          *
>          * Contrary to BUILD_BUG_ON(), static_assert() can be used at global
>          * scope, but requires the expression to be an integer constant
>          * expression (i.e., it is not enough that __builtin_constant_p() is
>          * true for expr).
>          [...]
>
> .. and there are plenty of global/module scoped users of it already.

And to proactively address the inevitable: why do we have both?  We
looked into wholesale replacing BUILD_BUG_ON's implementation with
_Static_assert, but found that they differ slightly in the handling of
integer constant expressions; BUILD_BUG_ON was reliant on some
compiler optimizations in expressions making use of
__builtin_constant_p that cannot be evaluated when the compiler
performs the _Static_assert check.  So the current implementation is
more flexible for expressions that use __builtin_constant_p than
_Static_assert is.  If we needed a rule of thumb, I'd recommend "use
_Static_assert unless you're passing an expression that relies on
__builtin_constant_p evaluation, at which point BUILD_BUG_ON must be
used."
-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] genksyms: Ignore module scoped _Static_assert()
  2020-12-01 15:20 [PATCH] genksyms: Ignore module scoped _Static_assert() Marco Elver
  2020-12-01 16:14 ` Christoph Hellwig
@ 2020-12-01 19:59 ` Nick Desaulniers
  2020-12-01 20:12   ` Marco Elver
  2020-12-04 10:21   ` Marco Elver
  1 sibling, 2 replies; 14+ messages in thread
From: Nick Desaulniers @ 2020-12-01 19:59 UTC (permalink / raw)
  To: Marco Elver; +Cc: LKML, kasan-dev, Masahiro Yamada, Joe Perches

On Tue, Dec 1, 2020 at 7:21 AM Marco Elver <elver@google.com> wrote:
>
> The C11 _Static_assert() keyword may be used at module scope, and we
> need to teach genksyms about it to not abort with an error. We currently
> have a growing number of static_assert() (but also direct usage of
> _Static_assert()) users at module scope:
>
>         git grep -E '^_Static_assert\(|^static_assert\(' | grep -v '^tools' | wc -l
>         135
>
> More recently, when enabling CONFIG_MODVERSIONS with CONFIG_KCSAN, we
> observe a number of warnings:
>
>         WARNING: modpost: EXPORT symbol "<..all kcsan symbols..>" [vmlinux] [...]
>
> When running a preprocessed source through 'genksyms -w' a number of
> syntax errors point at usage of static_assert()s. In the case of
> kernel/kcsan/encoding.h, new static_assert()s had been introduced which
> used expressions that appear to cause genksyms to not even be able to
> recover from the syntax error gracefully (as it appears was the case
> previously).
>
> Therefore, make genksyms ignore all _Static_assert() and the contained
> expression. With the fix, usage of _Static_assert() no longer cause
> "syntax error" all over the kernel, and the above modpost warnings for
> KCSAN are gone, too.
>
> Signed-off-by: Marco Elver <elver@google.com>

Ah, genksyms...if only there were a library that we could use to parse
C code...:P
Acked-by: Nick Desaulniers <ndesaulniers@google.com>

> ---
>  scripts/genksyms/keywords.c |  3 +++
>  scripts/genksyms/lex.l      | 27 ++++++++++++++++++++++++++-
>  scripts/genksyms/parse.y    |  7 +++++++
>  3 files changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/genksyms/keywords.c b/scripts/genksyms/keywords.c
> index 057c6cabad1d..b85e0979a00c 100644
> --- a/scripts/genksyms/keywords.c
> +++ b/scripts/genksyms/keywords.c
> @@ -32,6 +32,9 @@ static struct resword {
>         { "restrict", RESTRICT_KEYW },
>         { "asm", ASM_KEYW },
>
> +       // c11 keywords that can be used at module scope
> +       { "_Static_assert", STATIC_ASSERT_KEYW },
> +
>         // attribute commented out in modutils 2.4.2.  People are using 'attribute' as a
>         // field name which breaks the genksyms parser.  It is not a gcc keyword anyway.
>         // KAO. },
> diff --git a/scripts/genksyms/lex.l b/scripts/genksyms/lex.l
> index e265c5d96861..ae76472efc43 100644
> --- a/scripts/genksyms/lex.l
> +++ b/scripts/genksyms/lex.l
> @@ -118,7 +118,7 @@ yylex(void)
>  {
>    static enum {
>      ST_NOTSTARTED, ST_NORMAL, ST_ATTRIBUTE, ST_ASM, ST_TYPEOF, ST_TYPEOF_1,
> -    ST_BRACKET, ST_BRACE, ST_EXPRESSION,
> +    ST_BRACKET, ST_BRACE, ST_EXPRESSION, ST_STATIC_ASSERT,
>      ST_TABLE_1, ST_TABLE_2, ST_TABLE_3, ST_TABLE_4,
>      ST_TABLE_5, ST_TABLE_6
>    } lexstate = ST_NOTSTARTED;
> @@ -201,6 +201,11 @@ repeat:
>
>                   case EXPORT_SYMBOL_KEYW:
>                       goto fini;
> +
> +                 case STATIC_ASSERT_KEYW:
> +                   lexstate = ST_STATIC_ASSERT;
> +                   count = 0;
> +                   goto repeat;
>                   }
>               }
>             if (!suppress_type_lookup)
> @@ -401,6 +406,26 @@ repeat:
>         }
>        break;
>
> +    case ST_STATIC_ASSERT:
> +      APP;
> +      switch (token)
> +       {
> +       case '(':
> +         ++count;
> +         goto repeat;
> +       case ')':
> +         if (--count == 0)
> +           {
> +             lexstate = ST_NORMAL;
> +             token = STATIC_ASSERT_PHRASE;
> +             break;
> +           }
> +         goto repeat;
> +       default:
> +         goto repeat;
> +       }
> +      break;
> +
>      case ST_TABLE_1:
>        goto repeat;
>
> diff --git a/scripts/genksyms/parse.y b/scripts/genksyms/parse.y
> index e22b42245bcc..8e9b5e69e8f0 100644
> --- a/scripts/genksyms/parse.y
> +++ b/scripts/genksyms/parse.y
> @@ -80,6 +80,7 @@ static void record_compound(struct string_list **keyw,
>  %token SHORT_KEYW
>  %token SIGNED_KEYW
>  %token STATIC_KEYW
> +%token STATIC_ASSERT_KEYW
>  %token STRUCT_KEYW
>  %token TYPEDEF_KEYW
>  %token UNION_KEYW
> @@ -97,6 +98,7 @@ static void record_compound(struct string_list **keyw,
>  %token BRACE_PHRASE
>  %token BRACKET_PHRASE
>  %token EXPRESSION_PHRASE
> +%token STATIC_ASSERT_PHRASE
>
>  %token CHAR
>  %token DOTS
> @@ -130,6 +132,7 @@ declaration1:
>         | function_definition
>         | asm_definition
>         | export_definition
> +       | static_assert
>         | error ';'                             { $$ = $2; }
>         | error '}'                             { $$ = $2; }
>         ;
> @@ -493,6 +496,10 @@ export_definition:
>                 { export_symbol((*$3)->string); $$ = $5; }
>         ;
>
> +/* Ignore any module scoped _Static_assert(...) */
> +static_assert:
> +       STATIC_ASSERT_PHRASE ';'                        { $$ = $2; }
> +       ;
>
>  %%
>
> --
> 2.29.2.454.gaff20da3a2-goog
>


-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] genksyms: Ignore module scoped _Static_assert()
  2020-12-01 19:59 ` Nick Desaulniers
@ 2020-12-01 20:12   ` Marco Elver
  2020-12-04 10:21   ` Marco Elver
  1 sibling, 0 replies; 14+ messages in thread
From: Marco Elver @ 2020-12-01 20:12 UTC (permalink / raw)
  To: Nick Desaulniers; +Cc: LKML, kasan-dev, Masahiro Yamada, Joe Perches

On Tue, 1 Dec 2020 at 21:00, Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> On Tue, Dec 1, 2020 at 7:21 AM Marco Elver <elver@google.com> wrote:
> >
> > The C11 _Static_assert() keyword may be used at module scope, and we
> > need to teach genksyms about it to not abort with an error. We currently
> > have a growing number of static_assert() (but also direct usage of
> > _Static_assert()) users at module scope:
> >
> >         git grep -E '^_Static_assert\(|^static_assert\(' | grep -v '^tools' | wc -l
> >         135
> >
> > More recently, when enabling CONFIG_MODVERSIONS with CONFIG_KCSAN, we
> > observe a number of warnings:
> >
> >         WARNING: modpost: EXPORT symbol "<..all kcsan symbols..>" [vmlinux] [...]
> >
> > When running a preprocessed source through 'genksyms -w' a number of
> > syntax errors point at usage of static_assert()s. In the case of
> > kernel/kcsan/encoding.h, new static_assert()s had been introduced which
> > used expressions that appear to cause genksyms to not even be able to
> > recover from the syntax error gracefully (as it appears was the case
> > previously).
> >
> > Therefore, make genksyms ignore all _Static_assert() and the contained
> > expression. With the fix, usage of _Static_assert() no longer cause
> > "syntax error" all over the kernel, and the above modpost warnings for
> > KCSAN are gone, too.
> >
> > Signed-off-by: Marco Elver <elver@google.com>
>
> Ah, genksyms...if only there were a library that we could use to parse
> C code...:P

Hehe -- another usecase for using that library ;-)  If only we could
require LLVM be present even when building the rest of the kernel with
GCC.

I guess this works, for now. Until the next new keyword that is used
at module scope...

> Acked-by: Nick Desaulniers <ndesaulniers@google.com>

Thanks!

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] genksyms: Ignore module scoped _Static_assert()
  2020-12-01 19:56     ` Nick Desaulniers
@ 2020-12-01 21:01       ` Rasmus Villemoes
  0 siblings, 0 replies; 14+ messages in thread
From: Rasmus Villemoes @ 2020-12-01 21:01 UTC (permalink / raw)
  To: Nick Desaulniers, Marco Elver
  Cc: Christoph Hellwig, LKML, kasan-dev, Masahiro Yamada, Joe Perches,
	George Burgess

On 01/12/2020 20.56, Nick Desaulniers wrote:
> On Tue, Dec 1, 2020 at 9:04 AM Marco Elver <elver@google.com> wrote:
>>
>> On Tue, Dec 01, 2020 at 04:14PM +0000, Christoph Hellwig wrote:
>>> Why not use the kernels own BUILD_BUG_ON instead of this idiom?
>>
> And to proactively address the inevitable: why do we have both?  We
> looked into wholesale replacing BUILD_BUG_ON's implementation with
> _Static_assert, but found that they differ slightly in the handling of
> integer constant expressions; BUILD_BUG_ON was reliant on some
> compiler optimizations in expressions making use of
> __builtin_constant_p that cannot be evaluated when the compiler
> performs the _Static_assert check. 

... and _Static_assert() is a declaration, so even many of the
BUILD_BUG_ON() that have a bona fide ICE cannot be converted because
declaration-after-statement.

Rasmus

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] genksyms: Ignore module scoped _Static_assert()
  2020-12-01 19:59 ` Nick Desaulniers
  2020-12-01 20:12   ` Marco Elver
@ 2020-12-04 10:21   ` Marco Elver
  2020-12-10 10:35     ` Marco Elver
  1 sibling, 1 reply; 14+ messages in thread
From: Marco Elver @ 2020-12-04 10:21 UTC (permalink / raw)
  To: Nick Desaulniers; +Cc: LKML, kasan-dev, Masahiro Yamada, Joe Perches

On Tue, 1 Dec 2020 at 21:00, Nick Desaulniers <ndesaulniers@google.com> wrote:
> On Tue, Dec 1, 2020 at 7:21 AM Marco Elver <elver@google.com> wrote:
> > The C11 _Static_assert() keyword may be used at module scope, and we
> > need to teach genksyms about it to not abort with an error. We currently
> > have a growing number of static_assert() (but also direct usage of
> > _Static_assert()) users at module scope:
> >
> >         git grep -E '^_Static_assert\(|^static_assert\(' | grep -v '^tools' | wc -l
> >         135
> >
> > More recently, when enabling CONFIG_MODVERSIONS with CONFIG_KCSAN, we
> > observe a number of warnings:
> >
> >         WARNING: modpost: EXPORT symbol "<..all kcsan symbols..>" [vmlinux] [...]
> >
> > When running a preprocessed source through 'genksyms -w' a number of
> > syntax errors point at usage of static_assert()s. In the case of
> > kernel/kcsan/encoding.h, new static_assert()s had been introduced which
> > used expressions that appear to cause genksyms to not even be able to
> > recover from the syntax error gracefully (as it appears was the case
> > previously).
> >
> > Therefore, make genksyms ignore all _Static_assert() and the contained
> > expression. With the fix, usage of _Static_assert() no longer cause
> > "syntax error" all over the kernel, and the above modpost warnings for
> > KCSAN are gone, too.
> >
> > Signed-off-by: Marco Elver <elver@google.com>
>
> Ah, genksyms...if only there were a library that we could use to parse
> C code...:P
> Acked-by: Nick Desaulniers <ndesaulniers@google.com>

Which tree would this go into?

It'd be good if this problem could be fixed for 5.11.

Thanks,
-- Marco

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] genksyms: Ignore module scoped _Static_assert()
  2020-12-04 10:21   ` Marco Elver
@ 2020-12-10 10:35     ` Marco Elver
  2020-12-10 13:29       ` Miguel Ojeda
  0 siblings, 1 reply; 14+ messages in thread
From: Marco Elver @ 2020-12-10 10:35 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: LKML, kasan-dev, Masahiro Yamada, Joe Perches,
	Greg Kroah-Hartman, Miguel Ojeda

On Fri, 4 Dec 2020 at 11:21, Marco Elver <elver@google.com> wrote:
> On Tue, 1 Dec 2020 at 21:00, Nick Desaulniers <ndesaulniers@google.com> wrote:
> > On Tue, Dec 1, 2020 at 7:21 AM Marco Elver <elver@google.com> wrote:
> > > The C11 _Static_assert() keyword may be used at module scope, and we
> > > need to teach genksyms about it to not abort with an error. We currently
> > > have a growing number of static_assert() (but also direct usage of
> > > _Static_assert()) users at module scope:
> > >
> > >         git grep -E '^_Static_assert\(|^static_assert\(' | grep -v '^tools' | wc -l
> > >         135
> > >
> > > More recently, when enabling CONFIG_MODVERSIONS with CONFIG_KCSAN, we
> > > observe a number of warnings:
> > >
> > >         WARNING: modpost: EXPORT symbol "<..all kcsan symbols..>" [vmlinux] [...]
> > >
> > > When running a preprocessed source through 'genksyms -w' a number of
> > > syntax errors point at usage of static_assert()s. In the case of
> > > kernel/kcsan/encoding.h, new static_assert()s had been introduced which
> > > used expressions that appear to cause genksyms to not even be able to
> > > recover from the syntax error gracefully (as it appears was the case
> > > previously).
> > >
> > > Therefore, make genksyms ignore all _Static_assert() and the contained
> > > expression. With the fix, usage of _Static_assert() no longer cause
> > > "syntax error" all over the kernel, and the above modpost warnings for
> > > KCSAN are gone, too.
> > >
> > > Signed-off-by: Marco Elver <elver@google.com>
> >
> > Ah, genksyms...if only there were a library that we could use to parse
> > C code...:P
> > Acked-by: Nick Desaulniers <ndesaulniers@google.com>
>
> Which tree would this go into?
>
> It'd be good if this problem could be fixed for 5.11.

[+Cc everyone returned by 'get_maintainers.pl scripts/genksyms']

It looks like there's no clear MAINTAINER for this. :-/
It'd still be good to fix this for 5.11.

Thanks,
-- Marco

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] genksyms: Ignore module scoped _Static_assert()
  2020-12-10 10:35     ` Marco Elver
@ 2020-12-10 13:29       ` Miguel Ojeda
  2020-12-10 16:25         ` Marco Elver
  0 siblings, 1 reply; 14+ messages in thread
From: Miguel Ojeda @ 2020-12-10 13:29 UTC (permalink / raw)
  To: Marco Elver
  Cc: Nick Desaulniers, LKML, kasan-dev, Masahiro Yamada, Joe Perches,
	Greg Kroah-Hartman, richard.henderson, Andrew Morton

On Thu, Dec 10, 2020 at 11:35 AM Marco Elver <elver@google.com> wrote:
>
> It looks like there's no clear MAINTAINER for this. :-/
> It'd still be good to fix this for 5.11.

Richard seems to be the author, not sure if he picks patches (CC'd).

I guess Masahiro or akpm (Cc'd) would be two options; otherwise, I
could pick it up through compiler attributes (stretching the
definition...).

Cheers,
Miguel

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] genksyms: Ignore module scoped _Static_assert()
  2020-12-10 13:29       ` Miguel Ojeda
@ 2020-12-10 16:25         ` Marco Elver
  2020-12-10 21:48           ` Nick Desaulniers
  2020-12-11  5:24           ` Andrew Morton
  0 siblings, 2 replies; 14+ messages in thread
From: Marco Elver @ 2020-12-10 16:25 UTC (permalink / raw)
  To: Miguel Ojeda
  Cc: Nick Desaulniers, LKML, kasan-dev, Masahiro Yamada, Joe Perches,
	Greg Kroah-Hartman, Richard Henderson, Andrew Morton

On Thu, 10 Dec 2020 at 14:29, Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
> On Thu, Dec 10, 2020 at 11:35 AM Marco Elver <elver@google.com> wrote:
> >
> > It looks like there's no clear MAINTAINER for this. :-/
> > It'd still be good to fix this for 5.11.
>
> Richard seems to be the author, not sure if he picks patches (CC'd).
>
> I guess Masahiro or akpm (Cc'd) would be two options; otherwise, I
> could pick it up through compiler attributes (stretching the
> definition...).

Thanks for the info. I did find that there's an alternative patch to
fix _Static_assert() with genksyms that was sent 3 days after mine
(it's simpler, but might miss cases). I've responded there (
https://lkml.kernel.org/r/X9JI5KpWoo23wkRg@elver.google.com ).

Now we have some choice. I'd argue for this patch, because it's not
doing preprocessor workarounds, but in the end I won't make that call.
:-)

Thanks,
-- Marco

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] genksyms: Ignore module scoped _Static_assert()
  2020-12-10 16:25         ` Marco Elver
@ 2020-12-10 21:48           ` Nick Desaulniers
  2020-12-11  5:24           ` Andrew Morton
  1 sibling, 0 replies; 14+ messages in thread
From: Nick Desaulniers @ 2020-12-10 21:48 UTC (permalink / raw)
  To: Marco Elver
  Cc: Miguel Ojeda, LKML, kasan-dev, Masahiro Yamada, Joe Perches,
	Greg Kroah-Hartman, Richard Henderson, Andrew Morton

On Thu, Dec 10, 2020 at 8:25 AM Marco Elver <elver@google.com> wrote:
>
> On Thu, 10 Dec 2020 at 14:29, Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
> > On Thu, Dec 10, 2020 at 11:35 AM Marco Elver <elver@google.com> wrote:
> > >
> > > It looks like there's no clear MAINTAINER for this. :-/
> > > It'd still be good to fix this for 5.11.
> >
> > Richard seems to be the author, not sure if he picks patches (CC'd).
> >
> > I guess Masahiro or akpm (Cc'd) would be two options; otherwise, I
> > could pick it up through compiler attributes (stretching the
> > definition...).
>
> Thanks for the info. I did find that there's an alternative patch to
> fix _Static_assert() with genksyms that was sent 3 days after mine
> (it's simpler, but might miss cases). I've responded there (
> https://lkml.kernel.org/r/X9JI5KpWoo23wkRg@elver.google.com ).
>
> Now we have some choice. I'd argue for this patch, because it's not
> doing preprocessor workarounds, but in the end I won't make that call.
> :-)

I was half kidding about depending on a production C parser.  See
internal reference pa/1432607, choice quotes:
```
...
CONFIG_MODVERSIONS uses scripts/genksyms/genksyms to create a file,
Module.symvers, that is a simple mapping of CRCs of various symbols'
types to the symbol names.  It produces these CRCs by using the C
preprocessor, then passing this into genksyms. genksyms has a lex/yacc
based C parser to parse the preprocessed sources of kernel modules.  It
turns out that it's incomplete, copied from an upstream project that
ceased development in 2013, and was slated to be removed around the 4.9
kernel release.
...
Some possible solutions:
* Update the kernel's version of genksyms.  There's a comment that the
  kernel's sources were copied from "modutils." It seems that modutils'
  last release was v2.4.27 in 2004, and that development on it has
  stopped.  Upstream modutils also has the same parsing bug.
...
* Fix the parsing bug in genksysms. While the discussion about removing
  CONFIG_MODVERSIONS has started again upstream due to my bugreport,
  this would be the optimal solution, if I could just figure out how to
  rewrite the parser correctly.
...
A better long term solution would be to replace genksyms's
modutils/lex/yacc based incomplete and dead C parser with a libclang
based one, but such work is beyond the scope of a toolchain update.

For future travelers that would like to take a crack at fixing the
existing parser, I found the develop/build/test/debug cycle to be:

$ rm scripts/genksyms/genksyms
$ make scripts/genksyms/
$ ./scripts/genksyms/genksyms -d < test_case.i
$ ./scripts/genksyms/genksyms -d -d < test_case.i
Best of luck on that endeavor.
```

I was planning on talking about this timebomb at plumbers, but had to
cut it due to the tight time constraints we were allotted.
-- 
Thanks,
~Nick Desaulniers

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] genksyms: Ignore module scoped _Static_assert()
  2020-12-10 16:25         ` Marco Elver
  2020-12-10 21:48           ` Nick Desaulniers
@ 2020-12-11  5:24           ` Andrew Morton
  2020-12-19 12:40             ` Masahiro Yamada
  1 sibling, 1 reply; 14+ messages in thread
From: Andrew Morton @ 2020-12-11  5:24 UTC (permalink / raw)
  To: Marco Elver
  Cc: Miguel Ojeda, Nick Desaulniers, LKML, kasan-dev, Masahiro Yamada,
	Joe Perches, Greg Kroah-Hartman, Richard Henderson

On Thu, 10 Dec 2020 17:25:30 +0100 Marco Elver <elver@google.com> wrote:

> On Thu, 10 Dec 2020 at 14:29, Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
> > On Thu, Dec 10, 2020 at 11:35 AM Marco Elver <elver@google.com> wrote:
> > >
> > > It looks like there's no clear MAINTAINER for this. :-/
> > > It'd still be good to fix this for 5.11.
> >
> > Richard seems to be the author, not sure if he picks patches (CC'd).
> >
> > I guess Masahiro or akpm (Cc'd) would be two options; otherwise, I
> > could pick it up through compiler attributes (stretching the
> > definition...).
> 
> Thanks for the info. I did find that there's an alternative patch to
> fix _Static_assert() with genksyms that was sent 3 days after mine
> (it's simpler, but might miss cases). I've responded there (
> https://lkml.kernel.org/r/X9JI5KpWoo23wkRg@elver.google.com ).
> 
> Now we have some choice. I'd argue for this patch, because it's not
> doing preprocessor workarounds, but in the end I won't make that call.
> :-)

I have
https://lkml.kernel.org/r/20201203230955.1482058-1-arnd@kernel.org
queued for later this week.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] genksyms: Ignore module scoped _Static_assert()
  2020-12-11  5:24           ` Andrew Morton
@ 2020-12-19 12:40             ` Masahiro Yamada
  0 siblings, 0 replies; 14+ messages in thread
From: Masahiro Yamada @ 2020-12-19 12:40 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Marco Elver, Miguel Ojeda, Nick Desaulniers, LKML, kasan-dev,
	Joe Perches, Greg Kroah-Hartman, Richard Henderson

On Fri, Dec 11, 2020 at 2:24 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Thu, 10 Dec 2020 17:25:30 +0100 Marco Elver <elver@google.com> wrote:
>
> > On Thu, 10 Dec 2020 at 14:29, Miguel Ojeda
> > <miguel.ojeda.sandonis@gmail.com> wrote:
> > > On Thu, Dec 10, 2020 at 11:35 AM Marco Elver <elver@google.com> wrote:
> > > >
> > > > It looks like there's no clear MAINTAINER for this. :-/
> > > > It'd still be good to fix this for 5.11.
> > >
> > > Richard seems to be the author, not sure if he picks patches (CC'd).
> > >
> > > I guess Masahiro or akpm (Cc'd) would be two options; otherwise, I
> > > could pick it up through compiler attributes (stretching the
> > > definition...).
> >
> > Thanks for the info. I did find that there's an alternative patch to
> > fix _Static_assert() with genksyms that was sent 3 days after mine
> > (it's simpler, but might miss cases). I've responded there (
> > https://lkml.kernel.org/r/X9JI5KpWoo23wkRg@elver.google.com ).
> >
> > Now we have some choice. I'd argue for this patch, because it's not
> > doing preprocessor workarounds, but in the end I won't make that call.
> > :-)
>
> I have
> https://lkml.kernel.org/r/20201203230955.1482058-1-arnd@kernel.org
> queued for later this week.
>


Sorry for the delay, Marco.

And, thanks for the proper fix.
Now applied to linux-kbuild.



I will revert
14dc3983b5dff513a90bd5a8cc90acaf7867c3d0
later.





--
Best Regards
Masahiro Yamada

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2020-12-19 12:42 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-01 15:20 [PATCH] genksyms: Ignore module scoped _Static_assert() Marco Elver
2020-12-01 16:14 ` Christoph Hellwig
2020-12-01 17:04   ` Marco Elver
2020-12-01 19:56     ` Nick Desaulniers
2020-12-01 21:01       ` Rasmus Villemoes
2020-12-01 19:59 ` Nick Desaulniers
2020-12-01 20:12   ` Marco Elver
2020-12-04 10:21   ` Marco Elver
2020-12-10 10:35     ` Marco Elver
2020-12-10 13:29       ` Miguel Ojeda
2020-12-10 16:25         ` Marco Elver
2020-12-10 21:48           ` Nick Desaulniers
2020-12-11  5:24           ` Andrew Morton
2020-12-19 12:40             ` Masahiro Yamada

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).