Currently version.o is explicitly ignored as the file would change as a result
of the orignal and the patched build having possibly different dates and
times.

Fix such difference by exporting the date and time from the build script, so
that both builds share the same build time.  This allows checking for changes
in version.c, since the rest of fields need to be manually changed in order to
produce different output.

Setting XEN_BUILD_{DATE,TIME} as an environment variable has been supported
since before livepatch support was added to Xen, so it's safe to export those
variables unconditionally.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 livepatch-build | 4 ++++
 livepatch-gcc   | 2 --
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/livepatch-build b/livepatch-build
index e2ccce4f7fd7..f622683fc56c 100755
--- a/livepatch-build
+++ b/livepatch-build
@@ -417,6 +417,10 @@ if [ "${SKIP}" != "build" ]; then
 
     export CROSS_COMPILE="${TOOLSDIR}/livepatch-gcc "
 
+    # Force same date and time to prevent unwanted changes in version.c
+    export XEN_BUILD_DATE=`LC_ALL=C date`
+    export XEN_BUILD_TIME=`LC_ALL=C date +%T`
+
     echo "Perform full initial build with ${CPUS} CPU(s)..."
     build_full
 
diff --git a/livepatch-gcc b/livepatch-gcc
index fcad80551aa0..e4cb6fb59029 100755
--- a/livepatch-gcc
+++ b/livepatch-gcc
@@ -33,7 +33,6 @@ if [[ "$TOOLCHAINCMD" =~ $GCC_RE ]] ; then
             obj=$2
             [[ $2 = */.tmp_*.o ]] && obj=${2/.tmp_/}
             case "$(basename $obj)" in
-            version.o|\
             debug.o|\
             check.o|\
             *.xen-syms.*.o|\
@@ -63,7 +62,6 @@ done
 elif [[ "$TOOLCHAINCMD" =~ $OBJCOPY_RE ]] ; then
     obj="${!#}"
     case "$(basename $obj)" in
-        version.o|\
         debug.o|\
         check.o|\
         boot.o|\

base-commit: e588b7914e7afa3abb64b15a32fc2fdb57ded341
prerequisite-patch-id: 3419543c1c7c96b551db398518dbb10d81b3d5d9
prerequisite-patch-id: 640658ca7a1a21a540bfd6a862ff83669f70a065
prerequisite-patch-id: 650cd4210c2e73e9d2588b048be2c8278ae96acd
-- 
2.43.0

On 05/12/2023 12:34 pm, Roger Pau Monne wrote:
> Currently version.o is explicitly ignored as the file would change as a result
> of the orignal and the patched build having possibly different dates and
> times.
>
> Fix such difference by exporting the date and time from the build script, so
> that both builds share the same build time.  This allows checking for changes
> in version.c, since the rest of fields need to be manually changed in order to
> produce different output.
>
> Setting XEN_BUILD_{DATE,TIME} as an environment variable has been supported
> since before livepatch support was added to Xen, so it's safe to export those
> variables unconditionally.
>
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  livepatch-build | 4 ++++
>  livepatch-gcc   | 2 --
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/livepatch-build b/livepatch-build
> index e2ccce4f7fd7..f622683fc56c 100755
> --- a/livepatch-build
> +++ b/livepatch-build
> @@ -417,6 +417,10 @@ if [ "${SKIP}" != "build" ]; then
>  
>      export CROSS_COMPILE="${TOOLSDIR}/livepatch-gcc "
>  
> +    # Force same date and time to prevent unwanted changes in version.c
> +    export XEN_BUILD_DATE=`LC_ALL=C date`
> +    export XEN_BUILD_TIME=`LC_ALL=C date +%T`

Date is the one that goes wrong every time, but everything else in
compile.h can go wrong in a way that causes version.o to change.

Ideally, the pristine source for building livepatches would include a
generated compile.h, and livepatch would have a way to force no
regeneration of the header.  But I've got no idea how nice that would be
to arrange.

That way, you're using the same details as the Xen being patched, rather
than hoping that two identical different details will cancel out in the
binary diff.

