Skip to contentSkip to conversation

eliminate warnings in generated module files

PATCH In review Richard Henderson 8 participants 22 messages Feb 19, 2003 – Feb 26, 2003 Sparse checker

lore
Share & tools
lore

Richard Henderson

author
#0 lore raw

PATCH 1/1 eliminate warnings in generated module files

The compiler.h fragment should describe the problem well enough.

r~

===== include/linux/compiler.h 1.10 vs edited =====

include/linux/compiler.h +17 -0
@@ -25,6 +25,23 @@
 #define __deprecated
 #endif
 
+/*
+ * Allow us to avoid 'defined but not used' warnings on functions and data,
+ * as well as force them to be emitted to the assembly file.
+ *
+ * As of gcc 3.3, static functions that are not marked with attribute((used))
+ * may be elided from the assembly file.  As of gcc 3.3, static data not so
+ * marked will not be elided, but this may change in a future gcc version.
+ *
+ * In prior versions of gcc, such functions and data would be emitted, but
+ * would be warned about except with attribute((unused)).
+ */
+#if __GNUC__ == 3 && __GNUC_MINOR__ >= 3 || __GNUC__ > 3
+#define __attribute_used__	__attribute__((__used__))
+#else
+#define __attribute_used__	__attribute__((__unused__))
+#endif
+
 /* This macro obfuscates arithmetic on a variable address so that gcc
    shouldn't recognize the original var, and make assumptions about it */
 #define RELOC_HIDE(ptr, off)					\

===== scripts/modpost.c 1.7 vs edited =====

