| Message ID | a2b9920e-8f65-31d8-8809-a862213117df@sandeen.net (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [V2] xfsprogs: do not redeclare globals provided by libraries | expand |
On Wed, Jan 29, 2020 at 09:16:58AM -0600, Eric Sandeen wrote: > In each of these cases, db, logprint, and mdrestore are redeclaring > as a global variable something which was already provided by a > library they link with. > > gcc now defaults to -fno-common and trips over these global variables > which are declared in utilities as well as in libxfs and libxlog, and > it causes the build to fail. > > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > > V2: unmangle whitespace, I'm a n00b. BOFH FTW > diff --git a/db/init.c b/db/init.c > index 455220a..0ac3736 100644 > --- a/db/init.c > +++ b/db/init.c > @@ -27,7 +27,6 @@ static int force; > static struct xfs_mount xmount; > struct xfs_mount *mp; > static struct xlog xlog; > -libxfs_init_t x; > xfs_agnumber_t cur_agno = NULLAGNUMBER; > > static void > diff --git a/logprint/logprint.c b/logprint/logprint.c > index 7754a2a..5809af9 100644 > --- a/logprint/logprint.c > +++ b/logprint/logprint.c > @@ -24,7 +24,6 @@ int print_buffer; > int print_overwrite; > int print_no_data; > int print_no_print; > -int print_exit = 1; /* -e is now default. specify -c to override */ > static int print_operation = OP_PRINT; > > static void > @@ -132,6 +131,7 @@ main(int argc, char **argv) > bindtextdomain(PACKAGE, LOCALEDIR); > textdomain(PACKAGE); > memset(&mount, 0, sizeof(mount)); > + print_exit = 1; /* -e is now default. specify -c to override */ With the trailing whitespace after the comment fixed, Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> Though given your earlier comment on IRC, maybe we should investigate why -fno-common would be useful (since Fedora turned it on??) or if it should be in the regular build to catch multiply defined global vars? --D > > progname = basename(argv[0]); > while ((c = getopt(argc, argv, "bC:cdefl:iqnors:tDVv")) != EOF) { > @@ -152,7 +152,7 @@ main(int argc, char **argv) > case 'e': > /* -e is now default > */ > - print_exit++; > + print_exit = 1; > break; > case 'C': > print_operation = OP_COPY; > diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c > index 3375e08..1cd399d 100644 > --- a/mdrestore/xfs_mdrestore.c > +++ b/mdrestore/xfs_mdrestore.c > @@ -7,7 +7,6 @@ > #include "libxfs.h" > #include "xfs_metadump.h" > > -char *progname; > static int show_progress = 0; > static int show_info = 0; > static int progress_since_warning = 0; > >
On 1/29/20 10:01 AM, Darrick J. Wong wrote: > On Wed, Jan 29, 2020 at 09:16:58AM -0600, Eric Sandeen wrote: >> In each of these cases, db, logprint, and mdrestore are redeclaring >> as a global variable something which was already provided by a >> library they link with. >> >> gcc now defaults to -fno-common and trips over these global variables >> which are declared in utilities as well as in libxfs and libxlog, and >> it causes the build to fail. >> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> >> --- ... >> + print_exit = 1; /* -e is now default. specify -c to override */ > > With the trailing whitespace after the comment fixed, oh burn. ;) > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > > Though given your earlier comment on IRC, maybe we should investigate > why -fno-common would be useful (since Fedora turned it on??) or if it > should be in the regular build to catch multiply defined global vars? That's a good idea. FWIW my understanding is that it's a new default in gcc10. Docs say w.r.t. -fcommon: "This behavior is inconsistent with C++, and on many targets implies a speed and code size penalty on global variable references. It is mainly useful to enable legacy code to link without errors." Want me to send a V3 w/ -fno-common explicitly set? -Eric
And with unmangled whitespaces it looks even better :)
Reviewed-by: Christoph Hellwig <hch@lst.de>
On Wed, Jan 29, 2020 at 10:57:48AM -0600, Eric Sandeen wrote: > On 1/29/20 10:01 AM, Darrick J. Wong wrote: > > On Wed, Jan 29, 2020 at 09:16:58AM -0600, Eric Sandeen wrote: > >> In each of these cases, db, logprint, and mdrestore are redeclaring > >> as a global variable something which was already provided by a > >> library they link with. > >> > >> gcc now defaults to -fno-common and trips over these global variables > >> which are declared in utilities as well as in libxfs and libxlog, and > >> it causes the build to fail. > >> > >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> > >> --- > > ... > > >> + print_exit = 1; /* -e is now default. specify -c to override */ > > > > With the trailing whitespace after the comment fixed, > > oh burn. ;) > > > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > > > > Though given your earlier comment on IRC, maybe we should investigate > > why -fno-common would be useful (since Fedora turned it on??) or if it > > should be in the regular build to catch multiply defined global vars? > > That's a good idea. FWIW my understanding is that it's a new default in > gcc10. > > Docs say w.r.t. -fcommon: "This behavior is inconsistent with C++, and on many targets implies a speed and code size penalty on global variable references. It is mainly useful to enable legacy code to link without errors." > > Want me to send a V3 w/ -fno-common explicitly set? Yes, that sounds like a better place to start a conversation about ... whatever that option is. :) --D > -Eric
diff --git a/db/init.c b/db/init.c index 455220a..0ac3736 100644 --- a/db/init.c +++ b/db/init.c @@ -27,7 +27,6 @@ static int force; static struct xfs_mount xmount; struct xfs_mount *mp; static struct xlog xlog; -libxfs_init_t x; xfs_agnumber_t cur_agno = NULLAGNUMBER; static void diff --git a/logprint/logprint.c b/logprint/logprint.c index 7754a2a..5809af9 100644 --- a/logprint/logprint.c +++ b/logprint/logprint.c @@ -24,7 +24,6 @@ int print_buffer; int print_overwrite; int print_no_data; int print_no_print; -int print_exit = 1; /* -e is now default. specify -c to override */ static int print_operation = OP_PRINT; static void @@ -132,6 +131,7 @@ main(int argc, char **argv) bindtextdomain(PACKAGE, LOCALEDIR); textdomain(PACKAGE); memset(&mount, 0, sizeof(mount)); + print_exit = 1; /* -e is now default. specify -c to override */ progname = basename(argv[0]); while ((c = getopt(argc, argv, "bC:cdefl:iqnors:tDVv")) != EOF) { @@ -152,7 +152,7 @@ main(int argc, char **argv) case 'e': /* -e is now default */ - print_exit++; + print_exit = 1; break; case 'C': print_operation = OP_COPY; diff --git a/mdrestore/xfs_mdrestore.c b/mdrestore/xfs_mdrestore.c index 3375e08..1cd399d 100644 --- a/mdrestore/xfs_mdrestore.c +++ b/mdrestore/xfs_mdrestore.c @@ -7,7 +7,6 @@ #include "libxfs.h" #include "xfs_metadump.h" -char *progname; static int show_progress = 0; static int show_info = 0; static int progress_since_warning = 0;
In each of these cases, db, logprint, and mdrestore are redeclaring as a global variable something which was already provided by a library they link with. gcc now defaults to -fno-common and trips over these global variables which are declared in utilities as well as in libxfs and libxlog, and it causes the build to fail. Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- V2: unmangle whitespace, I'm a n00b.