> +
>      echo "Perform full initial build with ${CPUS} CPU(s)..."
>      build_full
>  
> diff --git a/livepatch-gcc b/livepatch-gcc
> index fcad80551aa0..e4cb6fb59029 100755
> --- a/livepatch-gcc
> +++ b/livepatch-gcc
> @@ -33,7 +33,6 @@ if [[ "$TOOLCHAINCMD" =~ $GCC_RE ]] ; then
>              obj=$2
>              [[ $2 = */.tmp_*.o ]] && obj=${2/.tmp_/}
>              case "$(basename $obj)" in
> -            version.o|\
>              debug.o|\
>              check.o|\

Tangential question.  check.o is excluded because it's a toolchain test,
but any idea what debug.o is doing in this list?

It can't possibly be the debug.c I've recently added to x86 (which we'll
want to be able to livepatch), so I guess it's got something to do the
ARM debug.S's, but I can't see anything in those that are worthy of
exemption either...

~Andrew

On Tue, Dec 05, 2023 at 02:15:05PM +0000, Andrew Cooper wrote:
> On 05/12/2023 12:34 pm, Roger Pau Monne wrote:
> > Currently version.o is explicitly ignored as the file would change as a result
> > of the orignal and the patched build having possibly different dates and
> > times.
> >
> > Fix such difference by exporting the date and time from the build script, so
> > that both builds share the same build time.  This allows checking for changes
> > in version.c, since the rest of fields need to be manually changed in order to
> > produce different output.
> >
> > Setting XEN_BUILD_{DATE,TIME} as an environment variable has been supported
> > since before livepatch support was added to Xen, so it's safe to export those
> > variables unconditionally.
> >
> > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> >  livepatch-build | 4 ++++
> >  livepatch-gcc   | 2 --
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/livepatch-build b/livepatch-build
> > index e2ccce4f7fd7..f622683fc56c 100755
> > --- a/livepatch-build
> > +++ b/livepatch-build
> > @@ -417,6 +417,10 @@ if [ "${SKIP}" != "build" ]; then
> >  
> >      export CROSS_COMPILE="${TOOLSDIR}/livepatch-gcc "
> >  
> > +    # Force same date and time to prevent unwanted changes in version.c
> > +    export XEN_BUILD_DATE=`LC_ALL=C date`
> > +    export XEN_BUILD_TIME=`LC_ALL=C date +%T`
> 
> Date is the one that goes wrong every time, but everything else in
> compile.h can go wrong in a way that causes version.o to change.

I've attempted to reflect that in "since the rest of fields need to be
manually changed in order to produce different output".

For those to change there must be some kind of environment change
between the original and the patched version build, and hence I don't
think that would be supported.

> Ideally, the pristine source for building livepatches would include a
> generated compile.h, and livepatch would have a way to force no
> regeneration of the header.  But I've got no idea how nice that would be
> to arrange.

Yes, no idea how fragile that would be either.  IMO the proposed
approach is not that bad.

> That way, you're using the same details as the Xen being patched, rather
> than hoping that two identical different details will cancel out in the
> binary diff.

Another option is to set all the env variables to disable any
build time probing.  However things like compiler or version changing
between the original and the patched builds likely point out to issues
elsewhere, unless it's intentional modification of the helpers.

> > +
> >      echo "Perform full initial build with ${CPUS} CPU(s)..."
> >      build_full
> >  
> > diff --git a/livepatch-gcc b/livepatch-gcc
> > index fcad80551aa0..e4cb6fb59029 100755
> > --- a/livepatch-gcc
> > +++ b/livepatch-gcc
> > @@ -33,7 +33,6 @@ if [[ "$TOOLCHAINCMD" =~ $GCC_RE ]] ; then
> >              obj=$2
> >              [[ $2 = */.tmp_*.o ]] && obj=${2/.tmp_/}
> >              case "$(basename $obj)" in
> > -            version.o|\
> >              debug.o|\
> >              check.o|\
> 
> Tangential question.  check.o is excluded because it's a toolchain test,
> but any idea what debug.o is doing in this list?
> 
> It can't possibly be the debug.c I've recently added to x86 (which we'll
> want to be able to livepatch), so I guess it's got something to do the
> ARM debug.S's, but I can't see anything in those that are worthy of
> exemption either...

Hm, that comes from the first commit that imported the wrapper to the
repository, and at that point only x86 had livepatch support.

I'm tempted to think this was inherited from the original xsplice
tooling, and so debug.o needs to be removed from the list.

Thanks, Roger.

