| Message ID | 20181103153941.1881966-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: avoid link error with CONFIG_NO_AUTO_INLINE | expand |
On 3.11.18 г. 17:39 ч., Arnd Bergmann wrote: > On 32-bit ARM with gcc-8, I see a link error with the addition of the > CONFIG_NO_AUTO_INLINE option: > > fs/btrfs/super.o: In function `btrfs_statfs': > super.c:(.text+0x67b8): undefined reference to `__aeabi_uldivmod' > super.c:(.text+0x67fc): undefined reference to `__aeabi_uldivmod' > super.c:(.text+0x6858): undefined reference to `__aeabi_uldivmod' > super.c:(.text+0x6920): undefined reference to `__aeabi_uldivmod' > super.c:(.text+0x693c): undefined reference to `__aeabi_uldivmod' > fs/btrfs/super.o:super.c:(.text+0x6958): more undefined references to `__aeabi_uldivmod' follow > > So far this is the only file that shows the behavior, so I'd propose > to just work around it by marking the functions as 'static inline' > that normally get inlined here. > > The reference to __aeabi_uldivmod comes from a div_u64() which has an > optimization for a constant division that uses a straight '/' operator > when the result should be known to the compiler. My interpretation is > that as we turn off inlining, gcc still expects the result to be constant > but fails to use that constant value. I read this as "this is a compiler bug", no ? So you are providing a hack around a compiler bug? > > Cc: Changbin Du <changbin.du@gmail.com> > Fixes: 943b8435c3bd ("kernel hacking: add a config option to disable compiler auto-inlining") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > fs/btrfs/super.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > index c3c1e7bee49d..b7af0b8936ad 100644 > --- a/fs/btrfs/super.c > +++ b/fs/btrfs/super.c > @@ -1922,7 +1922,7 @@ static int btrfs_remount(struct super_block *sb, int *flags, > } > > /* Used to sort the devices by max_avail(descending sort) */ > -static int btrfs_cmp_device_free_bytes(const void *dev_info1, > +static inline int btrfs_cmp_device_free_bytes(const void *dev_info1, > const void *dev_info2) > { > if (((struct btrfs_device_info *)dev_info1)->max_avail > > @@ -1951,8 +1951,8 @@ static inline void btrfs_descending_sort_devices( > * The helper to calc the free space on the devices that can be used to store > * file data. > */ > -static int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info, > - u64 *free_bytes) > +static inline int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info, > + u64 *free_bytes) > { > struct btrfs_device_info *devices_info; > struct btrfs_fs_devices *fs_devices = fs_info->fs_devices; >
On 11/3/18, Nikolay Borisov <nborisov@suse.com> wrote: > On 3.11.18 г. 17:39 ч., Arnd Bergmann wrote: >> On 32-bit ARM with gcc-8, I see a link error with the addition of the >> CONFIG_NO_AUTO_INLINE option: >> >> fs/btrfs/super.o: In function `btrfs_statfs': >> super.c:(.text+0x67b8): undefined reference to `__aeabi_uldivmod' >> super.c:(.text+0x67fc): undefined reference to `__aeabi_uldivmod' >> super.c:(.text+0x6858): undefined reference to `__aeabi_uldivmod' >> super.c:(.text+0x6920): undefined reference to `__aeabi_uldivmod' >> super.c:(.text+0x693c): undefined reference to `__aeabi_uldivmod' >> fs/btrfs/super.o:super.c:(.text+0x6958): more undefined references to >> `__aeabi_uldivmod' follow >> >> So far this is the only file that shows the behavior, so I'd propose >> to just work around it by marking the functions as 'static inline' >> that normally get inlined here. >> >> The reference to __aeabi_uldivmod comes from a div_u64() which has an >> optimization for a constant division that uses a straight '/' operator >> when the result should be known to the compiler. My interpretation is >> that as we turn off inlining, gcc still expects the result to be constant >> but fails to use that constant value. > > I read this as "this is a compiler bug", no ? So you are providing a > hack around a compiler bug? Mostly, yes. The do_div() macro is really pushing the boundaries of what we can expect the compiler to do in terms of optimizations, and we've had problems with it in the past. IIRC the gcc developers would not classify this as a bug because the result of __builtin_constant_p() is not guaranteed to work the way we expect, it just does so most of the time. Arnd
On 2018/11/3 下午11:39, Arnd Bergmann wrote: > On 32-bit ARM with gcc-8, I see a link error with the addition of the > CONFIG_NO_AUTO_INLINE option: > > fs/btrfs/super.o: In function `btrfs_statfs': > super.c:(.text+0x67b8): undefined reference to `__aeabi_uldivmod' > super.c:(.text+0x67fc): undefined reference to `__aeabi_uldivmod' > super.c:(.text+0x6858): undefined reference to `__aeabi_uldivmod' > super.c:(.text+0x6920): undefined reference to `__aeabi_uldivmod' > super.c:(.text+0x693c): undefined reference to `__aeabi_uldivmod' > fs/btrfs/super.o:super.c:(.text+0x6958): more undefined references to `__aeabi_uldivmod' follow > > So far this is the only file that shows the behavior, so I'd propose > to just work around it by marking the functions as 'static inline' > that normally get inlined here. As a workaround it looks OK, but it's definitely not the root cause. > > The reference to __aeabi_uldivmod comes from a div_u64() which has an > optimization for a constant division that uses a straight '/' operator > when the result should be known to the compiler. My interpretation is > that as we turn off inlining, gcc still expects the result to be constant > but fails to use that constant value. It looks more like a bug in div_u64() optimization. Despite this file in btrfs, did you hit the same problem for any other file? > > Cc: Changbin Du <changbin.du@gmail.com> > Fixes: 943b8435c3bd ("kernel hacking: add a config option to disable compiler auto-inlining") I can't find it in the mainline kernel, is the commit hash correct? If not merged, we should still has a chance to further polish that patch. Thanks, Qu > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > fs/btrfs/super.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > index c3c1e7bee49d..b7af0b8936ad 100644 > --- a/fs/btrfs/super.c > +++ b/fs/btrfs/super.c > @@ -1922,7 +1922,7 @@ static int btrfs_remount(struct super_block *sb, int *flags, > } > > /* Used to sort the devices by max_avail(descending sort) */ > -static int btrfs_cmp_device_free_bytes(const void *dev_info1, > +static inline int btrfs_cmp_device_free_bytes(const void *dev_info1, > const void *dev_info2) > { > if (((struct btrfs_device_info *)dev_info1)->max_avail > > @@ -1951,8 +1951,8 @@ static inline void btrfs_descending_sort_devices( > * The helper to calc the free space on the devices that can be used to store > * file data. > */ > -static int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info, > - u64 *free_bytes) > +static inline int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info, > + u64 *free_bytes) > { > struct btrfs_device_info *devices_info; > struct btrfs_fs_devices *fs_devices = fs_info->fs_devices; >
On 11/4/18, Qu Wenruo <wqu@suse.de> wrote: > > > On 2018/11/3 下午11:39, Arnd Bergmann wrote: >> On 32-bit ARM with gcc-8, I see a link error with the addition of the >> CONFIG_NO_AUTO_INLINE option: >> >> fs/btrfs/super.o: In function `btrfs_statfs': >> super.c:(.text+0x67b8): undefined reference to `__aeabi_uldivmod' >> super.c:(.text+0x67fc): undefined reference to `__aeabi_uldivmod' >> super.c:(.text+0x6858): undefined reference to `__aeabi_uldivmod' >> super.c:(.text+0x6920): undefined reference to `__aeabi_uldivmod' >> super.c:(.text+0x693c): undefined reference to `__aeabi_uldivmod' >> fs/btrfs/super.o:super.c:(.text+0x6958): more undefined references to >> `__aeabi_uldivmod' follow >> >> So far this is the only file that shows the behavior, so I'd propose >> to just work around it by marking the functions as 'static inline' >> that normally get inlined here. > > As a workaround it looks OK, but it's definitely not the root cause. > >> >> The reference to __aeabi_uldivmod comes from a div_u64() which has an >> optimization for a constant division that uses a straight '/' operator >> when the result should be known to the compiler. My interpretation is >> that as we turn off inlining, gcc still expects the result to be constant >> but fails to use that constant value. > > It looks more like a bug in div_u64() optimization. > > Despite this file in btrfs, did you hit the same problem for any other > file? Not this time. I've done a creduce on the file and got to this code struct kstatfs { u64 f_bfree; }; btrfs_calc_avail_data_space(p1) {} btrfs_statfs(struct kstatfs *p1) { u64 d = 0; unsigned e = 1; for (; a;) e = btrfs_bg_type_to_factor(); p1->f_bfree = div_u64(0, e) >> c; __asm__(""); div_u64(d, e); b = btrfs_calc_avail_data_space(&d); } Looking at the assembler code produced by this, it seems to be the same thing that we dealt with in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=72785 >> Cc: Changbin Du <changbin.du@gmail.com> >> Fixes: 943b8435c3bd ("kernel hacking: add a config option to disable >> compiler auto-inlining") > > I can't find it in the mainline kernel, is the commit hash correct? > If not merged, we should still has a chance to further polish that patch. It's in linux-next. Arnd
On 3.11.18 г. 17:39 ч., Arnd Bergmann wrote: > On 32-bit ARM with gcc-8, I see a link error with the addition of the > CONFIG_NO_AUTO_INLINE option: > > fs/btrfs/super.o: In function `btrfs_statfs': > super.c:(.text+0x67b8): undefined reference to `__aeabi_uldivmod' > super.c:(.text+0x67fc): undefined reference to `__aeabi_uldivmod' > super.c:(.text+0x6858): undefined reference to `__aeabi_uldivmod' > super.c:(.text+0x6920): undefined reference to `__aeabi_uldivmod' > super.c:(.text+0x693c): undefined reference to `__aeabi_uldivmod' > fs/btrfs/super.o:super.c:(.text+0x6958): more undefined references to `__aeabi_uldivmod' follow > > So far this is the only file that shows the behavior, so I'd propose > to just work around it by marking the functions as 'static inline' > that normally get inlined here. > > The reference to __aeabi_uldivmod comes from a div_u64() which has an > optimization for a constant division that uses a straight '/' operator > when the result should be known to the compiler. My interpretation is > that as we turn off inlining, gcc still expects the result to be constant > but fails to use that constant value. > > Cc: Changbin Du <changbin.du@gmail.com> > Fixes: 943b8435c3bd ("kernel hacking: add a config option to disable compiler auto-inlining") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> I spoke with Arnd on irc and am fine with taking this patch as-is if btrfs is the sole offender. So: Reviewed-by: Nikolay Borisov <nborisov@suse.com> > --- > fs/btrfs/super.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > index c3c1e7bee49d..b7af0b8936ad 100644 > --- a/fs/btrfs/super.c > +++ b/fs/btrfs/super.c > @@ -1922,7 +1922,7 @@ static int btrfs_remount(struct super_block *sb, int *flags, > } > > /* Used to sort the devices by max_avail(descending sort) */ > -static int btrfs_cmp_device_free_bytes(const void *dev_info1, > +static inline int btrfs_cmp_device_free_bytes(const void *dev_info1, > const void *dev_info2) > { > if (((struct btrfs_device_info *)dev_info1)->max_avail > > @@ -1951,8 +1951,8 @@ static inline void btrfs_descending_sort_devices( > * The helper to calc the free space on the devices that can be used to store > * file data. > */ > -static int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info, > - u64 *free_bytes) > +static inline int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info, > + u64 *free_bytes) > { > struct btrfs_device_info *devices_info; > struct btrfs_fs_devices *fs_devices = fs_info->fs_devices; >
Thanks for this fix. Reviewed-by: Changbin Du <changbin.du@gmail.com> On Sat, Nov 03, 2018 at 04:39:28PM +0100, Arnd Bergmann wrote: > On 32-bit ARM with gcc-8, I see a link error with the addition of the > CONFIG_NO_AUTO_INLINE option: > > fs/btrfs/super.o: In function `btrfs_statfs': > super.c:(.text+0x67b8): undefined reference to `__aeabi_uldivmod' > super.c:(.text+0x67fc): undefined reference to `__aeabi_uldivmod' > super.c:(.text+0x6858): undefined reference to `__aeabi_uldivmod' > super.c:(.text+0x6920): undefined reference to `__aeabi_uldivmod' > super.c:(.text+0x693c): undefined reference to `__aeabi_uldivmod' > fs/btrfs/super.o:super.c:(.text+0x6958): more undefined references to `__aeabi_uldivmod' follow > > So far this is the only file that shows the behavior, so I'd propose > to just work around it by marking the functions as 'static inline' > that normally get inlined here. > > The reference to __aeabi_uldivmod comes from a div_u64() which has an > optimization for a constant division that uses a straight '/' operator > when the result should be known to the compiler. My interpretation is > that as we turn off inlining, gcc still expects the result to be constant > but fails to use that constant value. > > Cc: Changbin Du <changbin.du@gmail.com> > Fixes: 943b8435c3bd ("kernel hacking: add a config option to disable compiler auto-inlining") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > fs/btrfs/super.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > index c3c1e7bee49d..b7af0b8936ad 100644 > --- a/fs/btrfs/super.c > +++ b/fs/btrfs/super.c > @@ -1922,7 +1922,7 @@ static int btrfs_remount(struct super_block *sb, int *flags, > } > > /* Used to sort the devices by max_avail(descending sort) */ > -static int btrfs_cmp_device_free_bytes(const void *dev_info1, > +static inline int btrfs_cmp_device_free_bytes(const void *dev_info1, > const void *dev_info2) > { > if (((struct btrfs_device_info *)dev_info1)->max_avail > > @@ -1951,8 +1951,8 @@ static inline void btrfs_descending_sort_devices( > * The helper to calc the free space on the devices that can be used to store > * file data. > */ > -static int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info, > - u64 *free_bytes) > +static inline int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info, > + u64 *free_bytes) > { > struct btrfs_device_info *devices_info; > struct btrfs_fs_devices *fs_devices = fs_info->fs_devices; > -- > 2.18.0 >
On Sun, Nov 04, 2018 at 11:32:03PM +0100, Arnd Bergmann wrote: > >> Cc: Changbin Du <changbin.du@gmail.com> > >> Fixes: 943b8435c3bd ("kernel hacking: add a config option to disable > >> compiler auto-inlining") > > > > I can't find it in the mainline kernel, is the commit hash correct? > > If not merged, we should still has a chance to further polish that patch. > > It's in linux-next. I can't find it in current linux-next either, the final reference in Fixes: must refer to a commit in Linus' tree. You can take this fix with the patch that introduces the config option (ack for that) in case merging through the btrfs tree would be too late for it (ie. no common base for the git trees containg the new code and fix). Or I can take it through btrfs tree in 4.20-rc cycle. In both cases the Fixes: does not need to be there.
On 11/5/18, David Sterba <dsterba@suse.cz> wrote: > On Sun, Nov 04, 2018 at 11:32:03PM +0100, Arnd Bergmann wrote: >> >> Cc: Changbin Du <changbin.du@gmail.com> >> >> Fixes: 943b8435c3bd ("kernel hacking: add a config option to disable >> >> compiler auto-inlining") >> > >> > I can't find it in the mainline kernel, is the commit hash correct? >> > If not merged, we should still has a chance to further polish that >> > patch. >> >> It's in linux-next. > > I can't find it in current linux-next either, the final reference in > Fixes: must refer to a commit in Linus' tree. Ah, right, it got rebased. The commit ID in today's linux-next is 917fad29febd. Most trees in linux-next don't rebase so this would not be an issue, but you are right that this one clearly did, so the line is wrong here. The commit is now delayed until 4.21 I assume. > You can take this fix with the patch that introduces the config option > (ack for that) in case merging through the btrfs tree would be too late > for it (ie. no common base for the git trees containg the new code and > fix). > > Or I can take it through btrfs tree in 4.20-rc cycle. In both cases the > Fixes: does not need to be there. Please take it through the btrfs tree. Let me know if you need me to extend the changelog to explain that situation better. Arnd
On Mon, Nov 05, 2018 at 10:27:03PM +0100, Arnd Bergmann wrote: > On 11/5/18, David Sterba <dsterba@suse.cz> wrote: > > On Sun, Nov 04, 2018 at 11:32:03PM +0100, Arnd Bergmann wrote: > >> >> Cc: Changbin Du <changbin.du@gmail.com> > >> >> Fixes: 943b8435c3bd ("kernel hacking: add a config option to disable > >> >> compiler auto-inlining") > >> > > >> > I can't find it in the mainline kernel, is the commit hash correct? > >> > If not merged, we should still has a chance to further polish that > >> > patch. > >> > >> It's in linux-next. > > > > I can't find it in current linux-next either, the final reference in > > Fixes: must refer to a commit in Linus' tree. > > Ah, right, it got rebased. The commit ID in today's linux-next > is 917fad29febd. Most trees in linux-next don't rebase so this > would not be an issue, but you are right that this one clearly > did, so the line is wrong here. > > The commit is now delayed until 4.21 I assume. > > > You can take this fix with the patch that introduces the config option > > (ack for that) in case merging through the btrfs tree would be too late > > for it (ie. no common base for the git trees containg the new code and > > fix). > > > > Or I can take it through btrfs tree in 4.20-rc cycle. In both cases the > > Fixes: does not need to be there. > > Please take it through the btrfs tree. Let me know if you need me > to extend the changelog to explain that situation better. Ok, I'll add it to my tree, the changelog is good, thanks.
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index c3c1e7bee49d..b7af0b8936ad 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -1922,7 +1922,7 @@ static int btrfs_remount(struct super_block *sb, int *flags, } /* Used to sort the devices by max_avail(descending sort) */ -static int btrfs_cmp_device_free_bytes(const void *dev_info1, +static inline int btrfs_cmp_device_free_bytes(const void *dev_info1, const void *dev_info2) { if (((struct btrfs_device_info *)dev_info1)->max_avail > @@ -1951,8 +1951,8 @@ static inline void btrfs_descending_sort_devices( * The helper to calc the free space on the devices that can be used to store * file data. */ -static int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info, - u64 *free_bytes) +static inline int btrfs_calc_avail_data_space(struct btrfs_fs_info *fs_info, + u64 *free_bytes) { struct btrfs_device_info *devices_info; struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
On 32-bit ARM with gcc-8, I see a link error with the addition of the CONFIG_NO_AUTO_INLINE option: fs/btrfs/super.o: In function `btrfs_statfs': super.c:(.text+0x67b8): undefined reference to `__aeabi_uldivmod' super.c:(.text+0x67fc): undefined reference to `__aeabi_uldivmod' super.c:(.text+0x6858): undefined reference to `__aeabi_uldivmod' super.c:(.text+0x6920): undefined reference to `__aeabi_uldivmod' super.c:(.text+0x693c): undefined reference to `__aeabi_uldivmod' fs/btrfs/super.o:super.c:(.text+0x6958): more undefined references to `__aeabi_uldivmod' follow So far this is the only file that shows the behavior, so I'd propose to just work around it by marking the functions as 'static inline' that normally get inlined here. The reference to __aeabi_uldivmod comes from a div_u64() which has an optimization for a constant division that uses a straight '/' operator when the result should be known to the compiler. My interpretation is that as we turn off inlining, gcc still expects the result to be constant but fails to use that constant value. Cc: Changbin Du <changbin.du@gmail.com> Fixes: 943b8435c3bd ("kernel hacking: add a config option to disable compiler auto-inlining") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- fs/btrfs/super.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)