| Message ID | 20220302142711.38953-5-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | livepatch: fixes for patch generation | expand |
On 02.03.22 15:27, Roger Pau Monne wrote: > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe. > > > > Do not attempt to modify the build system if CFLAGS are not set in > Rules.mk, and instead rely on CONFIG_LIVEPATCH already setting > -f{function,data}-sections. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > This depends on getting the patch to add -f{function,data}-sections > when using CONFIG_LIVEPATCH accepted. > --- > livepatch-build | 22 ++++++++++++++-------- > 1 file changed, 14 insertions(+), 8 deletions(-) > > diff --git a/livepatch-build b/livepatch-build > index 38a92be..656cdac 100755 > --- a/livepatch-build > +++ b/livepatch-build > @@ -98,14 +98,20 @@ function build_special() > > # Build with special GCC flags > cd "${SRCDIR}/xen" || die > - sed -i 's/CFLAGS += -nostdinc/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/' Rules.mk > - cp -p arch/x86/Makefile arch/x86/Makefile.bak > - sed -i 's/--section-alignment=0x200000/--section-alignment=0x1000/' arch/x86/Makefile > - # Restore timestamps to prevent spurious rebuilding > - touch --reference=arch/x86/Makefile.bak arch/x86/Makefile > - make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die > - sed -i 's/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/CFLAGS += -nostdinc/' Rules.mk > - mv -f arch/x86/Makefile.bak arch/x86/Makefile > + if grep -q 'nostdinc' Rules.mk; then This means we are still breaking livepatch builds for all Xen versions between your recent patch and the original patch that moved CFLAGS to xen/Makefile (commit 2740d96efdd3009f8adb40aacdbcf05cfe8d1bbb, Fri Apr 24 14:57:10 2020 +0200). Is this acceptable? (I mean, no one seems to have noticed...) > + # Support for old build system, attempt to set -f{function,data}-sections and rebuild > + sed -i 's/CFLAGS += -nostdinc/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/' Rules.mk > + cp -p arch/x86/Makefile arch/x86/Makefile.bak > + sed -i 's/--section-alignment=0x200000/--section-alignment=0x1000/' arch/x86/Makefile > + # Restore timestamps to prevent spurious rebuilding > + touch --reference=arch/x86/Makefile.bak arch/x86/Makefile > + make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die > + sed -i 's/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/CFLAGS += -nostdinc/' Rules.mk > + mv -f arch/x86/Makefile.bak arch/x86/Makefile > + else > + # -f{function,data}-sections set by CONFIG_LIVEPATCH > + make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die > + fi > > unset LIVEPATCH_BUILD_DIR > unset LIVEPATCH_CAPTURE_DIR Bjoern Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
On Wed, Mar 02, 2022 at 04:41:36PM +0100, Doebel, Bjoern wrote: > On 02.03.22 15:27, Roger Pau Monne wrote: > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe. > > > > > > > > Do not attempt to modify the build system if CFLAGS are not set in > > Rules.mk, and instead rely on CONFIG_LIVEPATCH already setting > > -f{function,data}-sections. > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > --- > > This depends on getting the patch to add -f{function,data}-sections > > when using CONFIG_LIVEPATCH accepted. > > --- > > livepatch-build | 22 ++++++++++++++-------- > > 1 file changed, 14 insertions(+), 8 deletions(-) > > > > diff --git a/livepatch-build b/livepatch-build > > index 38a92be..656cdac 100755 > > --- a/livepatch-build > > +++ b/livepatch-build > > @@ -98,14 +98,20 @@ function build_special() > > > > # Build with special GCC flags > > cd "${SRCDIR}/xen" || die > > - sed -i 's/CFLAGS += -nostdinc/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/' Rules.mk > > - cp -p arch/x86/Makefile arch/x86/Makefile.bak > > - sed -i 's/--section-alignment=0x200000/--section-alignment=0x1000/' arch/x86/Makefile > > - # Restore timestamps to prevent spurious rebuilding > > - touch --reference=arch/x86/Makefile.bak arch/x86/Makefile > > - make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die > > - sed -i 's/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/CFLAGS += -nostdinc/' Rules.mk > > - mv -f arch/x86/Makefile.bak arch/x86/Makefile > > + if grep -q 'nostdinc' Rules.mk; then > > This means we are still breaking livepatch builds for all Xen versions > between your recent patch and the original patch that moved CFLAGS to > xen/Makefile (commit 2740d96efdd3009f8adb40aacdbcf05cfe8d1bbb, Fri Apr 24 > 14:57:10 2020 +0200). > > Is this acceptable? (I mean, no one seems to have noticed...) I've also wondered. I think a reasonable approach would be for interested parties (ie: using livepatch) to backport the commit that sets -f{function,data}-sections in their Xen build for that broken window. I think we could also consider it as a candidate for backport to stable branches, but that won't fix versions we have already shipped. Thanks, Roger.
On 02/03/2022 15:41, Doebel, Bjoern wrote: > On 02.03.22 15:27, Roger Pau Monne wrote: >> CAUTION: This email originated from outside of the organization. Do >> not click links or open attachments unless you can confirm the sender >> and know the content is safe. >> >> >> >> Do not attempt to modify the build system if CFLAGS are not set in >> Rules.mk, and instead rely on CONFIG_LIVEPATCH already setting >> -f{function,data}-sections. >> >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >> --- >> This depends on getting the patch to add -f{function,data}-sections >> when using CONFIG_LIVEPATCH accepted. >> --- >> livepatch-build | 22 ++++++++++++++-------- >> 1 file changed, 14 insertions(+), 8 deletions(-) >> >> diff --git a/livepatch-build b/livepatch-build >> index 38a92be..656cdac 100755 >> --- a/livepatch-build >> +++ b/livepatch-build >> @@ -98,14 +98,20 @@ function build_special() >> >> # Build with special GCC flags >> cd "${SRCDIR}/xen" || die >> - sed -i 's/CFLAGS += -nostdinc/CFLAGS += -nostdinc >> -ffunction-sections -fdata-sections/' Rules.mk >> - cp -p arch/x86/Makefile arch/x86/Makefile.bak >> - sed -i >> 's/--section-alignment=0x200000/--section-alignment=0x1000/' >> arch/x86/Makefile >> - # Restore timestamps to prevent spurious rebuilding >> - touch --reference=arch/x86/Makefile.bak arch/x86/Makefile >> - make "-j$CPUS" $XEN_DEBUG &> >> "${OUTPUT}/build_${name}_compile.log" || die >> - sed -i 's/CFLAGS += -nostdinc -ffunction-sections >> -fdata-sections/CFLAGS += -nostdinc/' Rules.mk >> - mv -f arch/x86/Makefile.bak arch/x86/Makefile >> + if grep -q 'nostdinc' Rules.mk; then > > This means we are still breaking livepatch builds for all Xen versions > between your recent patch and the original patch that moved CFLAGS to > xen/Makefile (commit 2740d96efdd3009f8adb40aacdbcf05cfe8d1bbb, Fri Apr > 24 14:57:10 2020 +0200). > > Is this acceptable? (I mean, no one seems to have noticed...) So yeah - not putting this into OSSTest has had exactly the forseen outcome. The steps now are to unbreak it by whatever means are necessary, then get CI in place so we can't regress it again. The tip of livepatch-build-tools should work with all versions of Xen. If this means we need to backport fixes into the staging-* branches, then so be it. The downstream users of livepatching have patchqueues anyway, and these breakages are not the kind of thing which could slip though even a token testing attempt. As far as this issue goes, I suggest that we do what we can to avoid livepatch-build-tools needing to do surgery like that on the build system, then backport it everywhere. The above is the kind of fragility which really shouldn't exist :-/ ~Andrew
On 02.03.22 15:27, Roger Pau Monne wrote: > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe. > > > > Do not attempt to modify the build system if CFLAGS are not set in > Rules.mk, and instead rely on CONFIG_LIVEPATCH already setting > -f{function,data}-sections. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > This depends on getting the patch to add -f{function,data}-sections > when using CONFIG_LIVEPATCH accepted. > --- > livepatch-build | 22 ++++++++++++++-------- > 1 file changed, 14 insertions(+), 8 deletions(-) > > diff --git a/livepatch-build b/livepatch-build > index 38a92be..656cdac 100755 > --- a/livepatch-build > +++ b/livepatch-build > @@ -98,14 +98,20 @@ function build_special() > > # Build with special GCC flags > cd "${SRCDIR}/xen" || die > - sed -i 's/CFLAGS += -nostdinc/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/' Rules.mk > - cp -p arch/x86/Makefile arch/x86/Makefile.bak > - sed -i 's/--section-alignment=0x200000/--section-alignment=0x1000/' arch/x86/Makefile > - # Restore timestamps to prevent spurious rebuilding > - touch --reference=arch/x86/Makefile.bak arch/x86/Makefile > - make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die > - sed -i 's/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/CFLAGS += -nostdinc/' Rules.mk > - mv -f arch/x86/Makefile.bak arch/x86/Makefile > + if grep -q 'nostdinc' Rules.mk; then > + # Support for old build system, attempt to set -f{function,data}-sections and rebuild > + sed -i 's/CFLAGS += -nostdinc/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/' Rules.mk > + cp -p arch/x86/Makefile arch/x86/Makefile.bak > + sed -i 's/--section-alignment=0x200000/--section-alignment=0x1000/' arch/x86/Makefile > + # Restore timestamps to prevent spurious rebuilding > + touch --reference=arch/x86/Makefile.bak arch/x86/Makefile > + make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die > + sed -i 's/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/CFLAGS += -nostdinc/' Rules.mk > + mv -f arch/x86/Makefile.bak arch/x86/Makefile > + else > + # -f{function,data}-sections set by CONFIG_LIVEPATCH > + make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die > + fi > > unset LIVEPATCH_BUILD_DIR > unset LIVEPATCH_CAPTURE_DIR Reviewed-by: Bjoern Doebel <doebel@amazon.de> Confirming that I can build a livepatch against upstream master with your series and the other CONFIG_LIVEPATCH patch. Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
> From: Roger Pau Monne <roger.pau@citrix.com> > Sent: Wednesday, March 2, 2022 2:27 PM > To: xen-devel@lists.xenproject.org <xen-devel@lists.xenproject.org> > Cc: Ross Lagerwall <ross.lagerwall@citrix.com>; konrad.wilk@oracle.com <konrad.wilk@oracle.com>; doebel@amazon.de <doebel@amazon.de>; julien@xen.org <julien@xen.org>; Andrew Cooper <Andrew.Cooper3@citrix.com>; Roger Pau Monne <roger.pau@citrix.com> > Subject: [PATCH 4/4] livepatch: differentiate between old and new build systems > > Do not attempt to modify the build system if CFLAGS are not set in > Rules.mk, and instead rely on CONFIG_LIVEPATCH already setting > -f{function,data}-sections. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > This depends on getting the patch to add -f{function,data}-sections > when using CONFIG_LIVEPATCH accepted. > --- > livepatch-build | 22 ++++++++++++++-------- > 1 file changed, 14 insertions(+), 8 deletions(-) > > diff --git a/livepatch-build b/livepatch-build > index 38a92be..656cdac 100755 > --- a/livepatch-build > +++ b/livepatch-build > @@ -98,14 +98,20 @@ function build_special() > > # Build with special GCC flags > cd "${SRCDIR}/xen" || die > - sed -i 's/CFLAGS += -nostdinc/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/' Rules.mk > - cp -p arch/x86/Makefile arch/x86/Makefile.bak > - sed -i 's/--section-alignment=0x200000/--section-alignment=0x1000/' arch/x86/Makefile > - # Restore timestamps to prevent spurious rebuilding > - touch --reference=arch/x86/Makefile.bak arch/x86/Makefile > - make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die > - sed -i 's/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/CFLAGS += -nostdinc/' Rules.mk > - mv -f arch/x86/Makefile.bak arch/x86/Makefile > + if grep -q 'nostdinc' Rules.mk; then > + # Support for old build system, attempt to set -f{function,data}-sections and rebuild > + sed -i 's/CFLAGS += -nostdinc/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/' Rules.mk > + cp -p arch/x86/Makefile arch/x86/Makefile.bak > + sed -i 's/--section-alignment=0x200000/--section-alignment=0x1000/' arch/x86/Makefile > + # Restore timestamps to prevent spurious rebuilding > + touch --reference=arch/x86/Makefile.bak arch/x86/Makefile > + make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die > + sed -i 's/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/CFLAGS += -nostdinc/' Rules.mk > + mv -f arch/x86/Makefile.bak arch/x86/Makefile > + else > + # -f{function,data}-sections set by CONFIG_LIVEPATCH > + make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die > + fi > > unset LIVEPATCH_BUILD_DIR > unset LIVEPATCH_CAPTURE_DIR > -- > 2.34.1 Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>
On 02/03/2022 14:27, Roger Pau Monne wrote: > diff --git a/livepatch-build b/livepatch-build > index 38a92be..656cdac 100755 > --- a/livepatch-build > +++ b/livepatch-build > @@ -98,14 +98,20 @@ function build_special() > > # Build with special GCC flags > cd "${SRCDIR}/xen" || die > - sed -i 's/CFLAGS += -nostdinc/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/' Rules.mk > - cp -p arch/x86/Makefile arch/x86/Makefile.bak > - sed -i 's/--section-alignment=0x200000/--section-alignment=0x1000/' arch/x86/Makefile > - # Restore timestamps to prevent spurious rebuilding > - touch --reference=arch/x86/Makefile.bak arch/x86/Makefile > - make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die > - sed -i 's/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/CFLAGS += -nostdinc/' Rules.mk > - mv -f arch/x86/Makefile.bak arch/x86/Makefile > + if grep -q 'nostdinc' Rules.mk; then > + # Support for old build system, attempt to set -f{function,data}-sections and rebuild > + sed -i 's/CFLAGS += -nostdinc/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/' Rules.mk > + cp -p arch/x86/Makefile arch/x86/Makefile.bak > + sed -i 's/--section-alignment=0x200000/--section-alignment=0x1000/' arch/x86/Makefile > + # Restore timestamps to prevent spurious rebuilding > + touch --reference=arch/x86/Makefile.bak arch/x86/Makefile > + make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die > + sed -i 's/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/CFLAGS += -nostdinc/' Rules.mk > + mv -f arch/x86/Makefile.bak arch/x86/Makefile > + else > + # -f{function,data}-sections set by CONFIG_LIVEPATCH > + make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die > + fi This really ought to be the other way around, by spotting the thing we know is good, and then falling back to the heuristics. In light of the updates to the Xen side, something like: if grep -q CC_SPLIT_SECTIONS Kconfig; then # -f{function,data}-sections set by CONFIG_LIVEPATCH make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die else # Support for old build system, attempt to set -f{function,data}-sections and rebuild ... fi ? ~Andrew
On Tue, Mar 08, 2022 at 02:38:47PM +0000, Andrew Cooper wrote: > On 02/03/2022 14:27, Roger Pau Monne wrote: > > diff --git a/livepatch-build b/livepatch-build > > index 38a92be..656cdac 100755 > > --- a/livepatch-build > > +++ b/livepatch-build > > @@ -98,14 +98,20 @@ function build_special() > > > > # Build with special GCC flags > > cd "${SRCDIR}/xen" || die > > - sed -i 's/CFLAGS += -nostdinc/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/' Rules.mk > > - cp -p arch/x86/Makefile arch/x86/Makefile.bak > > - sed -i 's/--section-alignment=0x200000/--section-alignment=0x1000/' arch/x86/Makefile > > - # Restore timestamps to prevent spurious rebuilding > > - touch --reference=arch/x86/Makefile.bak arch/x86/Makefile > > - make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die > > - sed -i 's/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/CFLAGS += -nostdinc/' Rules.mk > > - mv -f arch/x86/Makefile.bak arch/x86/Makefile > > + if grep -q 'nostdinc' Rules.mk; then > > + # Support for old build system, attempt to set -f{function,data}-sections and rebuild > > + sed -i 's/CFLAGS += -nostdinc/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/' Rules.mk > > + cp -p arch/x86/Makefile arch/x86/Makefile.bak > > + sed -i 's/--section-alignment=0x200000/--section-alignment=0x1000/' arch/x86/Makefile > > + # Restore timestamps to prevent spurious rebuilding > > + touch --reference=arch/x86/Makefile.bak arch/x86/Makefile > > + make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die > > + sed -i 's/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/CFLAGS += -nostdinc/' Rules.mk > > + mv -f arch/x86/Makefile.bak arch/x86/Makefile > > + else > > + # -f{function,data}-sections set by CONFIG_LIVEPATCH > > + make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die > > + fi > > This really ought to be the other way around, by spotting the thing we > know is good, and then falling back to the heuristics. In light of the > updates to the Xen side, something like: I'm not sure I agree. I do prefer to spot the 'bad' one, and just fallback to expecting Xen to correctly set -f{function,data}-sections otherwise. > if grep -q CC_SPLIT_SECTIONS Kconfig; then Because this logic ties us to not moving CC_SPLIT_SECTIONS from being defined in xen/Kconfig (or even changing it's name), and gain ties the livepatch tools to internal details about the Xen build system. Thanks, Roger.
On 08/03/2022 14:52, Roger Pau Monne wrote: > On Tue, Mar 08, 2022 at 02:38:47PM +0000, Andrew Cooper wrote: >> On 02/03/2022 14:27, Roger Pau Monne wrote: >>> diff --git a/livepatch-build b/livepatch-build >>> index 38a92be..656cdac 100755 >>> --- a/livepatch-build >>> +++ b/livepatch-build >>> @@ -98,14 +98,20 @@ function build_special() >>> >>> # Build with special GCC flags >>> cd "${SRCDIR}/xen" || die >>> - sed -i 's/CFLAGS += -nostdinc/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/' Rules.mk >>> - cp -p arch/x86/Makefile arch/x86/Makefile.bak >>> - sed -i 's/--section-alignment=0x200000/--section-alignment=0x1000/' arch/x86/Makefile >>> - # Restore timestamps to prevent spurious rebuilding >>> - touch --reference=arch/x86/Makefile.bak arch/x86/Makefile >>> - make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die >>> - sed -i 's/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/CFLAGS += -nostdinc/' Rules.mk >>> - mv -f arch/x86/Makefile.bak arch/x86/Makefile >>> + if grep -q 'nostdinc' Rules.mk; then >>> + # Support for old build system, attempt to set -f{function,data}-sections and rebuild >>> + sed -i 's/CFLAGS += -nostdinc/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/' Rules.mk >>> + cp -p arch/x86/Makefile arch/x86/Makefile.bak >>> + sed -i 's/--section-alignment=0x200000/--section-alignment=0x1000/' arch/x86/Makefile >>> + # Restore timestamps to prevent spurious rebuilding >>> + touch --reference=arch/x86/Makefile.bak arch/x86/Makefile >>> + make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die >>> + sed -i 's/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/CFLAGS += -nostdinc/' Rules.mk >>> + mv -f arch/x86/Makefile.bak arch/x86/Makefile >>> + else >>> + # -f{function,data}-sections set by CONFIG_LIVEPATCH >>> + make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die >>> + fi >> This really ought to be the other way around, by spotting the thing we >> know is good, and then falling back to the heuristics. In light of the >> updates to the Xen side, something like: > I'm not sure I agree. I do prefer to spot the 'bad' one, and just > fallback to expecting Xen to correctly set -f{function,data}-sections > otherwise. > >> if grep -q CC_SPLIT_SECTIONS Kconfig; then > Because this logic ties us to not moving CC_SPLIT_SECTIONS from being > defined in xen/Kconfig (or even changing it's name), and gain ties the > livepatch tools to internal details about the Xen build system. It doesn't particularly matter which way around the if/else is. It does matter that we're choosing based on something relevant. nostdinc in Rules.mk has exactly the same amount of "magic string in magic file" as CC_SPLIT_SECTIONS in Kconfig, but has absolutely nothing to do with the property we actually care about. Really what you actually want is if grep -q CC_SPLIT_SECTIONS Kconfig; then # Xen behaves sensibly elif grep -q 'nostdinc' Rules.mk; then # Legacy mess with Rules.mk else die "Help with build system divination" fi The "behaves sensibly" case is unlikely to change name and unlikely to move locations, but each are easy to cope with via `grep -e FOO -e BAR file1 file2`, and this approach avoids the problem of blindly (and falsely) assuming that anything which is 4.14 and later splits sections correctly, and that this will remain true even when someone adds "# use to have -nostdinc here" to Rules.mk. ~Andrew
On Thu, Mar 10, 2022 at 06:01:48PM +0000, Andrew Cooper wrote: > On 08/03/2022 14:52, Roger Pau Monne wrote: > > On Tue, Mar 08, 2022 at 02:38:47PM +0000, Andrew Cooper wrote: > >> On 02/03/2022 14:27, Roger Pau Monne wrote: > >>> diff --git a/livepatch-build b/livepatch-build > >>> index 38a92be..656cdac 100755 > >>> --- a/livepatch-build > >>> +++ b/livepatch-build > >>> @@ -98,14 +98,20 @@ function build_special() > >>> > >>> # Build with special GCC flags > >>> cd "${SRCDIR}/xen" || die > >>> - sed -i 's/CFLAGS += -nostdinc/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/' Rules.mk > >>> - cp -p arch/x86/Makefile arch/x86/Makefile.bak > >>> - sed -i 's/--section-alignment=0x200000/--section-alignment=0x1000/' arch/x86/Makefile > >>> - # Restore timestamps to prevent spurious rebuilding > >>> - touch --reference=arch/x86/Makefile.bak arch/x86/Makefile > >>> - make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die > >>> - sed -i 's/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/CFLAGS += -nostdinc/' Rules.mk > >>> - mv -f arch/x86/Makefile.bak arch/x86/Makefile > >>> + if grep -q 'nostdinc' Rules.mk; then > >>> + # Support for old build system, attempt to set -f{function,data}-sections and rebuild > >>> + sed -i 's/CFLAGS += -nostdinc/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/' Rules.mk > >>> + cp -p arch/x86/Makefile arch/x86/Makefile.bak > >>> + sed -i 's/--section-alignment=0x200000/--section-alignment=0x1000/' arch/x86/Makefile > >>> + # Restore timestamps to prevent spurious rebuilding > >>> + touch --reference=arch/x86/Makefile.bak arch/x86/Makefile > >>> + make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die > >>> + sed -i 's/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/CFLAGS += -nostdinc/' Rules.mk > >>> + mv -f arch/x86/Makefile.bak arch/x86/Makefile > >>> + else > >>> + # -f{function,data}-sections set by CONFIG_LIVEPATCH > >>> + make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die > >>> + fi > >> This really ought to be the other way around, by spotting the thing we > >> know is good, and then falling back to the heuristics. In light of the > >> updates to the Xen side, something like: > > I'm not sure I agree. I do prefer to spot the 'bad' one, and just > > fallback to expecting Xen to correctly set -f{function,data}-sections > > otherwise. > > > >> if grep -q CC_SPLIT_SECTIONS Kconfig; then > > Because this logic ties us to not moving CC_SPLIT_SECTIONS from being > > defined in xen/Kconfig (or even changing it's name), and gain ties the > > livepatch tools to internal details about the Xen build system. > > It doesn't particularly matter which way around the if/else is. It does > matter that we're choosing based on something relevant. > > nostdinc in Rules.mk has exactly the same amount of "magic string in > magic file" as CC_SPLIT_SECTIONS in Kconfig, but has absolutely nothing > to do with the property we actually care about. > > Really what you actually want is > > if grep -q CC_SPLIT_SECTIONS Kconfig; then > # Xen behaves sensibly > elif grep -q 'nostdinc' Rules.mk; then > # Legacy mess with Rules.mk > else > die "Help with build system divination" > fi > > The "behaves sensibly" case is unlikely to change name and unlikely to > move locations, but each are easy to cope with via `grep -e FOO -e BAR > file1 file2`, and this approach avoids the problem of blindly (and > falsely) assuming that anything which is 4.14 and later splits sections > correctly, and that this will remain true even when someone adds "# use > to have -nostdinc here" to Rules.mk. TBH, I don't find the proposed solution is much better to what's in this patch, and as said I really dislike tying the behavior of the livepatch build tools to heuristics against Xen internal build files - be it a Kconfig or a Makefile. Specially because your proposed approach adds heuristics to detect the 'good' case which should be the default one going forward. A better option might be to just make the 'build adjustments' a command line option that the user can pass to the tools, ie: --build-adjust and let the user decide whether it needs the adjustments or not. If I was a livepatch user myself I would seriously consider picking the linker script changes and backport that to my production version. Thanks, Roger.
On Fri, Mar 11, 2022 at 09:33:15AM +0100, Roger Pau Monné wrote: > On Thu, Mar 10, 2022 at 06:01:48PM +0000, Andrew Cooper wrote: > > On 08/03/2022 14:52, Roger Pau Monne wrote: > > > On Tue, Mar 08, 2022 at 02:38:47PM +0000, Andrew Cooper wrote: > > >> On 02/03/2022 14:27, Roger Pau Monne wrote: > > >>> diff --git a/livepatch-build b/livepatch-build > > >>> index 38a92be..656cdac 100755 > > >>> --- a/livepatch-build > > >>> +++ b/livepatch-build > > >>> @@ -98,14 +98,20 @@ function build_special() > > >>> > > >>> # Build with special GCC flags > > >>> cd "${SRCDIR}/xen" || die > > >>> - sed -i 's/CFLAGS += -nostdinc/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/' Rules.mk > > >>> - cp -p arch/x86/Makefile arch/x86/Makefile.bak > > >>> - sed -i 's/--section-alignment=0x200000/--section-alignment=0x1000/' arch/x86/Makefile > > >>> - # Restore timestamps to prevent spurious rebuilding > > >>> - touch --reference=arch/x86/Makefile.bak arch/x86/Makefile > > >>> - make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die > > >>> - sed -i 's/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/CFLAGS += -nostdinc/' Rules.mk > > >>> - mv -f arch/x86/Makefile.bak arch/x86/Makefile > > >>> + if grep -q 'nostdinc' Rules.mk; then > > >>> + # Support for old build system, attempt to set -f{function,data}-sections and rebuild > > >>> + sed -i 's/CFLAGS += -nostdinc/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/' Rules.mk > > >>> + cp -p arch/x86/Makefile arch/x86/Makefile.bak > > >>> + sed -i 's/--section-alignment=0x200000/--section-alignment=0x1000/' arch/x86/Makefile > > >>> + # Restore timestamps to prevent spurious rebuilding > > >>> + touch --reference=arch/x86/Makefile.bak arch/x86/Makefile > > >>> + make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die > > >>> + sed -i 's/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/CFLAGS += -nostdinc/' Rules.mk > > >>> + mv -f arch/x86/Makefile.bak arch/x86/Makefile > > >>> + else > > >>> + # -f{function,data}-sections set by CONFIG_LIVEPATCH > > >>> + make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die > > >>> + fi > > >> This really ought to be the other way around, by spotting the thing we > > >> know is good, and then falling back to the heuristics. In light of the > > >> updates to the Xen side, something like: > > > I'm not sure I agree. I do prefer to spot the 'bad' one, and just > > > fallback to expecting Xen to correctly set -f{function,data}-sections > > > otherwise. > > > > > >> if grep -q CC_SPLIT_SECTIONS Kconfig; then > > > Because this logic ties us to not moving CC_SPLIT_SECTIONS from being > > > defined in xen/Kconfig (or even changing it's name), and gain ties the > > > livepatch tools to internal details about the Xen build system. > > > > It doesn't particularly matter which way around the if/else is. It does > > matter that we're choosing based on something relevant. > > > > nostdinc in Rules.mk has exactly the same amount of "magic string in > > magic file" as CC_SPLIT_SECTIONS in Kconfig, but has absolutely nothing > > to do with the property we actually care about. > > > > Really what you actually want is > > > > if grep -q CC_SPLIT_SECTIONS Kconfig; then > > # Xen behaves sensibly > > elif grep -q 'nostdinc' Rules.mk; then > > # Legacy mess with Rules.mk > > else > > die "Help with build system divination" > > fi > > > > The "behaves sensibly" case is unlikely to change name and unlikely to > > move locations, but each are easy to cope with via `grep -e FOO -e BAR > > file1 file2`, and this approach avoids the problem of blindly (and > > falsely) assuming that anything which is 4.14 and later splits sections > > correctly, and that this will remain true even when someone adds "# use > > to have -nostdinc here" to Rules.mk. > > TBH, I don't find the proposed solution is much better to what's in > this patch, and as said I really dislike tying the behavior of the > livepatch build tools to heuristics against Xen internal build files - > be it a Kconfig or a Makefile. Specially because your proposed > approach adds heuristics to detect the 'good' case which should be the > default one going forward. > > A better option might be to just make the 'build adjustments' a > command line option that the user can pass to the tools, ie: > --build-adjust and let the user decide whether it needs the > adjustments or not. If I was a livepatch user myself I would seriously > consider picking the linker script changes and backport that to my > production version. Ping? Is the proposed command line option an acceptable way to move this forward? Can I have an opinion from the maintainers? Thanks, Roger.
diff --git a/livepatch-build b/livepatch-build index 38a92be..656cdac 100755 --- a/livepatch-build +++ b/livepatch-build @@ -98,14 +98,20 @@ function build_special() # Build with special GCC flags cd "${SRCDIR}/xen" || die - sed -i 's/CFLAGS += -nostdinc/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/' Rules.mk - cp -p arch/x86/Makefile arch/x86/Makefile.bak - sed -i 's/--section-alignment=0x200000/--section-alignment=0x1000/' arch/x86/Makefile - # Restore timestamps to prevent spurious rebuilding - touch --reference=arch/x86/Makefile.bak arch/x86/Makefile - make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die - sed -i 's/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/CFLAGS += -nostdinc/' Rules.mk - mv -f arch/x86/Makefile.bak arch/x86/Makefile + if grep -q 'nostdinc' Rules.mk; then + # Support for old build system, attempt to set -f{function,data}-sections and rebuild + sed -i 's/CFLAGS += -nostdinc/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/' Rules.mk + cp -p arch/x86/Makefile arch/x86/Makefile.bak + sed -i 's/--section-alignment=0x200000/--section-alignment=0x1000/' arch/x86/Makefile + # Restore timestamps to prevent spurious rebuilding + touch --reference=arch/x86/Makefile.bak arch/x86/Makefile + make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die + sed -i 's/CFLAGS += -nostdinc -ffunction-sections -fdata-sections/CFLAGS += -nostdinc/' Rules.mk + mv -f arch/x86/Makefile.bak arch/x86/Makefile + else + # -f{function,data}-sections set by CONFIG_LIVEPATCH + make "-j$CPUS" $XEN_DEBUG &> "${OUTPUT}/build_${name}_compile.log" || die + fi unset LIVEPATCH_BUILD_DIR unset LIVEPATCH_CAPTURE_DIR
Do not attempt to modify the build system if CFLAGS are not set in Rules.mk, and instead rely on CONFIG_LIVEPATCH already setting -f{function,data}-sections. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- This depends on getting the patch to add -f{function,data}-sections when using CONFIG_LIVEPATCH accepted. --- livepatch-build | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)