On Tue, Dec 5, 2023 at 2:57 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
>
> On Tue, Dec 05, 2023 at 02:15:05PM +0000, Andrew Cooper wrote:
> > On 05/12/2023 12:34 pm, Roger Pau Monne wrote:
> > > Currently version.o is explicitly ignored as the file would change as a result
> > > of the orignal and the patched build having possibly different dates and
> > > times.
> > >
> > > Fix such difference by exporting the date and time from the build script, so
> > > that both builds share the same build time.  This allows checking for changes
> > > in version.c, since the rest of fields need to be manually changed in order to
> > > produce different output.
> > >
> > > Setting XEN_BUILD_{DATE,TIME} as an environment variable has been supported
> > > since before livepatch support was added to Xen, so it's safe to export those
> > > variables unconditionally.
> > >
> > > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > > ---
> > >  livepatch-build | 4 ++++
> > >  livepatch-gcc   | 2 --
> > >  2 files changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/livepatch-build b/livepatch-build
> > > index e2ccce4f7fd7..f622683fc56c 100755
> > > --- a/livepatch-build
> > > +++ b/livepatch-build
> > > @@ -417,6 +417,10 @@ if [ "${SKIP}" != "build" ]; then
> > >
> > >      export CROSS_COMPILE="${TOOLSDIR}/livepatch-gcc "
> > >
> > > +    # Force same date and time to prevent unwanted changes in version.c
> > > +    export XEN_BUILD_DATE=`LC_ALL=C date`
> > > +    export XEN_BUILD_TIME=`LC_ALL=C date +%T`
> >
> > Date is the one that goes wrong every time, but everything else in
> > compile.h can go wrong in a way that causes version.o to change.
>
> I've attempted to reflect that in "since the rest of fields need to be
> manually changed in order to produce different output".
>
> For those to change there must be some kind of environment change
> between the original and the patched version build, and hence I don't
> think that would be supported.

In general, yes. However, with this patch changes to the
hostname/domain/username would result in version.o being marked
as changed even though it is entirely fine to build the live patch
on a different build host from the original Xen.

>
> > Ideally, the pristine source for building livepatches would include a
> > generated compile.h, and livepatch would have a way to force no
> > regeneration of the header.  But I've got no idea how nice that would be
> > to arrange.
>
> Yes, no idea how fragile that would be either.  IMO the proposed
> approach is not that bad.
>
> > That way, you're using the same details as the Xen being patched, rather
> > than hoping that two identical different details will cancel out in the
> > binary diff.
>
> Another option is to set all the env variables to disable any
> build time probing.  However things like compiler or version changing
> between the original and the patched builds likely point out to issues
> elsewhere, unless it's intentional modification of the helpers.
>
> > > +
> > >      echo "Perform full initial build with ${CPUS} CPU(s)..."
> > >      build_full
> > >
> > > diff --git a/livepatch-gcc b/livepatch-gcc
> > > index fcad80551aa0..e4cb6fb59029 100755
> > > --- a/livepatch-gcc
> > > +++ b/livepatch-gcc
> > > @@ -33,7 +33,6 @@ if [[ "$TOOLCHAINCMD" =~ $GCC_RE ]] ; then
> > >              obj=$2
> > >              [[ $2 = */.tmp_*.o ]] && obj=${2/.tmp_/}
> > >              case "$(basename $obj)" in
> > > -            version.o|\
> > >              debug.o|\
> > >              check.o|\
> >
> > Tangential question.  check.o is excluded because it's a toolchain test,
> > but any idea what debug.o is doing in this list?
> >
> > It can't possibly be the debug.c I've recently added to x86 (which we'll
> > want to be able to livepatch), so I guess it's got something to do the
> > ARM debug.S's, but I can't see anything in those that are worthy of
> > exemption either...
>
> Hm, that comes from the first commit that imported the wrapper to the
> repository, and at that point only x86 had livepatch support.
>
> I'm tempted to think this was inherited from the original xsplice
> tooling, and so debug.o needs to be removed from the list.
>

livepatch-build-tools is derived from the kpatch build tooling and
debug.o has never been present there so it was added here for a
reason. AFAICT the gdbsx code used to live in debug.o. I can't
recall why it was being marked as changed unnecessarily but since
that is no longer an issue and the code lives elsewhere, the debug.o
lines can be dropped.

Ross

