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
> >textdata bss dec hex filename
> > 4716770 474560 618496 5809826 58a6a2 vmlinux
> > after:
> > $ size vmlinux
> >textdata 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:
textdata bss dec hex filename
326962105021759 6735572 444535412a64ea5 vmlinux.before
326962105021759 6735572 444535412a64ea5 vmlinux.after
Joe, what version of gcc are you using?
David
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: textdata bss dec hex filename 5386112 846328 719560 6952000 6a1440 vmlinux.before 5386048 846328 719560 6951936 6a1400 vmlinux.after
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:
textdata 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:
textdata 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
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:
textdata 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:
textdata 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
