[PATCH] sparse/parse.c: ignore hotpatch attribute

[PATCH] sparse/parse.c: ignore hotpatch attribute

[Heiko Carstens]

gcc knows about a new "hotpatch" attribute which sparse can safely ignore,
since it modifies only which code will be generated just like the
"no_instrument_function" attribute.

The gcc hotpatch feature patch:
https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=11762b8363737591bfb9c66093bc2edf289b917f

Currently the Linux kernel makes use of this attribute:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=61f552141c9c0e88b3fdc7046265781ffd8fa68a

Without this patch sparse will emit warnings like
"error: attribute 'hotpatch': unknown attribute"

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 parse.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/parse.c b/parse.c
index b43d6835528b..8afae73d5325 100644
--- a/parse.c
+++ b/parse.c
@@ -540,6 +540,8 @@ const char *ignored_attributes[] = {
 	"__gnu_inline__",
 	"hot",
 	"__hot__",
+	"hotpatch",
+	"__hotpatch__",
         "leaf",
         "__leaf__",
 	"l1_text",
-- 
2.1.4

Re: [PATCH] sparse/parse.c: ignore hotpatch attribute

[Christopher Li]

That patch looks fine.

Can you add some test case for the hot patch as well?
It need to cover the case you run into.

Preferably cover each of the variation of hot patch.

It will be useful when we parse these attributes properly.

Chris


On Tue, Apr 28, 2015 at 3:48 AM, Heiko Carstens
<heiko.carstens@de.ibm.com> wrote:
> gcc knows about a new "hotpatch" attribute which sparse can safely ignore,
> since it modifies only which code will be generated just like the
> "no_instrument_function" attribute.
>
> The gcc hotpatch feature patch:
> https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=11762b8363737591bfb9c66093bc2edf289b917f
>
> Currently the Linux kernel makes use of this attribute:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=61f552141c9c0e88b3fdc7046265781ffd8fa68a
>
> Without this patch sparse will emit warnings like
> "error: attribute 'hotpatch': unknown attribute"
>
> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> ---
>  parse.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/parse.c b/parse.c
> index b43d6835528b..8afae73d5325 100644
> --- a/parse.c
> +++ b/parse.c
> @@ -540,6 +540,8 @@ const char *ignored_attributes[] = {
>         "__gnu_inline__",
>         "hot",
>         "__hot__",
> +       "hotpatch",
> +       "__hotpatch__",
>          "leaf",
>          "__leaf__",
>         "l1_text",
> --
> 2.1.4
>

Re: [PATCH] sparse/parse.c: ignore hotpatch attribute

[Heiko Carstens]

On Wed, Apr 29, 2015 at 04:22:13PM -0700, Christopher Li wrote:
> That patch looks fine.
> 
> Can you add some test case for the hot patch as well?
> It need to cover the case you run into.
> 
> Preferably cover each of the variation of hot patch.
> 
> It will be useful when we parse these attributes properly.
> 
> Chris

Something like below? I have to admit that I have not much of a clue
of what I'm doing ;)

From 03da0b28318100cc9a1389c81920654b29405d73 Mon Sep 17 00:00:00 2001
From: Heiko Carstens <heiko.carstens@de.ibm.com>
Date: Thu, 30 Apr 2015 12:53:28 +0200
Subject: [PATCH] sparse/parse.c: ignore hotpatch attribute

gcc knows about a new "hotpatch" attribute which sparse can safely ignore,
since it modifies only which code will be generated just like the
"no_instrument_function" attribute.

The gcc hotpatch feature patch:
https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=11762b8363737591bfb9c66093bc2edf289b917f

Currently the Linux kernel makes use of this attribute:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=61f552141c9c0e88b3fdc7046265781ffd8fa68a

Without this patch sparse will emit warnings like
"error: attribute 'hotpatch': unknown attribute"

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 ident-list.h               | 1 +
 parse.c                    | 2 ++
 validation/attr-hotpatch.c | 7 +++++++
 3 files changed, 10 insertions(+)
 create mode 100644 validation/attr-hotpatch.c

diff --git a/ident-list.h b/ident-list.h
index b65b667da720..1849ea613b47 100644
--- a/ident-list.h
+++ b/ident-list.h
@@ -81,6 +81,7 @@ IDENT(constructor); IDENT(__constructor__);
 IDENT(destructor); IDENT(__destructor__);
 IDENT(cold); IDENT(__cold__);
 IDENT(hot); IDENT(__hot__);
+IDENT(hotpatch); IDENT(__hotpatch__);
 IDENT(cdecl); IDENT(__cdecl__);
 IDENT(stdcall); IDENT(__stdcall__);
 IDENT(fastcall); IDENT(__fastcall__);
diff --git a/parse.c b/parse.c
index b43d6835528b..8afae73d5325 100644
--- a/parse.c
+++ b/parse.c
@@ -540,6 +540,8 @@ const char *ignored_attributes[] = {
 	"__gnu_inline__",
 	"hot",
 	"__hot__",
+	"hotpatch",
+	"__hotpatch__",
         "leaf",
         "__leaf__",
 	"l1_text",
diff --git a/validation/attr-hotpatch.c b/validation/attr-hotpatch.c
new file mode 100644
index 000000000000..205169389956
--- /dev/null
+++ b/validation/attr-hotpatch.c
@@ -0,0 +1,7 @@
+static void __attribute__((hotpatch(0,3))) bar(void)
+{
+}
+
+/*
+ * check-name: attribute hotpatch
+ */
-- 
2.1.4

Re: [PATCH] sparse/parse.c: ignore hotpatch attribute

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 679 bytes --]

On Tue, Apr 28, 2015 at 3:48 AM, Heiko Carstens
<heiko.carstens@de.ibm.com> wrote:
> gcc knows about a new "hotpatch" attribute which sparse can safely ignore,
> since it modifies only which code will be generated just like the
> "no_instrument_function" attribute.

I'm wondering if sparse should just ignore all attributes it doesn't
recognize, so that we could just remove this ever-expanding list of
things that don't actually matter for sparse..

The "unrecognized attribute" thing made more sense way back when -
when I wanted to get he basic attributes handled. Now it's just
noise...

Something like the attached (mostly untested) patch..

                         Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 2751 bytes --]

 parse.c | 112 ----------------------------------------------------------------
 1 file changed, 112 deletions(-)

diff --git a/parse.c b/parse.c
index b43d6835528b..bab0a8e2e931 100644
--- a/parse.c
+++ b/parse.c
@@ -501,109 +501,6 @@ static struct init_keyword {
 	{ "__word__",	NS_KEYWORD,	MOD_LONG,	.op = &mode_word_op },
 };
 
-const char *ignored_attributes[] = {
-	"alias",
-	"__alias__",
-	"alloc_size",
-	"__alloc_size__",
-	"always_inline",
-	"__always_inline__",
-	"artificial",
-	"__artificial__",
-	"bounded",
-	"__bounded__",
-	"cdecl",
-	"__cdecl__",
-	"cold",
-	"__cold__",
-	"constructor",
-	"__constructor__",
-	"deprecated",
-	"__deprecated__",
-	"destructor",
-	"__destructor__",
-	"dllexport",
-	"__dllexport__",
-	"dllimport",
-	"__dllimport__",
-	"error",
-	"__error__",
-	"externally_visible",
-	"__externally_visible__",
-	"fastcall",
-	"__fastcall__",
-	"format",
-	"__format__",
-	"format_arg",
-	"__format_arg__",
-	"gnu_inline",
-	"__gnu_inline__",
-	"hot",
-	"__hot__",
-        "leaf",
-        "__leaf__",
-	"l1_text",
-	"__l1_text__",
-	"l1_data",
-	"__l1_data__",
-	"l2",
-	"__l2__",
-	"malloc",
-	"__malloc__",
-	"may_alias",
-	"__may_alias__",
-	"model",
-	"__model__",
-	"ms_abi",
-	"__ms_abi__",
-	"ms_hook_prologue",
-	"__ms_hook_prologue__",
-	"naked",
-	"__naked__",
-	"no_instrument_function",
-	"__no_instrument_function__",
-	"noclone",
-	"__noclone",
-	"__noclone__",
-	"noinline",
-	"__noinline__",
-	"nonnull",
-	"__nonnull",
-	"__nonnull__",
-	"nothrow",
-	"__nothrow",
-	"__nothrow__",
-	"regparm",
-	"__regparm__",
-	"section",
-	"__section__",
-	"sentinel",
-	"__sentinel__",
-	"signal",
-	"__signal__",
-	"stdcall",
-	"__stdcall__",
-	"syscall_linkage",
-	"__syscall_linkage__",
-	"sysv_abi",
-	"__sysv_abi__",
-	"unused",
-	"__unused__",
-	"used",
-	"__used__",
-	"vector_size",
-	"__vector_size__",
-	"visibility",
-	"__visibility__",
-	"warn_unused_result",
-	"__warn_unused_result__",
-	"warning",
-	"__warning__",
-	"weak",
-	"__weak__",
-};
-
-
 void init_parser(int stream)
 {
 	int i;
@@ -617,14 +514,6 @@ void init_parser(int stream)
 		sym->ctype.base_type = ptr->type;
 		sym->op = ptr->op;
 	}
-
-	for (i = 0; i < ARRAY_SIZE(ignored_attributes); i++) {
-		const char * name = ignored_attributes[i];
-		struct symbol *sym = create_symbol(stream, name, SYM_KEYWORD,
-						   NS_KEYWORD);
-		sym->ident->keyword = 1;
-		sym->op = &ignore_attr_op;
-	}
 }
 
 