On Wed, Dec 06, 2023 at 12:11:39PM +0000, Ross Lagerwall wrote:
> On Tue, Dec 5, 2023 at 2:57 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
> >
> > On Tue, Dec 05, 2023 at 02:15:05PM +0000, Andrew Cooper wrote:
> > > On 05/12/2023 12:34 pm, Roger Pau Monne wrote:
> > > > Currently version.o is explicitly ignored as the file would change as a result
> > > > of the orignal and the patched build having possibly different dates and
> > > > times.
> > > >
> > > > Fix such difference by exporting the date and time from the build script, so
> > > > that both builds share the same build time.  This allows checking for changes
> > > > in version.c, since the rest of fields need to be manually changed in order to
> > > > produce different output.
> > > >
> > > > Setting XEN_BUILD_{DATE,TIME} as an environment variable has been supported
> > > > since before livepatch support was added to Xen, so it's safe to export those
> > > > variables unconditionally.
> > > >
> > > > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > > > ---
> > > >  livepatch-build | 4 ++++
> > > >  livepatch-gcc   | 2 --
> > > >  2 files changed, 4 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/livepatch-build b/livepatch-build
> > > > index e2ccce4f7fd7..f622683fc56c 100755
> > > > --- a/livepatch-build
> > > > +++ b/livepatch-build
> > > > @@ -417,6 +417,10 @@ if [ "${SKIP}" != "build" ]; then
> > > >
> > > >      export CROSS_COMPILE="${TOOLSDIR}/livepatch-gcc "
> > > >
> > > > +    # Force same date and time to prevent unwanted changes in version.c
> > > > +    export XEN_BUILD_DATE=`LC_ALL=C date`
> > > > +    export XEN_BUILD_TIME=`LC_ALL=C date +%T`
> > >
> > > Date is the one that goes wrong every time, but everything else in
> > > compile.h can go wrong in a way that causes version.o to change.
> >
> > I've attempted to reflect that in "since the rest of fields need to be
> > manually changed in order to produce different output".
> >
> > For those to change there must be some kind of environment change
> > between the original and the patched version build, and hence I don't
> > think that would be supported.
> 
> In general, yes. However, with this patch changes to the
> hostname/domain/username would result in version.o being marked
> as changed even though it is entirely fine to build the live patch
> on a different build host from the original Xen.

Keep in mind livepatch-build-tools builds it's base version of Xen and
then a patched version, and that's how the diff is performed.  For the
hostname/domain/username changes to appear on the livepatch payload
such change would need to happen in the muddle of the execution of
livepatch-build.

This change doesn't prevent building the original Xen on a different
host than the one building the livepatch, and the
hostname/domain/username changes won't be part of the livepatch
payload.

> >
> > > Ideally, the pristine source for building livepatches would include a
> > > generated compile.h, and livepatch would have a way to force no
> > > regeneration of the header.  But I've got no idea how nice that would be
> > > to arrange.
> >
> > Yes, no idea how fragile that would be either.  IMO the proposed
> > approach is not that bad.
> >
> > > That way, you're using the same details as the Xen being patched, rather
> > > than hoping that two identical different details will cancel out in the
> > > binary diff.
> >
> > Another option is to set all the env variables to disable any
> > build time probing.  However things like compiler or version changing
> > between the original and the patched builds likely point out to issues
> > elsewhere, unless it's intentional modification of the helpers.
> >
> > > > +
> > > >      echo "Perform full initial build with ${CPUS} CPU(s)..."
> > > >      build_full
> > > >
> > > > diff --git a/livepatch-gcc b/livepatch-gcc
> > > > index fcad80551aa0..e4cb6fb59029 100755
> > > > --- a/livepatch-gcc
> > > > +++ b/livepatch-gcc
> > > > @@ -33,7 +33,6 @@ if [[ "$TOOLCHAINCMD" =~ $GCC_RE ]] ; then
> > > >              obj=$2
> > > >              [[ $2 = */.tmp_*.o ]] && obj=${2/.tmp_/}
> > > >              case "$(basename $obj)" in
> > > > -            version.o|\
> > > >              debug.o|\
> > > >              check.o|\
> > >
> > > Tangential question.  check.o is excluded because it's a toolchain test,
> > > but any idea what debug.o is doing in this list?
> > >
> > > It can't possibly be the debug.c I've recently added to x86 (which we'll
> > > want to be able to livepatch), so I guess it's got something to do the
> > > ARM debug.S's, but I can't see anything in those that are worthy of
> > > exemption either...
> >
> > Hm, that comes from the first commit that imported the wrapper to the
> > repository, and at that point only x86 had livepatch support.
> >
> > I'm tempted to think this was inherited from the original xsplice
> > tooling, and so debug.o needs to be removed from the list.
> >
> 
> livepatch-build-tools is derived from the kpatch build tooling and
> debug.o has never been present there so it was added here for a
> reason. AFAICT the gdbsx code used to live in debug.o. I can't
> recall why it was being marked as changed unnecessarily but since
> that is no longer an issue and the code lives elsewhere, the debug.o
> lines can be dropped.

