[livepatch-build-tools 1/4] livepatch-gcc: Allow toolchain command with versions

[livepatch-build-tools 1/4] livepatch-gcc: Allow toolchain command with versions

[Pawel Wieczorkiewicz]

Xen build system may enforce particular gcc version (e.g. gcc72).
Make sure the livepatch-gcc script accepts all input toolchain gcc
commands with or without version specified.

Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Reviewed-by: Martin Mazein <amazein@amazon.de>
Reviewed-by: Bjoern Doebel <doebel@amazon.de>
---
 livepatch-gcc | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/livepatch-gcc b/livepatch-gcc
index 634157a..617f865 100755
--- a/livepatch-gcc
+++ b/livepatch-gcc
@@ -25,7 +25,8 @@ shift
 declare -a args=("$@")
 keep=no
 
-if [[ "$TOOLCHAINCMD" = "gcc" ]] ; then
+declare -r GCC_RE='gcc.*'
+if [[ "$TOOLCHAINCMD" =~ $GCC_RE ]] ; then
     while [ "$#" -gt 0 ]; do
         if [ "$1" = "-o" ]; then
             obj=$2
-- 
2.16.5




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[livepatch-build-tools 2/4] livepatch-gcc: Ignore built_in.o and prelink.o object files

[Pawel Wieczorkiewicz]

Do not copy over the built_in.o and prelink.o object files when they
get rebuilt as they are used for transient linking by Xen's build
system.

Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Reviewed-by: Martin Pohlack <mpohlack@amazon.de>
Reviewed-by: Petre Eftime <epetre@amazon.com>
---
 livepatch-gcc | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/livepatch-gcc b/livepatch-gcc
index 617f865..01e4b8c 100755
--- a/livepatch-gcc
+++ b/livepatch-gcc
@@ -35,6 +35,8 @@ if [[ "$TOOLCHAINCMD" =~ $GCC_RE ]] ; then
             version.o|\
             debug.o|\
             *.xen-syms.*.o|\
+            built_in.o|\
+            prelink.o|\
             .*.o)
                 break
                 ;;
-- 
2.16.5




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [livepatch-build-tools 2/4] livepatch-gcc: Ignore built_in.o and prelink.o object files

[Ross Lagerwall]

On 4/8/19 9:32 AM, Pawel Wieczorkiewicz wrote:
> Do not copy over the built_in.o and prelink.o object files when they
> get rebuilt as they are used for transient linking by Xen's build
> system.
> 
> Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
> Reviewed-by: Martin Pohlack <mpohlack@amazon.de>
> Reviewed-by: Petre Eftime <epetre@amazon.com>

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[livepatch-build-tools 3/4] livepatch-build: Do not follow every symlink for patch file

[Pawel Wieczorkiewicz]

In some build systems symlinks might be used for patch file names
to point from target directories to actual patches. Following those
symlinks breaks naming convention as the resulting built modules
would be named after the actual hardlink insteads of the symlink.

Livepatch-build obtains hotpatch name from the patch file, so it
should not canonicalize the file path resolving all the symlinks to
not lose the original symlink name.

Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Reviewed-by: Martin Pohlack <mpohlack@amazon.de>
Reviewed-by: Bjoern Doebel <doebel@amazon.de>
Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
---
 livepatch-build | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/livepatch-build b/livepatch-build
index c057fa1..796838c 100755
--- a/livepatch-build
+++ b/livepatch-build
@@ -265,7 +265,9 @@ done
 [ -z "$DEPENDS" ] && die "Build-id dependency not given"
 
 SRCDIR="$(readlink -m -- "$srcarg")"
-PATCHFILE="$(readlink -m -- "$patcharg")"
+# We need an absolute path because we move around, but we need to
+# retain the name of the symlink (= realpath -s)
+PATCHFILE="$(readlink -f "$(dirname "$patcharg")")/$(basename "$patcharg")"
 CONFIGFILE="$(readlink -m -- "$configarg")"
 OUTPUT="$(readlink -m -- "$outputarg")"
 
-- 
2.16.5




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [livepatch-build-tools 3/4] livepatch-build: Do not follow every symlink for patch file

[Ross Lagerwall]