scripts/modpost.c +2 -0
@@ -384,6 +384,7 @@
 {
 	buf_printf(b, "#include <linux/module.h>\n");
 	buf_printf(b, "#include <linux/vermagic.h>\n");
+	buf_printf(b, "#include <linux/compiler.h>\n");
 	buf_printf(b, "\n");
 	buf_printf(b, "const char vermagic[]\n");
 	buf_printf(b, "__attribute__((section(\"__vermagic\"))) =\n");

@@ -449,6 +450,7 @@
 
 	buf_printf(b, "\n");
 	buf_printf(b, "static const char __module_depends[]\n");
+	buf_printf(b, "__attribute_used__\n");
 	buf_printf(b, "__attribute__((section(\".modinfo\"))) =\n");
 	buf_printf(b, "\"depends=");
 	for (s = mod->unres; s; s = s->next) {
#1 lore raw
Quoted 2 lines · Richard Henderson

The compiler.h fragment should describe the problem well enough.

Have you tested this with older compilers?

In particular, I have this dim memory of gcc historically not liking multiple separate __attribute__ bits, ie

	__attribute__((unused,__section__ ...))

would be fine, but

	__attribute__((unused)) __attribute__((__section__ ...))

would not compile.

But hey, my brain is cabbage, and my memory might be crap.

Linus

author
#2 lore raw
Quoted 1 line · Linus Torvalds

Have you tested this with older compilers?

Nope.

Quoted 5 lines

In particular, I have this dim memory of gcc historically not liking multiple separate __attribute__ bits, ie

	__attribute__((unused,__section__ ...))
Quoted 7 lines

would be fine, but

	__attribute__((unused)) __attribute__((__section__ ...))

would not compile.

But hey, my brain is cabbage, and my memory might be crap.

Hmm. It was always supposed to have worked, but I suppose there could have been bugs. How far back to I need to go looking?

r~

#3 lore raw
Quoted 6 lines · Richard Henderson
Quoted 2 lines

But hey, my brain is cabbage, and my memory might be crap.

Hmm. It was always supposed to have worked, but I suppose there could have been bugs. How far back to I need to go looking?

Some people are still using 2.95, I think anything past that is long since unsupported and not worth worrying about.

Linus

author
#4 lore raw
Quoted 2 lines · Linus Torvalds

Some people are still using 2.95, I think anything past that is long since unsupported and not worth worrying about.

[kanga:~] cat z.c
static char foo []
__attribute__((unused)) __attribute__((section(".data.foo")))
= "asdfasdf";
[kanga:~] /usr/bin/gcc -Wall -c z.c
[kanga:~] /usr/bin/gcc -v
Reading specs from /usr/lib/gcc-lib/alpha-linux/2.95.4/specs gcc version 2.95.4 20011002 (Debian prerelease) [kanga:~] objdump -h z.o | grep foo
3 .data.foo 00000009 0000000000000000 0000000000000000 00000040 2**0

Seems to work, both wrt the warning message and
honoring the section directive.

r~

#5 lore raw
Quoted 3 lines · Richard Henderson
Quoted 2 lines · Linus Torvalds

Some people are still using 2.95, I think anything past that is long since unsupported and not worth worrying about.

I tried egcs-2.91.66 (not sure if the sparc people are still using that?), looks ok.

I merged the patch into my kbuild tree.

However, I'm not quite happy with "__attribute_used__".

Other examples of similar defines:

__deprecated
__init
__exit

So what about "__used" ? Admittedly, it's a bit short, but I like it better anyhow.

--Kai

author
#6 lore raw
Quoted 2 lines · Kai Germaschewski

So what about "__used" ? Admittedly, it's a bit short, but I like it better anyhow.

I don't really like it, but don't care that much.

r~

#7 lore raw
In reply to Richard Henderson

In message <[email protected]> you write:

Quoted 4 lines

gcc version 2.95.4 20011002 (Debian prerelease)

Seems to work, both wrt the warning message and
honoring the section directive.

Excellent. For Linus's (and Kai's) benefit, this is Richard's patch again. Thanks Richard!

From: Richard Henderson <[email protected]>

===== include/linux/compiler.h 1.10 vs edited =====

include/linux/compiler.h +17 -0
@@ -25,6 +25,23 @@
 #define __deprecated
 #endif
 
+/*
+ * Allow us to avoid 'defined but not used' warnings on functions and data,
+ * as well as force them to be emitted to the assembly file.
+ *
+ * As of gcc 3.3, static functions that are not marked with attribute((used))
+ * may be elided from the assembly file.  As of gcc 3.3, static data not so
+ * marked will not be elided, but this may change in a future gcc version.
+ *
+ * In prior versions of gcc, such functions and data would be emitted, but
+ * would be warned about except with attribute((unused)).
+ */
+#if __GNUC__ == 3 && __GNUC_MINOR__ >= 3 || __GNUC__ > 3
+#define __attribute_used__	__attribute__((__used__))
+#else
+#define __attribute_used__	__attribute__((__unused__))
+#endif
+
 /* This macro obfuscates arithmetic on a variable address so that gcc
    shouldn't recognize the original var, and make assumptions about it */
 #define RELOC_HIDE(ptr, off)					\

===== scripts/modpost.c 1.7 vs edited =====

scripts/modpost.c +2 -0
@@ -384,6 +384,7 @@
 {
 	buf_printf(b, "#include <linux/module.h>\n");
 	buf_printf(b, "#include <linux/vermagic.h>\n");
+	buf_printf(b, "#include <linux/compiler.h>\n");
 	buf_printf(b, "\n");
 	buf_printf(b, "const char vermagic[]\n");
 	buf_printf(b, "__attribute__((section(\"__vermagic\"))) =\n");

@@ -449,6 +450,7 @@
 
 	buf_printf(b, "\n");
 	buf_printf(b, "static const char __module_depends[]\n");
+	buf_printf(b, "__attribute_used__\n");
 	buf_printf(b, "__attribute__((section(\".modinfo\"))) =\n");
 	buf_printf(b, "\"depends=");
 	for (s = mod->unres; s; s = s->next) {

--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

#8 lore raw
In reply to Linus Torvalds
Quoted 2 lines · Linus Torvalds

Some people are still using 2.95, I think anything past that is long since unsupported and not worth worrying about.

I've recently started using 3.2 for testing and it seems, thus far, so worse than 2.95.x and appears to have fewer bugs in some regards (i.e. don't seem to go bonkers with register pressure form long long).

At some point, 2.95.x might be considered too old and gcc 3.2+ with have to be the minimum --- is this time near?

What about MIPS and Sparc64 --- are they still using ancient compilers?

--cw

#9 lore raw
In reply to Richard Henderson

In message <[email protected]> you write:

Review

Quoted 6 lines
(quoted diff) +6 -0
+#if __GNUC__ == 3 && __GNUC_MINOR__ >= 3 || __GNUC__ > 3
+#define __attribute_used__	__attribute__((__used__))
+#else
+#define __attribute_used__	__attribute__((__unused__))
+#endif
+

After some thought, I prefer __optional. The unused attribute has muddied the waters too badly for "used" or "unused" to be clear.

We could debate this for days. Linus, apply or don't. Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Name: __optional attribute
Author: Rusty Russell
Status: Trivial

D: Renames __attribute_used to __optional.

include/linux/compiler.h +2 -2
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .21098-linux-2.5.63/include/linux/compiler.h .21098-linux-2.5.63.updated/include/linux/compiler.h
@@ -37,9 +37,9 @@
  * would be warned about except with attribute((unused)).
  */
 #if __GNUC__ == 3 && __GNUC_MINOR__ >= 3 || __GNUC__ > 3
-#define __attribute_used__	__attribute__((__used__))
+#define __optional	__attribute__((__used__))
 #else
-#define __attribute_used__	__attribute__((__unused__))
+#define __optional	__attribute__((__unused__))
 #endif
 
 /* This macro obfuscates arithmetic on a variable address so that gcc
scripts/modpost.c +1 -1
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .21098-linux-2.5.63/scripts/modpost.c .21098-linux-2.5.63.updated/scripts/modpost.c
@@ -450,7 +450,7 @@ add_depends(struct buffer *b, struct mod
 
 	buf_printf(b, "\n");
 	buf_printf(b, "static const char __module_depends[]\n");
-	buf_printf(b, "__attribute_used__\n");
+	buf_printf(b, "__optional\n");
 	buf_printf(b, "__attribute__((section(\".modinfo\"))) =\n");
 	buf_printf(b, "\"depends=");
 	for (s = mod->unres; s; s = s->next) {
author
#10 lore raw
Quoted 9 lines · Rusty Russell

In message <[email protected]> you write:

Review

Quoted 6 lines
(quoted diff) +6 -0
+#if __GNUC__ == 3 && __GNUC_MINOR__ >= 3 || __GNUC__ > 3
+#define __attribute_used__	__attribute__((__used__))
+#else
+#define __attribute_used__	__attribute__((__unused__))
+#endif
+

After some thought, I prefer __optional.

Um, "optional" does not in any way accurately describe attribute used. In fact, it means almost exactly the opposite.

r~

#11 lore raw
Quoted 2 lines · Richard Henderson

Um, "optional" does not in any way accurately describe attribute used. In fact, it means almost exactly the opposite.

ironically the same could be said for "used" and "unused" ...

--cw

#12 lore raw
In reply to Richard Henderson

In message <[email protected]> you write:

Quoted 5 lines · Rusty Russell
Quoted 1 line

After some thought, I prefer __optional.

Um, "optional" does not in any way accurately describe attribute used. In fact, it means almost exactly the opposite.

Yep.

__optional should always be __attribute__((__unused__)), and __required should be your __attribute_used__.

This one makes more sense to the user, I think:

/* May not be used depending on config options */ static ctl_table ip_conntrack_table[] __optional = { ...

/* Must be in binary for strings to find */
static char version_string[] = "Version foo.c 1.2.3" __required;

Thoughts?
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Name: __optional attribute
Author: Rusty Russell
Status: Trivial

D: Renames __attribute_used to __required, and introduces __optional.

include/linux/compiler.h +3 -2
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .25651-linux-2.5.63/include/linux/compiler.h .25651-linux-2.5.63.updated/include/linux/compiler.h
@@ -37,10 +37,11 @@
  * would be warned about except with attribute((unused)).
  */
 #if __GNUC__ == 3 && __GNUC_MINOR__ >= 3 || __GNUC__ > 3
-#define __attribute_used__	__attribute__((__used__))
+#define __required	__attribute__((__used__))
 #else
-#define __attribute_used__	__attribute__((__unused__))
+#define __required	__attribute__((__unused__))
 #endif
+#define __optional	__attribute__((__unused__))
 
 /* This macro obfuscates arithmetic on a variable address so that gcc
    shouldn't recognize the original var, and make assumptions about it */
scripts/modpost.c +1 -1
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .25651-linux-2.5.63/scripts/modpost.c .25651-linux-2.5.63.updated/scripts/modpost.c
@@ -450,7 +450,7 @@ add_depends(struct buffer *b, struct mod
 
 	buf_printf(b, "\n");
 	buf_printf(b, "static const char __module_depends[]\n");
-	buf_printf(b, "__attribute_used__\n");
+	buf_printf(b, "__optional\n");
 	buf_printf(b, "__attribute__((section(\".modinfo\"))) =\n");
 	buf_printf(b, "\"depends=");
 	for (s = mod->unres; s; s = s->next) {
#13 lore raw
Quoted 3 lines · Rusty Russell

__optional should always be __attribute__((__unused__)), and __required should be your __attribute_used__.

But I think rth's point was that "__module_depends" should definitely _not_ be "optional", since that just means that the compiler can (and will) optimize away the whole thing.

So marking it optional is definitely the wrong thing to do.

Linus

#14 lore raw

In message <[email protected]> you wri te:

Quoted 11 lines · Rusty Russell
Quoted 3 lines

__optional should always be __attribute__((__unused__)), and __required should be your __attribute_used__.

But I think rth's point was that "__module_depends" should definitely _not_ be "optional", since that just means that the compiler can (and will) optimize away the whole thing.

So marking it optional is definitely the wrong thing to do.

This time for sure!

Name: __optional attribute
Author: Rusty Russell
Status: Trivial

D: Renames __attribute_used to __required, and introduces __optional.

include/linux/compiler.h +3 -2
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .25651-linux-2.5.63/include/linux/compiler.h .25651-linux-2.5.63.updated/include/linux/compiler.h
@@ -37,10 +37,11 @@
  * would be warned about except with attribute((unused)).
  */
 #if __GNUC__ == 3 && __GNUC_MINOR__ >= 3 || __GNUC__ > 3
-#define __attribute_used__	__attribute__((__used__))
+#define __required	__attribute__((__used__))
 #else
-#define __attribute_used__	__attribute__((__unused__))
+#define __required	__attribute__((__unused__))
 #endif
+#define __optional	__attribute__((__unused__))
 
 /* This macro obfuscates arithmetic on a variable address so that gcc
    shouldn't recognize the original var, and make assumptions about it */
scripts/modpost.c +1 -1
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .25651-linux-2.5.63/scripts/modpost.c .25651-linux-2.5.63.updated/scripts/modpost.c
@@ -450,7 +450,7 @@ add_depends(struct buffer *b, struct mod
 
 	buf_printf(b, "\n");
 	buf_printf(b, "static const char __module_depends[]\n");
-	buf_printf(b, "__attribute_used__\n");
+	buf_printf(b, "__required\n");
 	buf_printf(b, "__attribute__((section(\".modinfo\"))) =\n");
 	buf_printf(b, "\"depends=");
 	for (s = mod->unres; s; s = s->next) {

--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

#15 lore raw
Quoted 8 lines · Rusty Russell

In message <[email protected]> you wri te:

Quoted 6 lines · Rusty Russell
Quoted 3 lines

__optional should always be __attribute__((__unused__)), and __required should be your __attribute_used__.

Quoted 7 lines
Quoted 5 lines

But I think rth's point was that "__module_depends" should definitely _not_ be "optional", since that just means that the compiler can (and will) optimize away the whole thing.

So marking it optional is definitely the wrong thing to do.

This time for sure!

FWIW, I think it's not a good idea. Why call it 'required' in the kernel when the normal (gcc) expression for it is 'used'. - We didn't rename 'deprecated' to 'obsolete', either ;)

Also, I don't really see any use for __optional at this point, so why add it at all?

So IMO, the only change which possibly makes sense is to rename __attribute_used__ to __used, since it makes it more consistent with similar things like __deprecated, __init, __exit etc.

--Kai

#16 lore raw

In message <[email protected]> y ou write:

Quoted 3 lines

FWIW, I think it's not a good idea. Why call it 'required' in the kernel when the normal (gcc) expression for it is 'used'. - We didn't rename 'deprecated' to 'obsolete', either ;)

But deprecated was a fine name. "used" is a terrible name, and since we're renaming it via a macro anyway... (see "likey").

Quoted 4 lines

Also, I don't really see any use for __optional at this point, so why add it at all?

From ip_conntrack_core.c:

#ifdef CONFIG_SYSCTL
static struct ctl_table_header *ip_conntrack_sysctl_header;

static ctl_table ip_conntrack_table[] = {
	{
		.ctl_name	= NET_IP_CONNTRACK_MAX,
		.procname	= NET_IP_CONNTRACK_MAX_NAME,
		.data		= &ip_conntrack_max,
		.maxlen		= sizeof(ip_conntrack_max),
		.mode		= 0644,
		.proc_handler	= proc_dointvec
	},

{ .ctl_name = 0 }
};

static ctl_table ip_conntrack_dir_table[] = {
	{
		.ctl_name	= NET_IPV4,
		.procname	= "ipv4",
		.maxlen		= 0,
		.mode		= 0555,
		.child		= ip_conntrack_table
	},
	{ .ctl_name = 0 }
};
static ctl_table ip_conntrack_root_table[] = {
	{
		.ctl_name	= CTL_NET,
		.procname	= "net",
		.maxlen		= 0,
		.mode		= 0555,
		.child		= ip_conntrack_dir_table
	},
	{ .ctl_name = 0 }
};

#endif /*CONFIG_SYSCTL*/

I'd love to frop the #ifdef and just mark them __optional: before that would just mean bloat, but when gcc 3.3 rolls in, they should vanish nicely.

There are numerous other examples...
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

#17 lore raw
Quoted 3 lines

In message <[email protected]> you write:

[...]

Quoted 5 lines
Quoted 4 lines

Also, I don't really see any use for __optional at this point, so why add it at all?

From ip_conntrack_core.c:

Quoted 7 lines

#ifdef CONFIG_SYSCTL
[snipped largeish data structure]
#endif /*CONFIG_SYSCTL*/

I'd love to frop the #ifdef and just mark them __optional: before that would just mean bloat, but when gcc 3.3 rolls in, they should vanish nicely.

If gcc will just discard it, why bother marking it specially? Unless it gives ugly warnings, that is.

Signature · 4 lines
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513
author
#18 lore raw
In reply to Rusty Russell
Quoted 3 lines · Rusty Russell

I'd love to frop the #ifdef and just mark them __optional: before that would just mean bloat, but when gcc 3.3 rolls in, they should vanish nicely.

Um, no, as mentioned in the comment, at present only unused static functions will get removed. Data isn't touched, yet.

r~

#19 lore raw
In reply to Rusty Russell
Quoted 12 lines

In message <[email protected]> you wri te:

Quoted 10 lines · Rusty Russell
Quoted 3 lines

__optional should always be __attribute__((__unused__)), and __required should be your __attribute_used__.

But I think rth's point was that "__module_depends" should definitely _not_ be "optional", since that just means that the compiler can (and will) optimize away the whole thing.

Quoted 7 lines
Quoted 1 line

So marking it optional is definitely the wrong thing to do.

This time for sure!

Name: __optional attribute
Author: Rusty Russell
Status: Trivial

I have to agree with Kai and Milton Miller on this (bad) naming. __required and __optional don't generate the corrent connotations of what is being attempted here.

Milton suggesting spelling __attribute_used__ as __keep or __needed. I prefer __attribute_used__, but something like __mark_as_used__ would be OK too.

And what uses __optional, however it is spelled?

Thanks,
~Randy

#20 lore raw

In message <[email protected]> you write:

Quoted 3 lines

I have to agree with Kai and Milton Miller on this (bad) naming. __required and __optional don't generate the corrent connotations of what is being attempted here.

The problem with "used" is that you're saying "compiler, treat this as used, so you don't discard it".

The level of indirection seems completely natural to compiler people, but as a coder I just want to say "don't discard this", hence "__keep" is good.

OTOH, __optional is fairly clearly "you can drop it". "Unused" is clearly a lie for some configurations.

Here's Take III:

Name: __keep and __optional attributes
Author: Rusty Russell
Status: Trivial

D: Renames __attribute_used to __keep, and introduces __optional.

include/linux/compiler.h +3 -2
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .25651-linux-2.5.63/include/linux/compiler.h .25651-linux-2.5.63.updated/include/linux/compiler.h
@@ -37,10 +37,11 @@
  * would be warned about except with attribute((unused)).
  */
 #if __GNUC__ == 3 && __GNUC_MINOR__ >= 3 || __GNUC__ > 3
-#define __attribute_used__	__attribute__((__used__))
+#define __keep		__attribute__((__used__))
 #else
-#define __attribute_used__	__attribute__((__unused__))
+#define __keep		__attribute__((__unused__))
 #endif
+#define __optional	__attribute__((__unused__))
 
 /* This macro obfuscates arithmetic on a variable address so that gcc
    shouldn't recognize the original var, and make assumptions about it */
scripts/modpost.c +1 -1
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .25651-linux-2.5.63/scripts/modpost.c .25651-linux-2.5.63.updated/scripts/modpost.c
@@ -450,7 +450,7 @@ add_depends(struct buffer *b, struct mod
 
 	buf_printf(b, "\n");
 	buf_printf(b, "static const char __module_depends[]\n");
-	buf_printf(b, "__attribute_used__\n");
+	buf_printf(b, "__keep\n");
 	buf_printf(b, "__attribute__((section(\".modinfo\"))) =\n");
 	buf_printf(b, "\"depends=");
 	for (s = mod->unres; s; s = s->next) {

--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

#21 lore raw
Quoted 2 lines · Rusty Russell

OTOH, __optional is fairly clearly "you can drop it". "Unused" is clearly a lie for some configurations.

__maybe_unused ? :-)

- Werner

Signature · 3 lines
  _________________________________________________________________________
 / Werner Almesberger, Buenos Aires, Argentina         [email protected] /
/_http://www.almesberger.net/____________________________________________/