Will someone send a patch for this, or should I do it?

Thanks, Roger.

On Mon, Dec 11, 2023 at 10:42 AM Roger Pau Monné <roger.pau@citrix.com> wrote:
>
> On Wed, Dec 06, 2023 at 12:11:39PM +0000, Ross Lagerwall wrote:
> > On Tue, Dec 5, 2023 at 2:57 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
> > >
> > > On Tue, Dec 05, 2023 at 02:15:05PM +0000, Andrew Cooper wrote:
> > > > On 05/12/2023 12:34 pm, Roger Pau Monne wrote:
> > > > > Currently version.o is explicitly ignored as the file would change as a result
> > > > > of the orignal and the patched build having possibly different dates and
> > > > > times.
> > > > >
> > > > > Fix such difference by exporting the date and time from the build script, so
> > > > > that both builds share the same build time.  This allows checking for changes
> > > > > in version.c, since the rest of fields need to be manually changed in order to
> > > > > produce different output.
> > > > >
> > > > > Setting XEN_BUILD_{DATE,TIME} as an environment variable has been supported
> > > > > since before livepatch support was added to Xen, so it's safe to export those
> > > > > variables unconditionally.
> > > > >
> > > > > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > > > > ---
> > > > >  livepatch-build | 4 ++++
> > > > >  livepatch-gcc   | 2 --
> > > > >  2 files changed, 4 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/livepatch-build b/livepatch-build
> > > > > index e2ccce4f7fd7..f622683fc56c 100755
> > > > > --- a/livepatch-build
> > > > > +++ b/livepatch-build
> > > > > @@ -417,6 +417,10 @@ if [ "${SKIP}" != "build" ]; then
> > > > >
> > > > >      export CROSS_COMPILE="${TOOLSDIR}/livepatch-gcc "
> > > > >
> > > > > +    # Force same date and time to prevent unwanted changes in version.c
> > > > > +    export XEN_BUILD_DATE=`LC_ALL=C date`
> > > > > +    export XEN_BUILD_TIME=`LC_ALL=C date +%T`
> > > >
> > > > Date is the one that goes wrong every time, but everything else in
> > > > compile.h can go wrong in a way that causes version.o to change.
> > >
> > > I've attempted to reflect that in "since the rest of fields need to be
> > > manually changed in order to produce different output".
> > >
> > > For those to change there must be some kind of environment change
> > > between the original and the patched version build, and hence I don't
> > > think that would be supported.
> >
> > In general, yes. However, with this patch changes to the
> > hostname/domain/username would result in version.o being marked
> > as changed even though it is entirely fine to build the live patch
> > on a different build host from the original Xen.
>
> Keep in mind livepatch-build-tools builds it's base version of Xen and
> then a patched version, and that's how the diff is performed.  For the
> hostname/domain/username changes to appear on the livepatch payload
> such change would need to happen in the muddle of the execution of
> livepatch-build.
>
> This change doesn't prevent building the original Xen on a different
> host than the one building the livepatch, and the
> hostname/domain/username changes won't be part of the livepatch
> payload.

Oh, of course. That makes sense.

There is one other issue that may cause version.o to change
unnecessarily.

XEN_CHANGESET may change when the source tree is a Git repo.
In some cases, it will change from <commit> to <commit>-dirty
when the patch is applied. I guess we could set .scmversion
explicitly to avoid this?