On 4/8/19 9:32 AM, Pawel Wieczorkiewicz wrote:
> In some build systems symlinks might be used for patch file names
> to point from target directories to actual patches. Following those
> symlinks breaks naming convention as the resulting built modules
> would be named after the actual hardlink insteads of the symlink.
> 
> Livepatch-build obtains hotpatch name from the patch file, so it
> should not canonicalize the file path resolving all the symlinks to
> not lose the original symlink name.
> 
> Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
> Reviewed-by: Martin Pohlack <mpohlack@amazon.de>
> Reviewed-by: Bjoern Doebel <doebel@amazon.de>
> Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
> ---
>   livepatch-build | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/livepatch-build b/livepatch-build
> index c057fa1..796838c 100755
> --- a/livepatch-build
> +++ b/livepatch-build
> @@ -265,7 +265,9 @@ done
>   [ -z "$DEPENDS" ] && die "Build-id dependency not given"
>   
>   SRCDIR="$(readlink -m -- "$srcarg")"
> -PATCHFILE="$(readlink -m -- "$patcharg")"
> +# We need an absolute path because we move around, but we need to
> +# retain the name of the symlink (= realpath -s)
> +PATCHFILE="$(readlink -f "$(dirname "$patcharg")")/$(basename "$patcharg")"
>   CONFIGFILE="$(readlink -m -- "$configarg")"
>   OUTPUT="$(readlink -m -- "$outputarg")"
>   
> 

This works, but would it not be simpler to just pass $patcharg into 
make_patch_name()?

-- 
Ross Lagerwall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [livepatch-build-tools 3/4] livepatch-build: Do not follow every symlink for patch file

[Wieczorkiewicz, Pawel]

> On 29. Apr 2019, at 14:40, Ross Lagerwall <ross.lagerwall@citrix.com> wrote:
> 
> On 4/8/19 9:32 AM, Pawel Wieczorkiewicz wrote:
>> In some build systems symlinks might be used for patch file names
>> to point from target directories to actual patches. Following those
>> symlinks breaks naming convention as the resulting built modules
>> would be named after the actual hardlink insteads of the symlink.
>> Livepatch-build obtains hotpatch name from the patch file, so it
>> should not canonicalize the file path resolving all the symlinks to
>> not lose the original symlink name.
>> Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
>> Reviewed-by: Martin Pohlack <mpohlack@amazon.de>
>> Reviewed-by: Bjoern Doebel <doebel@amazon.de>
>> Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
>> ---
>>  livepatch-build | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>> diff --git a/livepatch-build b/livepatch-build
>> index c057fa1..796838c 100755
>> --- a/livepatch-build
>> +++ b/livepatch-build
>> @@ -265,7 +265,9 @@ done
>>  [ -z "$DEPENDS" ] && die "Build-id dependency not given"
>>    SRCDIR="$(readlink -m -- "$srcarg")"
>> -PATCHFILE="$(readlink -m -- "$patcharg")"
>> +# We need an absolute path because we move around, but we need to
>> +# retain the name of the symlink (= realpath -s)
>> +PATCHFILE="$(readlink -f "$(dirname "$patcharg")")/$(basename "$patcharg")"
>>  CONFIGFILE="$(readlink -m -- "$configarg")"
>>  OUTPUT="$(readlink -m -- "$outputarg")"
>>  
> 
> This works, but would it not be simpler to just pass $patcharg into make_patch_name()?
> 

No strong opinion here, but the readlink change felt less invasive (no changes to the existing semantics).

Thank you for looking into this.

> -- 
> Ross Lagerwall
> 


Best Regards,
Pawel Wieczorkiewicz





Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[livepatch-build-tools 4/4] livepatch-build: Handle newly created object files

[Pawel Wieczorkiewicz]

Up to now the livepatch-build ignores newly created object files.
When patch applies new .c file and augments its Makefile to build it
the resulting object file is not taken into account for final linking
step.

Such newly created object files can be detected by comparing patched/
and original/ directories and copied over to the output directory for
the final linking step.

Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
Reviewed-by: Bjoern Doebel <doebel@amazon.de>
Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
---
 livepatch-build | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/livepatch-build b/livepatch-build
index 796838c..b43730c 100755
--- a/livepatch-build
+++ b/livepatch-build
@@ -146,6 +146,12 @@ function create_patch()
         fi
     done
 
