| Message ID | e1259a8b-b5aa-cfa2-aeab-13d63757d7a5@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfsprogs: don't include all xfs headers just for crc32 | expand |
On Wed, Oct 10, 2018 at 03:55:32PM -0500, Eric Sandeen wrote: > Brian Norris reported that "The $BUILD_CC toolchain might have an > older set of Linux headers than the $CC toolchain. It's generally > unsafe to try to build both with the same definitions, but in > particular, this one can cause compilation failures in the local > crc32selftest build: [failure to find fsmap.h]" > > It seems like the most straightforward thing to do here is include > a specific set of system headers, instead of pulling in the whole > xfs.h header chain which has multiple tests and definitions in > place for headers that may or may not be there during the build. > > Reported-by: Brian Norris <briannorris@chromium.org> > Signed-off-by: Eric Sandeen <sandeen@redhat.com> > --- > > Brian, here's try #3, does this also work for you? Tested-by: Brian Norris <briannorris@chromium.org> This still leaves $BUILD_CFLAGS with bad entries, but as long as they don't get used anywhere that matters, it'll be OK. Brian > diff --git a/libfrog/crc32.c b/libfrog/crc32.c > index 1d52f68..220b33b 100644 > --- a/libfrog/crc32.c > +++ b/libfrog/crc32.c > @@ -29,8 +29,11 @@ > * match the hardware acceleration available on Intel CPUs. > */ > > +#include <inttypes.h> > +#include <asm/types.h> > +#include <sys/time.h> > #include "platform_defs.h" > -#include "xfs.h" > +/* For endian conversion routines */ > #include "xfs_arch.h" > #include "crc32defs.h" > >
On 10/11/18 2:59 PM, Brian Norris wrote: > On Wed, Oct 10, 2018 at 03:55:32PM -0500, Eric Sandeen wrote: >> Brian Norris reported that "The $BUILD_CC toolchain might have an >> older set of Linux headers than the $CC toolchain. It's generally >> unsafe to try to build both with the same definitions, but in >> particular, this one can cause compilation failures in the local >> crc32selftest build: [failure to find fsmap.h]" >> >> It seems like the most straightforward thing to do here is include >> a specific set of system headers, instead of pulling in the whole >> xfs.h header chain which has multiple tests and definitions in >> place for headers that may or may not be there during the build. >> >> Reported-by: Brian Norris <briannorris@chromium.org> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> >> --- >> >> Brian, here's try #3, does this also work for you? > > Tested-by: Brian Norris <briannorris@chromium.org> > > This still leaves $BUILD_CFLAGS with bad entries, but as long as they > don't get used anywhere that matters, it'll be OK. Thanks. At this point I think you have a better grasp of what all the $FOO_FLAGS do than I do ;) It may be to keep things sorted and separate, but it also seemed useful to get a giant tangle of xfs header out of a non-xfs library file. I'm also on the fence about whether cross-compiling the self-check really even gains us much, given that it may be built or optimized completely differently from the code on the target arch ... -Eric
On Thu, Oct 11, 2018 at 05:36:36PM -0500, Eric Sandeen wrote: > > > On 10/11/18 2:59 PM, Brian Norris wrote: > > On Wed, Oct 10, 2018 at 03:55:32PM -0500, Eric Sandeen wrote: > >> Brian Norris reported that "The $BUILD_CC toolchain might have an > >> older set of Linux headers than the $CC toolchain. It's generally > >> unsafe to try to build both with the same definitions, but in > >> particular, this one can cause compilation failures in the local > >> crc32selftest build: [failure to find fsmap.h]" > >> > >> It seems like the most straightforward thing to do here is include > >> a specific set of system headers, instead of pulling in the whole > >> xfs.h header chain which has multiple tests and definitions in > >> place for headers that may or may not be there during the build. > >> > >> Reported-by: Brian Norris <briannorris@chromium.org> > >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> > >> --- > >> > >> Brian, here's try #3, does this also work for you? > > > > Tested-by: Brian Norris <briannorris@chromium.org> > > > > This still leaves $BUILD_CFLAGS with bad entries, but as long as they > > don't get used anywhere that matters, it'll be OK. > > Thanks. At this point I think you have a better grasp of what all the > $FOO_FLAGS do than I do ;) It may be to keep things sorted > and separate, but it also seemed useful to get a giant tangle of > xfs header out of a non-xfs library file. > > I'm also on the fence about whether cross-compiling the self-check > really even gains us much, given that it may be built or optimized > completely differently from the code on the target arch ... So long as it's a software algorithm with a big lookup table I guess it's a serviceable sanity check that nobody totally goobered up the source code, but I don't know if it adds much value in the cross compilation case either. If you ever wanted to add a faster implementation in the built libxfs <cough> that would be more of an issue. (Granted, I've been running a patched xfsprogs with hw accelerated crc32c for a year now and haven't noticed any difference in runtime. Maybe now that I've finished upgrading everything to flash...) --D > -Eric
On Thu, Oct 11, 2018 at 3:36 PM Eric Sandeen <sandeen@sandeen.net> wrote: > Thanks. At this point I think you have a better grasp of what all the > $FOO_FLAGS do than I do ;) It may be to keep things sorted > and separate, but it also seemed useful to get a giant tangle of > xfs header out of a non-xfs library file. Yep! I'm happy with this patch. If I'm bothered enough, I can still send another patch to sort out the flags further. > I'm also on the fence about whether cross-compiling the self-check > really even gains us much, given that it may be built or optimized > completely differently from the code on the target arch ... I don't have a whole lot of opinion here. Regards, Brian
On Thu, Oct 11, 2018 at 03:45:36PM -0700, Darrick J. Wong wrote: > On Thu, Oct 11, 2018 at 05:36:36PM -0500, Eric Sandeen wrote: > > > > > > On 10/11/18 2:59 PM, Brian Norris wrote: > > > On Wed, Oct 10, 2018 at 03:55:32PM -0500, Eric Sandeen wrote: > > >> Brian Norris reported that "The $BUILD_CC toolchain might have an > > >> older set of Linux headers than the $CC toolchain. It's generally > > >> unsafe to try to build both with the same definitions, but in > > >> particular, this one can cause compilation failures in the local > > >> crc32selftest build: [failure to find fsmap.h]" > > >> > > >> It seems like the most straightforward thing to do here is include > > >> a specific set of system headers, instead of pulling in the whole > > >> xfs.h header chain which has multiple tests and definitions in > > >> place for headers that may or may not be there during the build. > > >> > > >> Reported-by: Brian Norris <briannorris@chromium.org> > > >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> > > >> --- > > >> > > >> Brian, here's try #3, does this also work for you? > > > > > > Tested-by: Brian Norris <briannorris@chromium.org> > > > > > > This still leaves $BUILD_CFLAGS with bad entries, but as long as they > > > don't get used anywhere that matters, it'll be OK. > > > > Thanks. At this point I think you have a better grasp of what all the > > $FOO_FLAGS do than I do ;) It may be to keep things sorted > > and separate, but it also seemed useful to get a giant tangle of > > xfs header out of a non-xfs library file. > > > > I'm also on the fence about whether cross-compiling the self-check > > really even gains us much, given that it may be built or optimized > > completely differently from the code on the target arch ... > > So long as it's a software algorithm with a big lookup table I guess > it's a serviceable sanity check that nobody totally goobered up the > source code, but I don't know if it adds much value in the cross > compilation case either. If you ever wanted to add a faster > implementation in the built libxfs <cough> that would be more of an > issue. > > (Granted, I've been running a patched xfsprogs with hw accelerated > crc32c for a year now and haven't noticed any difference in runtime. > Maybe now that I've finished upgrading everything to flash...) > Looks ok, I guess... (...and by "I guess" what I really mean is that given a lot of people cross-compiling things these days I think I want a similar test of the libxfs builtin crc32c algorithm that we can call from xfstests... but that's not what's under review here. :)) Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> --D > --D > > > -Eric
diff --git a/libfrog/crc32.c b/libfrog/crc32.c index 1d52f68..220b33b 100644 --- a/libfrog/crc32.c +++ b/libfrog/crc32.c @@ -29,8 +29,11 @@ * match the hardware acceleration available on Intel CPUs. */ +#include <inttypes.h> +#include <asm/types.h> +#include <sys/time.h> #include "platform_defs.h" -#include "xfs.h" +/* For endian conversion routines */ #include "xfs_arch.h" #include "crc32defs.h"
Brian Norris reported that "The $BUILD_CC toolchain might have an older set of Linux headers than the $CC toolchain. It's generally unsafe to try to build both with the same definitions, but in particular, this one can cause compilation failures in the local crc32selftest build: [failure to find fsmap.h]" It seems like the most straightforward thing to do here is include a specific set of system headers, instead of pulling in the whole xfs.h header chain which has multiple tests and definitions in place for headers that may or may not be there during the build. Reported-by: Brian Norris <briannorris@chromium.org> Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- Brian, here's try #3, does this also work for you?