>
> > >
> > > > Ideally, the pristine source for building livepatches would include a
> > > > generated compile.h, and livepatch would have a way to force no
> > > > regeneration of the header.  But I've got no idea how nice that would be
> > > > to arrange.
> > >
> > > Yes, no idea how fragile that would be either.  IMO the proposed
> > > approach is not that bad.
> > >
> > > > That way, you're using the same details as the Xen being patched, rather
> > > > than hoping that two identical different details will cancel out in the
> > > > binary diff.
> > >
> > > Another option is to set all the env variables to disable any
> > > build time probing.  However things like compiler or version changing
> > > between the original and the patched builds likely point out to issues
> > > elsewhere, unless it's intentional modification of the helpers.
> > >
> > > > > +
> > > > >      echo "Perform full initial build with ${CPUS} CPU(s)..."
> > > > >      build_full
> > > > >
> > > > > diff --git a/livepatch-gcc b/livepatch-gcc
> > > > > index fcad80551aa0..e4cb6fb59029 100755
> > > > > --- a/livepatch-gcc
> > > > > +++ b/livepatch-gcc
> > > > > @@ -33,7 +33,6 @@ if [[ "$TOOLCHAINCMD" =~ $GCC_RE ]] ; then
> > > > >              obj=$2
> > > > >              [[ $2 = */.tmp_*.o ]] && obj=${2/.tmp_/}
> > > > >              case "$(basename $obj)" in
> > > > > -            version.o|\
> > > > >              debug.o|\
> > > > >              check.o|\
> > > >
> > > > Tangential question.  check.o is excluded because it's a toolchain test,
> > > > but any idea what debug.o is doing in this list?
> > > >
> > > > It can't possibly be the debug.c I've recently added to x86 (which we'll
> > > > want to be able to livepatch), so I guess it's got something to do the
> > > > ARM debug.S's, but I can't see anything in those that are worthy of
> > > > exemption either...
> > >
> > > Hm, that comes from the first commit that imported the wrapper to the
> > > repository, and at that point only x86 had livepatch support.
> > >
> > > I'm tempted to think this was inherited from the original xsplice
> > > tooling, and so debug.o needs to be removed from the list.
> > >
> >
> > livepatch-build-tools is derived from the kpatch build tooling and
> > debug.o has never been present there so it was added here for a
> > reason. AFAICT the gdbsx code used to live in debug.o. I can't
> > recall why it was being marked as changed unnecessarily but since
> > that is no longer an issue and the code lives elsewhere, the debug.o
> > lines can be dropped.
>
> Will someone send a patch for this, or should I do it?
>

I will send a patch for this.

Thanks,
Ross

On Wed, Dec 13, 2023 at 04:39:04PM +0000, Ross Lagerwall wrote:
> On Mon, Dec 11, 2023 at 10:42 AM Roger Pau Monné <roger.pau@citrix.com> wrote:
> >
> > On Wed, Dec 06, 2023 at 12:11:39PM +0000, Ross Lagerwall wrote:
> > > On Tue, Dec 5, 2023 at 2:57 PM Roger Pau Monné <roger.pau@citrix.com> wrote:
> > > >
> > > > On Tue, Dec 05, 2023 at 02:15:05PM +0000, Andrew Cooper wrote:
> > > > > On 05/12/2023 12:34 pm, Roger Pau Monne wrote:
> > > > > > Currently version.o is explicitly ignored as the file would change as a result
> > > > > > of the orignal and the patched build having possibly different dates and
> > > > > > times.
> > > > > >
> > > > > > Fix such difference by exporting the date and time from the build script, so
> > > > > > that both builds share the same build time.  This allows checking for changes
> > > > > > in version.c, since the rest of fields need to be manually changed in order to
> > > > > > produce different output.
> > > > > >
> > > > > > Setting XEN_BUILD_{DATE,TIME} as an environment variable has been supported
> > > > > > since before livepatch support was added to Xen, so it's safe to export those
> > > > > > variables unconditionally.
> > > > > >
> > > > > > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > > > > > ---
> > > > > >  livepatch-build | 4 ++++
> > > > > >  livepatch-gcc   | 2 --
> > > > > >  2 files changed, 4 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/livepatch-build b/livepatch-build
> > > > > > index e2ccce4f7fd7..f622683fc56c 100755
> > > > > > --- a/livepatch-build
> > > > > > +++ b/livepatch-build
> > > > > > @@ -417,6 +417,10 @@ if [ "${SKIP}" != "build" ]; then
> > > > > >
> > > > > >      export CROSS_COMPILE="${TOOLSDIR}/livepatch-gcc "
> > > > > >
> > > > > > +    # Force same date and time to prevent unwanted changes in version.c
> > > > > > +    export XEN_BUILD_DATE=`LC_ALL=C date`
> > > > > > +    export XEN_BUILD_TIME=`LC_ALL=C date +%T`
> > > > >
> > > > > Date is the one that goes wrong every time, but everything else in
> > > > > compile.h can go wrong in a way that causes version.o to change.
> > > >
> > > > I've attempted to reflect that in "since the rest of fields need to be
> > > > manually changed in order to produce different output".
> > > >
> > > > For those to change there must be some kind of environment change
> > > > between the original and the patched version build, and hence I don't
> > > > think that would be supported.
> > >
> > > In general, yes. However, with this patch changes to the
> > > hostname/domain/username would result in version.o being marked
> > > as changed even though it is entirely fine to build the live patch
> > > on a different build host from the original Xen.
> >
> > Keep in mind livepatch-build-tools builds it's base version of Xen and
> > then a patched version, and that's how the diff is performed.  For the
> > hostname/domain/username changes to appear on the livepatch payload
> > such change would need to happen in the muddle of the execution of
> > livepatch-build.
> >
> > This change doesn't prevent building the original Xen on a different
> > host than the one building the livepatch, and the
> > hostname/domain/username changes won't be part of the livepatch
> > payload.
> 
> Oh, of course. That makes sense.
> 
> There is one other issue that may cause version.o to change
> unnecessarily.
> 
> XEN_CHANGESET may change when the source tree is a Git repo.
> In some cases, it will change from <commit> to <commit>-dirty
> when the patch is applied. I guess we could set .scmversion
> explicitly to avoid this?