@@ -1222,7 +1111,6 @@ static struct token *recover_unknown_attribute(struct token *token)
 {
 	struct expression *expr = NULL;
 
-	sparse_error(token->pos, "attribute '%s': unknown attribute", show_ident(token->ident));
 	token = token->next;
 	if (match_op(token, '('))
 		token = parens_expression(token, &expr, "in attribute");

Re: [PATCH] sparse/parse.c: ignore hotpatch attribute

[Nicholas Mc Guire]

On Thu, 30 Apr 2015, Linus Torvalds wrote:

> On Tue, Apr 28, 2015 at 3:48 AM, Heiko Carstens
> <heiko.carstens@de.ibm.com> wrote:
> > gcc knows about a new "hotpatch" attribute which sparse can safely ignore,
> > since it modifies only which code will be generated just like the
> > "no_instrument_function" attribute.
> 
> I'm wondering if sparse should just ignore all attributes it doesn't
> recognize, so that we could just remove this ever-expanding list of
> things that don't actually matter for sparse..
> 
> The "unrecognized attribute" thing made more sense way back when -
> when I wanted to get he basic attributes handled. Now it's just
> noise...
>
The list of attributes does not really exapand quickly so explicitly
marking them to be ignored after a new "unrecognized attribute" triggers
would have the advantage that it is actually looked at and not
silently ignored.

thx!
hofrat

Re: [PATCH] sparse/parse.c: ignore hotpatch attribute

[josh]

On Thu, Apr 30, 2015 at 08:45:33AM -0700, Linus Torvalds wrote:
> On Tue, Apr 28, 2015 at 3:48 AM, Heiko Carstens
> <heiko.carstens@de.ibm.com> wrote:
> > gcc knows about a new "hotpatch" attribute which sparse can safely ignore,
> > since it modifies only which code will be generated just like the
> > "no_instrument_function" attribute.
> 
> I'm wondering if sparse should just ignore all attributes it doesn't
> recognize, so that we could just remove this ever-expanding list of
> things that don't actually matter for sparse..
> 
> The "unrecognized attribute" thing made more sense way back when -
> when I wanted to get he basic attributes handled. Now it's just
> noise...

Unfortunately, ever so often there's an attribute we *can't* just
ignore, and in those cases it's better for us to warn rather than
breaking.  For instance, we won't just be able to ignore "cleanup"; we
need to actually handle it, because it affects code flow.

I think part of the problem arises because sparse claims (via the
preprocessor symbols we provide) to be whatever version of GCC it was
compiled with.  I think that's a mistake.  We should pick a version of
GCC for which we support all the attributes we actually need to do
something with, and advertise ourselves specifically as that version.
That way, a codebase that includes detection for available GCC features
will avoid feeding us attributes and other features that we don't
understand.

I suspect that 3.2 is probably the version sparse should claim to be,
for now.

We could then use something like the patch you posted, possibly with the
addition of an attribute blacklist for any attributes we know we need to
do something with but don't yet, so that we warn about *those*
attributes and no others.  For anything only supported by a newer
version of GCC, it's the code we're scanning's fault for feeding us
something we never claimed to understand.

We can then update the version of GCC we claim to be once we add support
for any features of that version of GCC that absolutely need Sparse
support.

- Josh Triplett

Re: [PATCH] sparse/parse.c: ignore hotpatch attribute

[Linus Torvalds]

On Thu, Apr 30, 2015 at 10:38 AM,  <josh@joshtriplett.org> wrote:
>
> I think part of the problem arises because sparse claims (via the
> preprocessor symbols we provide) to be whatever version of GCC it was
> compiled with.  I think that's a mistake.  We should pick a version of
> GCC for which we support all the attributes we actually need to do
> something with, and advertise ourselves specifically as that version.

Fair enough. Although there may then be headers that are unhappy.

> I suspect that 3.2 is probably the version sparse should claim to be,
> for now.

That may be too old. You can't reliably compile the kernel with gcc
that old (some config options will complain).

At least CONFIG_GCOV_KERNEL wants 3.4 minimum.

                       Linus

Re: [PATCH] sparse/parse.c: ignore hotpatch attribute

[josh]

On Thu, Apr 30, 2015 at 10:51:42AM -0700, Linus Torvalds wrote:
> On Thu, Apr 30, 2015 at 10:38 AM,  <josh@joshtriplett.org> wrote:
> > I think part of the problem arises because sparse claims (via the
> > preprocessor symbols we provide) to be whatever version of GCC it was
> > compiled with.  I think that's a mistake.  We should pick a version of
> > GCC for which we support all the attributes we actually need to do
> > something with, and advertise ourselves specifically as that version.
> 
> Fair enough. Although there may then be headers that are unhappy.

If you mean the GCC internal headers, I don't think those have version
checks; after all, you'd only use them with the GCC they ship with,
right? ;)

For any other headers, I think we'll get worse results by claiming to be
a version of GCC that has features we don't actually support.

> > I suspect that 3.2 is probably the version sparse should claim to be,
> > for now.
> 
> That may be too old. You can't reliably compile the kernel with gcc
> that old (some config options will complain).
> 
> At least CONFIG_GCOV_KERNEL wants 3.4 minimum.

If we want to claim to be 3.4 instead, then there are some attributes
we'll need to check for and warn if we find.  From a quick read of
https://ohse.de/uwe/articles/gcc-attributes.html I think this is a
complete list of attributes in 3.4 that sparse has to care about but
doesn't currently support:

cleanup
gcc_struct
ms_struct

"cleanup" we'll need to handle because it affects control flow;
gcc_struct and ms_struct affect structure layout, which we have warnings
related to.

For the moment, we could just add an explicit warning if we see any of
those three attributes, set our GCC version to 3.4, and then drop the
"unknown attribute" warning.

- Josh Triplett