2008-02-27 03:09:20

by Joe Perches

[permalink] [raw]
Subject: [PATCH] linux/kernel.h linux/device.h - Convert debug functions declared inline __attribute__((format (printf,x,y) to statement expression macros

When DEBUG is not defined, pr_debug and dev_dbg and some
other local debugging functions are specified as:

"inline __attribute__((format (printf, x, y)))"

This is done to validate printk arguments when not debugging.

Converting these functions to macros or statement expressions
"do { if (0) printk(fmt, ##arg); } while (0)"
or
"({ if (0) printk(fmt, ##arg); 0; })
makes at least gcc 4.2.2 produce smaller objects.

This has the additional benefit of allowing the optimizer to
avoid calling functions like print_mac that might have been
arguments to the printk.

defconfig x86 current:

$ size vmlinux
text data bss dec hex filename
4716770 474560 618496 5809826 58a6a2 vmlinux

all converted: (More patches follow)

$ size vmlinux
text data bss dec hex filename
4716642 474560 618496 5809698 58a622 vmlinux

Even kernel/sched.o, which doesn't even use these
functions, becomes smaller.

It appears that merely having an indirect include
of <linux/device.h> can cause bigger objects.

$ size sched.inline.o sched.if0.o
text data bss dec hex filename
31385 2854 328 34567 8707 sched.inline.o
31366 2854 328 34548 86f4 sched.if0.o

The current preprocessed only kernel/sched.i file contains:

# 612 "include/linux/device.h"
static inline __attribute__((always_inline)) int __attribute__ ((format (printf, 2, 3)))
dev_dbg(struct device *dev, const char *fmt, ...)
{
return 0;
}
# 628 "include/linux/device.h"
static inline __attribute__((always_inline)) int __attribute__ ((format (printf, 2, 3)))
dev_vdbg(struct device *dev, const char *fmt, ...)
{
return 0;
}

Removing these unused inlines from sched.i shrinks sched.o

Signed-off-by: Joe Perches <[email protected]>

include/linux/kernel.h | 6 ++----
include/linux/device.h | 15 +++++----------

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 2df44e7..cd6d02c 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -293,10 +293,8 @@ extern void print_hex_dump_bytes(const char *prefix_str, int prefix_type,
#define pr_debug(fmt, arg...) \
printk(KERN_DEBUG fmt, ##arg)
#else
-static inline int __attribute__ ((format (printf, 1, 2))) pr_debug(const char * fmt, ...)
-{
- return 0;
-}
+#define pr_debug(fmt, arg...) \
+ ({ if (0) printk(KERN_DEBUG fmt, ##arg); 0; })
#endif

/*
diff --git a/include/linux/device.h b/include/linux/device.h
index 2258d89..12186e5 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -608,21 +608,16 @@ extern const char *dev_driver_string(struct device *dev);
#define dev_dbg(dev, format, arg...) \
dev_printk(KERN_DEBUG , dev , format , ## arg)
#else
-static inline int __attribute__ ((format (printf, 2, 3)))
-dev_dbg(struct device *dev, const char *fmt, ...)
-{
- return 0;
-}
+#define dev_dbg(dev, format, arg...) \
+ ({ if (0) dev_printk(KERN_DEBUG, dev, format, ##arg); 0; })
#endif

#ifdef VERBOSE_DEBUG
#define dev_vdbg dev_dbg
#else
-static inline int __attribute__ ((format (printf, 2, 3)))
-dev_vdbg(struct device *dev, const char *fmt, ...)
-{
- return 0;
-}
+
+#define dev_vdbg(dev, format, arg...) \
+ ({ if (0) dev_printk(KERN_DEBUG, dev, format, ##arg); 0; })
#endif

/* Create alias, so I can be autoloaded. */


2008-02-27 04:03:01

by Joe Perches

[permalink] [raw]
Subject: [PATCH] linux/fs.h - Convert debug functions declared inline __attribute__((format (printf,x,y) to statement expression macros

Converting inline __attribute__((format (printf,x,y) functions
to macros or statement expressions produces smaller objects

before:
$ size vmlinux
text data bss dec hex filename
4716770 474560 618496 5809826 58a6a2 vmlinux
after:
$ size vmlinux
text data bss dec hex filename
4716706 474560 618496 5809762 58a662 vmlinux

Signed-off-by: Joe Perches <[email protected]>

include/linux/fs.h | 9 ++++-----
1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index b84b848..a0ba590 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2055,11 +2055,10 @@ static struct file_operations __fops = { \
.write = simple_attr_write, \
};

-static inline void __attribute__((format(printf, 1, 2)))
-__simple_attr_check_format(const char *fmt, ...)
-{
- /* don't do anything, just let the compiler check the arguments; */
-}
+/* don't do anything, just let the compiler check the arguments; */
+
+#define __simple_attr_check_format(fmt, args...) \
+ do { if (0) printk(fmt, ##args); } while (0)

int simple_attr_open(struct inode *inode, struct file *file,
int (*get)(void *, u64 *), int (*set)(void *, u64),

2008-02-27 04:13:28

by Matthew Wilcox

[permalink] [raw]
Subject: Re: [PATCH] linux/fs.h - Convert debug functions declared inline __attribute__((format (printf,x,y) to statement expression macros

On Tue, Feb 26, 2008 at 08:02:27PM -0800, Joe Perches wrote:
> Converting inline __attribute__((format (printf,x,y) functions
> to macros or statement expressions produces smaller objects
>
> before:
> $ size vmlinux
> text data bss dec hex filename
> 4716770 474560 618496 5809826 58a6a2 vmlinux
> after:
> $ size vmlinux
> text data bss dec hex filename
> 4716706 474560 618496 5809762 58a662 vmlinux

> -static inline void __attribute__((format(printf, 1, 2)))
> -__simple_attr_check_format(const char *fmt, ...)
> -{
> - /* don't do anything, just let the compiler check the arguments; */
> -}
> +/* don't do anything, just let the compiler check the arguments; */
> +
> +#define __simple_attr_check_format(fmt, args...) \
> + do { if (0) printk(fmt, ##args); } while (0)

That's very interesting. It's only 64 bytes, but still, it's not
supposed to have any different effect. Could you distill a test case
for the GCC folks and file it in their bugzilla?

--
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."

2008-02-27 04:58:19

by Joe Perches

[permalink] [raw]
Subject: Re: [PATCH] linux/fs.h - Convert debug functions declared inline __attribute__((format (printf,x,y) to statement expression macros

On Tue, 2008-02-26 at 21:13 -0700, Matthew Wilcox wrote:
> That's very interesting. It's only 64 bytes, but still, it's not
> supposed to have any different effect.

Especially because __simple_attr_check_format is not even used
or called in an x86 defconfig. It's powerpc/cell specific.

> Could you distill a test case for the GCC folks

I'll play around with it.

2008-02-27 05:45:33

by David Rientjes

[permalink] [raw]
Subject: Re: [PATCH] linux/fs.h - Convert debug functions declared inline __attribute__((format (printf,x,y) to statement expression macros

On Tue, 26 Feb 2008, Matthew Wilcox wrote:

> On Tue, Feb 26, 2008 at 08:02:27PM -0800, Joe Perches wrote:
> > Converting inline __attribute__((format (printf,x,y) functions
> > to macros or statement expressions produces smaller objects
> >
> > before:
> > $ size vmlinux
> > text data bss dec hex filename
> > 4716770 474560 618496 5809826 58a6a2 vmlinux
> > after:
> > $ size vmlinux
> > text data bss dec hex filename
> > 4716706 474560 618496 5809762 58a662 vmlinux
>
> > -static inline void __attribute__((format(printf, 1, 2)))
> > -__simple_attr_check_format(const char *fmt, ...)
> > -{
> > - /* don't do anything, just let the compiler check the arguments; */
> > -}
> > +/* don't do anything, just let the compiler check the arguments; */
> > +
> > +#define __simple_attr_check_format(fmt, args...) \
> > + do { if (0) printk(fmt, ##args); } while (0)
>
> That's very interesting. It's only 64 bytes, but still, it's not
> supposed to have any different effect. Could you distill a test case
> for the GCC folks and file it in their bugzilla?
>

I'm not seeing any change in text size with allyesconfig after applying
this patch with latest git:

text data bss dec hex filename
32696210 5021759 6735572 44453541 2a64ea5 vmlinux.before
32696210 5021759 6735572 44453541 2a64ea5 vmlinux.after

Joe, what version of gcc are you using?

David

2008-02-27 06:56:54

by Joe Perches

[permalink] [raw]
Subject: Re: [PATCH] linux/fs.h - Convert debug functions declared inline __attribute__((format (printf,x,y) to statement expression macros

On Tue, 2008-02-26 at 21:44 -0800, David Rientjes wrote:
> I'm not seeing any change in text size with allyesconfig after applying
> this patch with latest git:

This is just x86 defconfig

> Joe, what version of gcc are you using?

$ gcc --version
gcc (GCC) 4.2.2 20071128 (prerelease) (4.2.2-3.1mdv2008.0)

It's definitely odd.
The .o size changes are inconsistent.
Some get bigger, some get smaller.

The versioning ones I understand but I have no idea why
changes in drivers/ or mm/ or net/ exist.

I think it's gcc optimization changes, but dunno...
Any good ideas?

$ git reset --hard
HEAD is now at 7704a8b... Merge branch 'for-linus' of git://oss.sgi.com:8090/xfs/xfs-2.6
$ make mrproper ; make defconfig ; make > /dev/null
$ size vmlinux
text data bss dec hex filename
4716770 474560 618496 5809826 58a6a2 vmlinux
$ size $(find -type f -print | grep "\.o$" | grep -vP "(vmlinux|built-in|piggy|allsyms.)\.o$") > size.default
$ patch -p1 < inline/fs.h.d
$ make > /dev/null
$ size vmlinux
text data bss dec hex filename
4716706 474560 618496 5809762 58a662 vmlinux
$ size $(find -type f -print | grep "\.o$" | grep -vP "(vmlinux|built-in|piggy|allsyms.)\.o$") > size.inline_fs
$ diff --unified=0 size.default size.inline_fs
--- size.default 2008-02-26 22:18:33.000000000 -0800
+++ size.inline_fs 2008-02-26 22:33:27.000000000 -0800
@@ -21 +21 @@
- 79 0 0 79 4f ./arch/x86/boot/version.o
+ 85 0 0 85 55 ./arch/x86/boot/version.o
@@ -335 +335 @@
- 5206 72 12 5290 14aa ./drivers/base/core.o
+ 5201 72 12 5285 14a5 ./drivers/base/core.o
@@ -374 +374 @@
- 18192 104 1648 19944 4de8 ./drivers/char/tty_io.o
+ 18184 104 1648 19936 4de0 ./drivers/char/tty_io.o
@@ -390 +390 @@
- 4293 560 24 4877 130d ./drivers/char/hpet.o
+ 4287 560 24 4871 1307 ./drivers/char/hpet.o
@@ -473 +473 @@
- 38914 32 341 39287 9977 ./drivers/message/fusion/mptbase.o
+ 38922 32 341 39295 997f ./drivers/message/fusion/mptbase.o
@@ -492 +492 @@
- 81665 2613 4 84282 1493a ./drivers/net/tg3.o
+ 81659 2613 4 84276 14934 ./drivers/net/tg3.o
@@ -544 +544 @@
- 17508 845 552 18905 49d9 ./drivers/scsi/aic7xxx/aic79xx_osm.o
+ 17510 845 552 18907 49db ./drivers/scsi/aic7xxx/aic79xx_osm.o
@@ -581 +581 @@
- 74 4480 0 4554 11ca ./drivers/scsi/scsi_wait_scan.mod.o
+ 80 4480 0 4560 11d0 ./drivers/scsi/scsi_wait_scan.mod.o
@@ -774 +774 @@
- 1924 4 4 1932 78c ./fs/proc/kcore.o
+ 1922 4 4 1930 78a ./fs/proc/kcore.o
@@ -776 +776 @@
- 41462 652 80 42194 a4d2 ./fs/proc/proc.o
+ 41458 652 80 42190 a4ce ./fs/proc/proc.o
@@ -828 +828 @@
- 9583 80 0 9663 25bf ./fs/locks.o
+ 9571 80 0 9651 25b3 ./fs/locks.o
@@ -870 +870 @@
- 277 396 4 677 2a5 ./init/version.o
+ 281 396 4 681 2a9 ./init/version.o
@@ -926 +926 @@
- 8379 460 8 8847 228f ./kernel/sys.o
+ 8381 460 8 8849 2291 ./kernel/sys.o
@@ -954 +954 @@
- 13337 188 73 13598 351e ./kernel/module.o
+ 13341 188 73 13602 3522 ./kernel/module.o
@@ -1044 +1044 @@
- 1845 0 0 1845 735 ./mm/mremap.o
+ 1841 0 0 1841 731 ./mm/mremap.o
@@ -1052 +1052 @@
- 8781 44 2196 11021 2b0d ./mm/swapfile.o
+ 8777 44 2196 11017 2b09 ./mm/swapfile.o
@@ -1065 +1065 @@
- 2630 0 0 2630 a46 ./net/core/datagram.o
+ 2631 0 0 2631 a47 ./net/core/datagram.o
@@ -1101 +1101 @@
- 13190 24 0 13214 339e ./net/ipv4/tcp_output.o
+ 13192 24 0 13216 33a0 ./net/ipv4/tcp_output.o
@@ -1109 +1109 @@
- 6244 468 0 6712 1a38 ./net/ipv4/arp.o
+ 6239 468 0 6707 1a33 ./net/ipv4/arp.o
@@ -1138 +1138 @@
- 4660 132 44 4836 12e4 ./net/ipv6/ip6_fib.o
+ 4644 132 44 4820 12d4 ./net/ipv6/ip6_fib.o
@@ -1146 +1146 @@
- 16397 24 4 16425 4029 ./net/ipv6/mcast.o
+ 16399 24 4 16427 402b ./net/ipv6/mcast.o
@@ -1159 +1159 @@
- 143799 7424 3036 154259 25a93 ./net/ipv6/ipv6.o
+ 143787 7424 3036 154247 25a87 ./net/ipv6/ipv6.o
@@ -1202 +1202 @@
- 2109 600 0 2709 a95 ./net/xfrm/xfrm_algo.o
+ 2111 600 0 2711 a97 ./net/xfrm/xfrm_algo.o

2008-02-27 07:39:42

by David Rientjes

[permalink] [raw]
Subject: Re: [PATCH] linux/fs.h - Convert debug functions declared inline __attribute__((format (printf,x,y) to statement expression macros

On Tue, 26 Feb 2008, Joe Perches wrote:

> > I'm not seeing any change in text size with allyesconfig after applying
> > this patch with latest git:
>
> This is just x86 defconfig
>

allyesconfig should be able to capture any text savings that this patch
offers.

> > Joe, what version of gcc are you using?
>
> $ gcc --version
> gcc (GCC) 4.2.2 20071128 (prerelease) (4.2.2-3.1mdv2008.0)
>

My x86_64 defconfig with gcc 4.0.3 had no difference in text size after
applying your patch, yet the same config on gcc 4.1.2 did:

text data bss dec hex filename
5386112 846328 719560 6952000 6a1440 vmlinux.before
5386048 846328 719560 6951936 6a1400 vmlinux.after

2008-02-27 22:59:21

by David Rientjes

[permalink] [raw]
Subject: Re: [PATCH] linux/fs.h - Convert debug functions declared inline __attribute__((format (printf,x,y) to statement expression macros

On Tue, 26 Feb 2008, Joe Perches wrote:

> > Joe, what version of gcc are you using?
>
> $ gcc --version
> gcc (GCC) 4.2.2 20071128 (prerelease) (4.2.2-3.1mdv2008.0)
>
> It's definitely odd.
> The .o size changes are inconsistent.
> Some get bigger, some get smaller.
>
> The versioning ones I understand but I have no idea why
> changes in drivers/ or mm/ or net/ exist.
>

When I did the same comparisons on my x86_64 defconfig with gcc 4.1.3, I
only saw differences in drivers/ and fs/.

> I think it's gcc optimization changes, but dunno...
> Any good ideas?
>

What's interesting about this is that it doesn't appear to be related to
your change (static inline function to macro definition). It appears to
be simply removing the static inline function.

The only reference to __simple_attr_check_format() in either the x86 or
x86_64 defconfig is via DEFINE_SIMPLE_ATTRIBUTE() in fs/debugfs/file.c.

If you remove the only reference to it:

diff --git a/include/linux/fs.h b/include/linux/fs.h
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2044,7 +2044,6 @@ static inline void simple_transaction_set(struct file *file, size_t n)
#define DEFINE_SIMPLE_ATTRIBUTE(__fops, __get, __set, __fmt) \
static int __fops ## _open(struct inode *inode, struct file *file) \
{ \
- __simple_attr_check_format(__fmt, 0ull); \
return simple_attr_open(inode, file, __get, __set, __fmt); \
} \
static struct file_operations __fops = { \

The text size remains the same:

text data bss dec hex filename
5386111 846328 719560 6951999 6a143f vmlinux.before
5386111 846328 719560 6951999 6a143f vmlinux.after

Yet if you remove the reference _and_ the static inline function itself,
replacing it with nothing:

diff --git a/include/linux/fs.h b/include/linux/fs.h
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2044,7 +2044,6 @@ static inline void simple_transaction_set(struct file *file, size_t n)
#define DEFINE_SIMPLE_ATTRIBUTE(__fops, __get, __set, __fmt) \
static int __fops ## _open(struct inode *inode, struct file *file) \
{ \
- __simple_attr_check_format(__fmt, 0ull); \
return simple_attr_open(inode, file, __get, __set, __fmt); \
} \
static struct file_operations __fops = { \
@@ -2055,12 +2054,6 @@ static struct file_operations __fops = { \
.write = simple_attr_write, \
};

-static inline void __attribute__((format(printf, 1, 2)))
-__simple_attr_check_format(const char *fmt, ...)
-{
- /* don't do anything, just let the compiler check the arguments; */
-}
-
int simple_attr_open(struct inode *inode, struct file *file,
int (*get)(void *, u64 *), int (*set)(void *, u64),
const char *fmt);

The text size does become smaller:

text data bss dec hex filename
5386111 846328 719560 6951999 6a143f vmlinux.before
5386047 846328 719560 6951935 6a13ff vmlinux.after

gcc 4.0.3 maintains the same text size for both cases, while it appears
gcc 4.1.3 and your version, 4.2.2, have this different behavior.

David

2008-02-27 23:58:48

by Jan Hubicka

[permalink] [raw]
Subject: Re: [PATCH] linux/fs.h - Convert debug functions declared inline __attribute__((format (printf,x,y) to statement expression macros

>
> -static inline void __attribute__((format(printf, 1, 2)))
> -__simple_attr_check_format(const char *fmt, ...)

It would be nice to have a testcase, but I guess it is because GCC can't
inline variadic functions. The function gets identified as const and
removed as unused by DCE, but this happens later (that is after early
inlining and before real inlining). GCC 4.0.3 didn't have early inliner
so it is probably where the difference is comming from.

One possibility to handle this side case would be to mark const
functions early during early optimization and only refine it using
Kenny's existing IPA pass that should turn this issue into no-op.

We probably also can simply allow inlining variadic functions not
calling va_start. I must say that this option appeared to me but I was
unable to think of any sane use case. This probably is one ;)

Honza
> -{
> - /* don't do anything, just let the compiler check the arguments; */
> -}
> -
> int simple_attr_open(struct inode *inode, struct file *file,
> int (*get)(void *, u64 *), int (*set)(void *, u64),
> const char *fmt);
>
> The text size does become smaller:
>
> text data bss dec hex filename
> 5386111 846328 719560 6951999 6a143f vmlinux.before
> 5386047 846328 719560 6951935 6a13ff vmlinux.after
>
> gcc 4.0.3 maintains the same text size for both cases, while it appears
> gcc 4.1.3 and your version, 4.2.2, have this different behavior.
>
> David

2008-02-28 08:29:28

by David Rientjes

[permalink] [raw]
Subject: Re: [PATCH] linux/fs.h - Convert debug functions declared inline __attribute__((format (printf,x,y) to statement expression macros

On Thu, 28 Feb 2008, Jan Hubicka wrote:

> > -static inline void __attribute__((format(printf, 1, 2)))
> > -__simple_attr_check_format(const char *fmt, ...)
>
> It would be nice to have a testcase, but I guess it is because GCC can't
> inline variadic functions. The function gets identified as const and
> removed as unused by DCE, but this happens later (that is after early
> inlining and before real inlining). GCC 4.0.3 didn't have early inliner
> so it is probably where the difference is comming from.
>
> One possibility to handle this side case would be to mark const
> functions early during early optimization and only refine it using
> Kenny's existing IPA pass that should turn this issue into no-op.
>
> We probably also can simply allow inlining variadic functions not
> calling va_start. I must say that this option appeared to me but I was
> unable to think of any sane use case. This probably is one ;)
>

The only testcase I can identify is the current version of Linux, because
I've certainly never seen this type of behavior before.

What's interesting is that you can remove __simple_attr_check_format()
completely and replace it with an empty function definition:

static inline void completely_useless_function(void)
{
}

that is not referenced anywhere in the tree:

diff --git a/include/linux/fs.h b/include/linux/fs.h
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2044,7 +2044,6 @@ static inline void simple_transaction_set(struct file *file, size_t n)
#define DEFINE_SIMPLE_ATTRIBUTE(__fops, __get, __set, __fmt) \
static int __fops ## _open(struct inode *inode, struct file *file) \
{ \
- __simple_attr_check_format(__fmt, 0ull); \
return simple_attr_open(inode, file, __get, __set, __fmt); \
} \
static struct file_operations __fops = { \
@@ -2055,10 +2054,8 @@ static struct file_operations __fops = { \
.write = simple_attr_write, \
};

-static inline void __attribute__((format(printf, 1, 2)))
-__simple_attr_check_format(const char *fmt, ...)
+static inline void completely_useless_function(void)
{
- /* don't do anything, just let the compiler check the arguments; */
}

int simple_attr_open(struct inode *inode, struct file *file,


And the size remains identical after this patch has been applied:

text data bss dec hex filename
5386111 846328 719560 6951999 6a143f vmlinux.before
5386111 846328 719560 6951999 6a143f vmlinux.after

Yet, if you subsequently remove completely_useless_function(), the image
is smaller:


diff --git a/include/linux/fs.h b/include/linux/fs.h
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2044,7 +2044,6 @@ static inline void simple_transaction_set(struct file *file, size_t n)
#define DEFINE_SIMPLE_ATTRIBUTE(__fops, __get, __set, __fmt) \
static int __fops ## _open(struct inode *inode, struct file *file) \
{ \
- __simple_attr_check_format(__fmt, 0ull); \
return simple_attr_open(inode, file, __get, __set, __fmt); \
} \
static struct file_operations __fops = { \
@@ -2055,12 +2054,6 @@ static struct file_operations __fops = { \
.write = simple_attr_write, \
};

-static inline void __attribute__((format(printf, 1, 2)))
-__simple_attr_check_format(const char *fmt, ...)
-{
- /* don't do anything, just let the compiler check the arguments; */
-}
-
int simple_attr_open(struct inode *inode, struct file *file,
int (*get)(void *, u64 *), int (*set)(void *, u64),
const char *fmt);


Then:

text data bss dec hex filename
5386111 846328 719560 6951999 6a143f vmlinux.before
5386047 846328 719560 6951935 6a13ff vmlinux.after

So the only thing I can imagine is that gcc 4.1.3 and gcc 4.2.2 emit a
larger text size simply based on the number of functions being defined,
regardless of whether they are referenced in the source or not. gcc 4.0.3
doesn't have this behavior.

It's easy to reproduce:

$ wget http://www.kernel.org/pub/linux/kernel/v2.6/linux-2.6.24.3.tar.bz2
$ tar xvf linux-2.6.24.3.tar.bz2
$ cd linux-2.6.24.3
$ make defconfig ARCH=x86_64
$ make ARCH=x86_64
$ size vmlinux
$ patch -p1 < remove-simple-attr-check-format.patch
$ make ARCH=x86_64
$ size vmlinux

2008-02-28 08:43:25

by Jakub Jelinek

[permalink] [raw]
Subject: Re: [PATCH] linux/fs.h - Convert debug functions declared inline __attribute__((format (printf,x,y) to statement expression macros

On Thu, Feb 28, 2008 at 12:58:35AM +0100, Jan Hubicka wrote:
> We probably also can simply allow inlining variadic functions not
> calling va_start. I must say that this option appeared to me but I was
> unable to think of any sane use case. This probably is one ;)

We already allow inlining variadic functions not calling va_start, already
3.2.x did that and so do all following gccs. In 4.3+
__builtin_va_arg_pack{,_len} support was added, so that you can even pass
the ... arguments to variable length functions, query their count etc.

Jakub

2008-02-28 10:23:36

by Jan Hubicka

[permalink] [raw]
Subject: Re: [PATCH] linux/fs.h - Convert debug functions declared inline __attribute__((format (printf,x,y) to statement expression macros

> On Thu, Feb 28, 2008 at 12:58:35AM +0100, Jan Hubicka wrote:
> > We probably also can simply allow inlining variadic functions not
> > calling va_start. I must say that this option appeared to me but I was
> > unable to think of any sane use case. This probably is one ;)
>
> We already allow inlining variadic functions not calling va_start, already
> 3.2.x did that and so do all following gccs. In 4.3+
> __builtin_va_arg_pack{,_len} support was added, so that you can even pass
> the ... arguments to variable length functions, query their count etc.

Hmm, I now remember this change. Thanks for pointing it out ;) I guess
it is just more or less random difference then (ie different ordering of
hashtables). The difference is tiny anyway. The call ought to be always
early inlined and not seen by any optimization pass.

Honza
>
> Jakub

2008-02-29 01:14:19

by Joe Perches

[permalink] [raw]
Subject: Re: [PATCH] linux/fs.h - Convert debug functions declared inline __attribute__((format (printf,x,y) to statement expression macros

On Thu, 2008-02-28 at 11:23 +0100, Jan Hubicka wrote:
> The call ought to be always
> early inlined and not seen by any optimization pass.

The inlined functions don't actually appear in the generated code.

Look at the code generation differences for kernel/sched.c
function place_entity

$ size sched.inline.o sched.if0.o
text data bss dec hex filename
31385 2854 328 34567 8707 sched.inline.o
31366 2854 328 34548 86f4 sched.if0.o

The current preprocessed only kernel/sched.i file contains:

# 612 "include/linux/device.h"
static inline __attribute__((always_inline)) int __attribute__ ((format (printf, 2, 3)))
dev_dbg(struct device *dev, const char *fmt, ...)
{
return 0;
}
# 628 "include/linux/device.h"
static inline __attribute__((always_inline)) int __attribute__ ((format (printf, 2, 3)))
dev_vdbg(struct device *dev, const char *fmt, ...)
{
return 0;
}

But the function place_entity doesn't use it directly or indirectly.
If the lines above are removed, the generated code for place_entity changes.

static void
place_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int initial)
{
u64 vruntime;

vruntime = cfs_rq->min_vruntime;

if (sched_feat(TREE_AVG)) {
struct sched_entity *last = __pick_last_entity(cfs_rq);
if (last) {
vruntime += last->vruntime;
vruntime >>= 1;
}
} else if (sched_feat(APPROX_AVG) && cfs_rq->nr_running)
vruntime += sched_vslice(cfs_rq)/2;

/*
* The 'current' period is already promised to the current tasks,
* however the extra weight of the new task will slow them down a
* little, place the new task so that it fits in the slot that
* stays open at the end.
*/
if (initial && sched_feat(START_DEBIT))
vruntime += sched_vslice_add(cfs_rq, se);

if (!initial) {
/* sleeps upto a single latency don't count. */
if (sched_feat(NEW_FAIR_SLEEPERS))
vruntime -= sysctl_sched_latency;

/* ensure we never gain time by being placed backwards. */
vruntime = max_vruntime(se->vruntime, vruntime);
}

se->vruntime = vruntime;
}

2008-03-23 15:24:14

by Denys Vlasenko

[permalink] [raw]
Subject: Re: [PATCH] linux/fs.h - Convert debug functions declared inline __attribute__((format (printf,x,y) to statement expression macros

On Friday 29 February 2008 02:09, Joe Perches wrote:
> But the function place_entity doesn't use it directly or indirectly.
> If the lines above are removed, the generated code for place_entity changes.

I see it all the time. Whenever I add/remove/change something
to a header, some functions grow a tiny bit, some shrink a bit.
gcc 4.2.1. Report is here:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=29950
--
vda

2008-03-24 19:58:20

by Joe Perches

[permalink] [raw]
Subject: Re: [PATCH] linux/fs.h - Convert debug functions declared inline __attribute__((format (printf,x,y) to statement expression macros

On Sun, 2008-03-23 at 16:22 +0100, Denys Vlasenko wrote:
> On Friday 29 February 2008 02:09, Joe Perches wrote:
> > But the function place_entity doesn't use it directly or indirectly.
> > If the lines above are removed, the generated code for place_entity changes.
> I see it all the time. Whenever I add/remove/change something
> to a header, some functions grow a tiny bit, some shrink a bit.
> gcc 4.2.1. Report is here:
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=29950

This seems more like a machine descriptions or target pass defect
than an RTL problem.

Should this defect be classified in group "Target" rather than "RTL"?