+    NEW_FILES=$(comm -23 <(find patched -type f -name '*.o' | cut -f2- -d'/' | sort -u) <(find original -type f -name '*.o' | cut -f2- -d'/' | sort -u))
+    for i in $NEW_FILES; do
+        cp "patched/$i" "output/$i"
+        CHANGED=1
+    done
+
     if [[ $ERROR -ne 0 ]]; then
         die "$ERROR error(s) encountered"
     fi
-- 
2.16.5




Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [livepatch-build-tools 4/4] livepatch-build: Handle newly created object files

[Andrew Cooper]

On 08/04/2019 09:32, Pawel Wieczorkiewicz wrote:
> Up to now the livepatch-build ignores newly created object files.
> When patch applies new .c file and augments its Makefile to build it
> the resulting object file is not taken into account for final linking
> step.
>
> Such newly created object files can be detected by comparing patched/
> and original/ directories and copied over to the output directory for
> the final linking step.
>
> Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
> Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
> Reviewed-by: Bjoern Doebel <doebel@amazon.de>
> Reviewed-by: Norbert Manthey <nmanthey@amazon.de>

This looks to resolve:

https://xenproject.atlassian.net/browse/XEN-90

(I think?)

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [livepatch-build-tools 4/4] livepatch-build: Handle newly created object files

[Ross Lagerwall]

On 4/29/19 1:53 PM, Andrew Cooper wrote:
> On 08/04/2019 09:32, Pawel Wieczorkiewicz wrote:
>> Up to now the livepatch-build ignores newly created object files.
>> When patch applies new .c file and augments its Makefile to build it
>> the resulting object file is not taken into account for final linking
>> step.
>>
>> Such newly created object files can be detected by comparing patched/
>> and original/ directories and copied over to the output directory for
>> the final linking step.
>>
>> Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
>> Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
>> Reviewed-by: Bjoern Doebel <doebel@amazon.de>
>> Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
> 
> This looks to resolve:
> 
> https://xenproject.atlassian.net/browse/XEN-90
> 
> (I think?)
> 

Yes, I expect it would.

-- 
Ross Lagerwall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [livepatch-build-tools 4/4] livepatch-build: Handle newly created object files

[Ross Lagerwall]

On 4/8/19 9:32 AM, Pawel Wieczorkiewicz wrote:
> Up to now the livepatch-build ignores newly created object files.
> When patch applies new .c file and augments its Makefile to build it
> the resulting object file is not taken into account for final linking
> step.
> 
> Such newly created object files can be detected by comparing patched/
> and original/ directories and copied over to the output directory for
> the final linking step.
> 
> Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
> Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
> Reviewed-by: Bjoern Doebel <doebel@amazon.de>
> Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
> ---
>   livepatch-build | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/livepatch-build b/livepatch-build
> index 796838c..b43730c 100755
> --- a/livepatch-build
> +++ b/livepatch-build
> @@ -146,6 +146,12 @@ function create_patch()
>           fi
>       done
>   
> +    NEW_FILES=$(comm -23 <(find patched -type f -name '*.o' | cut -f2- -d'/' | sort -u) <(find original -type f -name '*.o' | cut -f2- -d'/' | sort -u))

The paths should be `patched/xen` and `original/xen` so that only 
hypervisor changes are processed. It is done this way earlier (see 
FILES="$(find xen ...)").

Since process substitution creates a subshell, it might be simpler to do 
<(cd patched/xen && find . ...) and then drop the `cut`. Also, the `-u` 
argument to sort seems unnecessary.

> +    for i in $NEW_FILES; do
> +        cp "patched/$i" "output/$i"
> +        CHANGED=1
> +    done

If the live patch for whatever reason has no "patched" object files and 
only "new" object files, then it is not going to do anything useful 
since nothing will use the new functions. This should fail to build with 
an error. E.g. set NEW=1 above and then later check for CHANGED=0 && NEW=1.

Previously all object files that were linked into the livepatch were 
from the output of create-diff-object. This change just includes the 
newly built object files directly. I wonder if there are any issues or 
subtle differences when doing this? A couple of differences off the top 
of my head:

1) The object file will include _everything_ (e.g. potentially unused 
functions) while create-diff-object generally only includes the minimum 
that is needed.

2) Hooks and ignored functions/sections in the new object file would not 
be processed at all.