Oh, right, I did consider that <commit> cannot change, but it can
indeed become <commit>-dirty.  Will expand to make it fixed also.

> >
> > > >
> > > > > Ideally, the pristine source for building livepatches would include a
> > > > > generated compile.h, and livepatch would have a way to force no
> > > > > regeneration of the header.  But I've got no idea how nice that would be
> > > > > to arrange.
> > > >
> > > > Yes, no idea how fragile that would be either.  IMO the proposed
> > > > approach is not that bad.
> > > >
> > > > > That way, you're using the same details as the Xen being patched, rather
> > > > > than hoping that two identical different details will cancel out in the
> > > > > binary diff.
> > > >
> > > > Another option is to set all the env variables to disable any
> > > > build time probing.  However things like compiler or version changing
> > > > between the original and the patched builds likely point out to issues
> > > > elsewhere, unless it's intentional modification of the helpers.
> > > >
> > > > > > +
> > > > > >      echo "Perform full initial build with ${CPUS} CPU(s)..."
> > > > > >      build_full
> > > > > >
> > > > > > diff --git a/livepatch-gcc b/livepatch-gcc
> > > > > > index fcad80551aa0..e4cb6fb59029 100755
> > > > > > --- a/livepatch-gcc
> > > > > > +++ b/livepatch-gcc
> > > > > > @@ -33,7 +33,6 @@ if [[ "$TOOLCHAINCMD" =~ $GCC_RE ]] ; then
> > > > > >              obj=$2
> > > > > >              [[ $2 = */.tmp_*.o ]] && obj=${2/.tmp_/}
> > > > > >              case "$(basename $obj)" in
> > > > > > -            version.o|\
> > > > > >              debug.o|\
> > > > > >              check.o|\
> > > > >
> > > > > Tangential question.  check.o is excluded because it's a toolchain test,
> > > > > but any idea what debug.o is doing in this list?
> > > > >
> > > > > It can't possibly be the debug.c I've recently added to x86 (which we'll
> > > > > want to be able to livepatch), so I guess it's got something to do the
> > > > > ARM debug.S's, but I can't see anything in those that are worthy of
> > > > > exemption either...
> > > >
> > > > Hm, that comes from the first commit that imported the wrapper to the
> > > > repository, and at that point only x86 had livepatch support.
> > > >
> > > > I'm tempted to think this was inherited from the original xsplice
> > > > tooling, and so debug.o needs to be removed from the list.
> > > >
> > >
> > > livepatch-build-tools is derived from the kpatch build tooling and
> > > debug.o has never been present there so it was added here for a
> > > reason. AFAICT the gdbsx code used to live in debug.o. I can't
> > > recall why it was being marked as changed unnecessarily but since
> > > that is no longer an issue and the code lives elsewhere, the debug.o
> > > lines can be dropped.
> >
> > Will someone send a patch for this, or should I do it?
> >
> 
> I will send a patch for this.

Thanks, Roger.