The was previously a patch on xen-devel which took a different approach 
(https://lists.xenproject.org/archives/html/xen-devel/2017-06/msg03532.html). 
Perhaps you could look at it and see which approach would be better?

Thanks,
-- 
Ross Lagerwall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [livepatch-build-tools 4/4] livepatch-build: Handle newly created object files

[Wieczorkiewicz, Pawel]

> On 29. Apr 2019, at 15:53, Ross Lagerwall <ross.lagerwall@citrix.com> wrote:
> 
> On 4/8/19 9:32 AM, Pawel Wieczorkiewicz wrote:
>> Up to now the livepatch-build ignores newly created object files.
>> When patch applies new .c file and augments its Makefile to build it
>> the resulting object file is not taken into account for final linking
>> step.
>> Such newly created object files can be detected by comparing patched/
>> and original/ directories and copied over to the output directory for
>> the final linking step.
>> Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
>> Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
>> Reviewed-by: Bjoern Doebel <doebel@amazon.de>
>> Reviewed-by: Norbert Manthey <nmanthey@amazon.de>
>> ---
>>  livepatch-build | 6 ++++++
>>  1 file changed, 6 insertions(+)
>> diff --git a/livepatch-build b/livepatch-build
>> index 796838c..b43730c 100755
>> --- a/livepatch-build
>> +++ b/livepatch-build
>> @@ -146,6 +146,12 @@ function create_patch()
>>          fi
>>      done
>>  +    NEW_FILES=$(comm -23 <(find patched -type f -name '*.o' | cut -f2- -d'/' | sort -u) <(find original -type f -name '*.o' | cut -f2- -d'/' | sort -u))
> 
> The paths should be `patched/xen` and `original/xen` so that only hypervisor changes are processed. It is done this way earlier (see FILES="$(find xen ...)").

ACK. Will fix.

> 
> Since process substitution creates a subshell, it might be simpler to do <(cd patched/xen && find . ...) and then drop the `cut`. Also, the `-u` argument to sort seems unnecessary.

ACK. Will fix.
The -u for sort is just my paranoia.

> 
>> +    for i in $NEW_FILES; do
>> +        cp "patched/$i" "output/$i"
>> +        CHANGED=1
>> +    done
> 
> If the live patch for whatever reason has no "patched" object files and only "new" object files, then it is not going to do anything useful since nothing will use the new functions. This should fail to build with an error. E.g. set NEW=1 above and then later check for CHANGED=0 && NEW=1.
> 

I disagree here. Inline assembly patching can use new functions even without having any "patched" objects.
Also hotpatch modules with hooks (functionality I will send upstream soon) can use the "new" objects provided
functionality for various reasons (re-registrating a callback for instance, or some testing related activities).

> Previously all object files that were linked into the livepatch were from the output of create-diff-object. This change just includes the newly built object files directly. I wonder if there are any issues or subtle differences when doing this? A couple of differences off the top of my head:
> 
> 1) The object file will include _everything_ (e.g. potentially unused functions) while create-diff-object generally only includes the minimum that is needed.

Yes, that’s right. When a patch defines new files (and thereby new object files) it should do it wisely.

> 
> 2) Hooks and ignored functions/sections in the new object file would not be processed at all.

Not sure I follow the point here, but hooks and functions properly defined in new object files will be
processed and used during link time.

I have a test hotpatch that does that.

> 
> The was previously a patch on xen-devel which took a different approach (https://lists.xenproject.org/archives/html/xen-devel/2017-06/msg03532.html). Perhaps you could look at it and see which approach would be better?

Taking a look. Thanks.

> 
> Thanks,
> -- 
> Ross Lagerwall
> 
> 

Best Regards,
Pawel Wieczorkiewicz



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [livepatch-build-tools 1/4] livepatch-gcc: Allow toolchain command with versions

[Ross Lagerwall]

On 4/8/19 9:32 AM, Pawel Wieczorkiewicz wrote:
> Xen build system may enforce particular gcc version (e.g. gcc72).
> Make sure the livepatch-gcc script accepts all input toolchain gcc
> commands with or without version specified.
> 
> Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
> Reviewed-by: Martin Mazein <amazein@amazon.de>
> Reviewed-by: Bjoern Doebel <doebel@amazon.de>
Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel