[Xen-devel] [PATCH v2] build: provide option to disambiguate symbol names

[Xen-devel] [PATCH v2] build: provide option to disambiguate symbol names

[Jan Beulich]

The .file assembler directives generated by the compiler do not include
any path components (gcc) or just the ones specified on the command line
(clang, at least version 5), and hence multiple identically named source
files (in different directories) may produce identically named static
symbols (in their kallsyms representation). The binary diffing algorithm
used by xen-livepatch, however, depends on having unique symbols.

Make the ENFORCE_UNIQUE_SYMBOLS Kconfig option control the (build)
behavior, and if enabled use objcopy to prepend the (relative to the
xen/ subdirectory) path to the compiler invoked STT_FILE symbols. Note
that this build option is made no longer depend on LIVEPATCH, but merely
defaults to its setting now.

Conditionalize explicit .file directive insertion in C files where it
exists just to disambiguate names in a less generic manner; note that
at the same time the redundant emission of STT_FILE symbols gets
suppressed for clang. Assembler files as well as multiply compiled C
ones using __OBJECT_FILE__ are left alone for the time being.

Since we now expect there not to be any duplicates anymore, also don't
force the selection of the option to 'n' anymore in allrandom.config.
Similarly COVERAGE no longer suppresses duplicate symbol warnings if
enforcement is in effect, which in turn allows
SUPPRESS_DUPLICATE_SYMBOL_WARNINGS to simply depend on
!ENFORCE_UNIQUE_SYMBOLS.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Re-base. Conditionalize COVERAGE's select.

The clang behavior may require further tweaking if different versions
behave differently. Alternatively we could pass two --redefine-sym
arguments to objcopy.

--- a/xen/Kconfig.debug
+++ b/xen/Kconfig.debug
@@ -38,7 +38,7 @@ config FRAME_POINTER
 config COVERAGE
 	bool "Code coverage support"
 	depends on !LIVEPATCH
-	select SUPPRESS_DUPLICATE_SYMBOL_WARNINGS
+	select SUPPRESS_DUPLICATE_SYMBOL_WARNINGS if !ENFORCE_UNIQUE_SYMBOLS
 	---help---
 	  Enable code coverage support.
 
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -194,12 +194,24 @@ FORCE:
 
 .PHONY: clean
 clean:: $(addprefix _clean_, $(subdir-all))
-	rm -f *.o *~ core $(DEPS_RM)
+	rm -f *.o .*.o.tmp *~ core $(DEPS_RM)
 _clean_%/: FORCE
 	$(MAKE) -f $(BASEDIR)/Rules.mk -C $* clean
 
+SRCPATH := $(patsubst $(BASEDIR)/%,%,$(CURDIR))
+
 %.o: %.c Makefile
+ifeq ($(CONFIG_ENFORCE_UNIQUE_SYMBOLS),y)
+	$(CC) $(CFLAGS) -c $< -o $(@D)/.$(@F).tmp
+ifeq ($(clang),y)
+	$(OBJCOPY) --redefine-sym $<=$(SRCPATH)/$< $(@D)/.$(@F).tmp $@
+else
+	$(OBJCOPY) --redefine-sym $(<F)=$(SRCPATH)/$< $(@D)/.$(@F).tmp $@
+endif
+	rm -f $(@D)/.$(@F).tmp
+else
 	$(CC) $(CFLAGS) -c $< -o $@
+endif
 
 %.o: %.S Makefile
 	$(CC) $(AFLAGS) -c $< -o $@
--- a/xen/arch/x86/x86_64/compat.c
+++ b/xen/arch/x86/x86_64/compat.c
@@ -2,7 +2,7 @@
  * compat.c
  */
 
-asm(".file \"" __FILE__ "\"");
+EMIT_FILE;
 
 #include <xen/hypercall.h>
 #include <compat/xen.h>
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -16,7 +16,7 @@
  * with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-asm(".file \"" __FILE__ "\"");
+EMIT_FILE;
 
 #include <xen/lib.h>
 #include <xen/init.h>
--- a/xen/arch/x86/x86_64/physdev.c
+++ b/xen/arch/x86/x86_64/physdev.c
@@ -2,7 +2,7 @@
  * physdev.c
  */
 
-asm(".file \"" __FILE__ "\"");
+EMIT_FILE;
 
 #include <xen/types.h>
 #include <xen/guest_access.h>
--- a/xen/arch/x86/x86_64/platform_hypercall.c
+++ b/xen/arch/x86/x86_64/platform_hypercall.c
@@ -2,7 +2,7 @@
  * platform_hypercall.c
  */
 
-asm(".file \"" __FILE__ "\"");
+EMIT_FILE;
 
 #include <xen/lib.h>
 #include <compat/platform.h>
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -373,8 +373,7 @@ config FAST_SYMBOL_LOOKUP
 
 config ENFORCE_UNIQUE_SYMBOLS
 	bool "Enforce unique symbols"
-	default y
-	depends on LIVEPATCH
+	default LIVEPATCH
 	---help---
 	  Multiple symbols with the same name aren't generally a problem
 	  unless livepatching is to be used.
@@ -387,8 +386,8 @@ config ENFORCE_UNIQUE_SYMBOLS
 	  livepatch build and apply correctly.
 
 config SUPPRESS_DUPLICATE_SYMBOL_WARNINGS
-	bool "Suppress duplicate symbol warnings" if !ENFORCE_UNIQUE_SYMBOLS
-	default y if !ENFORCE_UNIQUE_SYMBOLS
+	bool "Suppress duplicate symbol warnings"
+	depends on !ENFORCE_UNIQUE_SYMBOLS
 	---help---
 	  Multiple symbols with the same name aren't generally a problem
 	  unless Live patching is to be used, so these warnings can be
--- a/xen/common/compat/domain.c
+++ b/xen/common/compat/domain.c
@@ -3,7 +3,7 @@
  *
  */
 
-asm(".file \"" __FILE__ "\"");
+EMIT_FILE;
 
 #include <xen/lib.h>
 #include <xen/sched.h>
--- a/xen/common/compat/kernel.c
+++ b/xen/common/compat/kernel.c
@@ -2,7 +2,7 @@
  * kernel.c
  */
 
-asm(".file \"" __FILE__ "\"");
+EMIT_FILE;
 
 #include <xen/init.h>
 #include <xen/lib.h>
--- a/xen/common/compat/memory.c
+++ b/xen/common/compat/memory.c
@@ -1,4 +1,4 @@
-asm(".file \"" __FILE__ "\"");
+EMIT_FILE;
 
 #include <xen/types.h>
 #include <xen/hypercall.h>
--- a/xen/common/compat/multicall.c
+++ b/xen/common/compat/multicall.c
@@ -2,7 +2,7 @@
  * multicall.c
  */
 
-asm(".file \"" __FILE__ "\"");
+EMIT_FILE;
 
 #include <xen/types.h>
 #include <xen/multicall.h>
--- a/xen/include/xen/config.h
+++ b/xen/include/xen/config.h
@@ -11,7 +11,15 @@
 
 #ifndef __ASSEMBLY__
 #include <xen/compiler.h>
+
+#if defined(CONFIG_ENFORCE_UNIQUE_SYMBOLS) || defined(__clang__)
+# define EMIT_FILE asm ( "" )
+#else
+# define EMIT_FILE asm ( ".file \"" __FILE__ "\"" )
+#endif
+
 #endif
+
 #include <asm/config.h>
 
 #define EXPORT_SYMBOL(var)
--- a/xen/tools/kconfig/allrandom.config
+++ b/xen/tools/kconfig/allrandom.config
@@ -2,4 +2,3 @@
 
 CONFIG_GCOV_FORMAT_AUTODETECT=y
 CONFIG_UBSAN=n
-CONFIG_ENFORCE_UNIQUE_SYMBOLS=n

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

Re: [Xen-devel] [PATCH v2] build: provide option to disambiguate symbol names

[Konrad Rzeszutek Wilk]

On Fri, Nov 08, 2019 at 12:18:40PM +0100, Jan Beulich wrote:
> The .file assembler directives generated by the compiler do not include
> any path components (gcc) or just the ones specified on the command line
> (clang, at least version 5), and hence multiple identically named source
> files (in different directories) may produce identically named static
> symbols (in their kallsyms representation). The binary diffing algorithm
> used by xen-livepatch, however, depends on having unique symbols.
> 
> Make the ENFORCE_UNIQUE_SYMBOLS Kconfig option control the (build)
> behavior, and if enabled use objcopy to prepend the (relative to the
> xen/ subdirectory) path to the compiler invoked STT_FILE symbols. Note
> that this build option is made no longer depend on LIVEPATCH, but merely
> defaults to its setting now.
> 
> Conditionalize explicit .file directive insertion in C files where it
> exists just to disambiguate names in a less generic manner; note that
> at the same time the redundant emission of STT_FILE symbols gets
> suppressed for clang. Assembler files as well as multiply compiled C
> ones using __OBJECT_FILE__ are left alone for the time being.
> 
> Since we now expect there not to be any duplicates anymore, also don't
> force the selection of the option to 'n' anymore in allrandom.config.
> Similarly COVERAGE no longer suppresses duplicate symbol warnings if
> enforcement is in effect, which in turn allows
> SUPPRESS_DUPLICATE_SYMBOL_WARNINGS to simply depend on
> !ENFORCE_UNIQUE_SYMBOLS.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

> ---
> v2: Re-base. Conditionalize COVERAGE's select.
> 
> The clang behavior may require further tweaking if different versions
> behave differently. Alternatively we could pass two --redefine-sym
> arguments to objcopy.
> 
> --- a/xen/Kconfig.debug
> +++ b/xen/Kconfig.debug
> @@ -38,7 +38,7 @@ config FRAME_POINTER
>  config COVERAGE
>  	bool "Code coverage support"
>  	depends on !LIVEPATCH
> -	select SUPPRESS_DUPLICATE_SYMBOL_WARNINGS
> +	select SUPPRESS_DUPLICATE_SYMBOL_WARNINGS if !ENFORCE_UNIQUE_SYMBOLS
>  	---help---
>  	  Enable code coverage support.
>  
> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -194,12 +194,24 @@ FORCE:
>  
>  .PHONY: clean
>  clean:: $(addprefix _clean_, $(subdir-all))
> -	rm -f *.o *~ core $(DEPS_RM)
> +	rm -f *.o .*.o.tmp *~ core $(DEPS_RM)
>  _clean_%/: FORCE
>  	$(MAKE) -f $(BASEDIR)/Rules.mk -C $* clean
>  
> +SRCPATH := $(patsubst $(BASEDIR)/%,%,$(CURDIR))
> +
>  %.o: %.c Makefile
> +ifeq ($(CONFIG_ENFORCE_UNIQUE_SYMBOLS),y)
> +	$(CC) $(CFLAGS) -c $< -o $(@D)/.$(@F).tmp
> +ifeq ($(clang),y)
> +	$(OBJCOPY) --redefine-sym $<=$(SRCPATH)/$< $(@D)/.$(@F).tmp $@
> +else
> +	$(OBJCOPY) --redefine-sym $(<F)=$(SRCPATH)/$< $(@D)/.$(@F).tmp $@
> +endif
> +	rm -f $(@D)/.$(@F).tmp
> +else
>  	$(CC) $(CFLAGS) -c $< -o $@
> +endif
>  
>  %.o: %.S Makefile
>  	$(CC) $(AFLAGS) -c $< -o $@
> --- a/xen/arch/x86/x86_64/compat.c
> +++ b/xen/arch/x86/x86_64/compat.c
> @@ -2,7 +2,7 @@
>   * compat.c
>   */
>  
> -asm(".file \"" __FILE__ "\"");
> +EMIT_FILE;
>  
>  #include <xen/hypercall.h>
>  #include <compat/xen.h>
> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -16,7 +16,7 @@
>   * with this program; If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> -asm(".file \"" __FILE__ "\"");
> +EMIT_FILE;
>  
>  #include <xen/lib.h>
>  #include <xen/init.h>
> --- a/xen/arch/x86/x86_64/physdev.c
> +++ b/xen/arch/x86/x86_64/physdev.c
> @@ -2,7 +2,7 @@
>   * physdev.c
>   */
>  
> -asm(".file \"" __FILE__ "\"");
> +EMIT_FILE;
>  
>  #include <xen/types.h>
>  #include <xen/guest_access.h>
> --- a/xen/arch/x86/x86_64/platform_hypercall.c
> +++ b/xen/arch/x86/x86_64/platform_hypercall.c
> @@ -2,7 +2,7 @@
>   * platform_hypercall.c
>   */
>  
> -asm(".file \"" __FILE__ "\"");
> +EMIT_FILE;
>  
>  #include <xen/lib.h>
>  #include <compat/platform.h>
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -373,8 +373,7 @@ config FAST_SYMBOL_LOOKUP
>  
>  config ENFORCE_UNIQUE_SYMBOLS
>  	bool "Enforce unique symbols"
> -	default y
> -	depends on LIVEPATCH
> +	default LIVEPATCH
>  	---help---
>  	  Multiple symbols with the same name aren't generally a problem
>  	  unless livepatching is to be used.
> @@ -387,8 +386,8 @@ config ENFORCE_UNIQUE_SYMBOLS
>  	  livepatch build and apply correctly.
>  
>  config SUPPRESS_DUPLICATE_SYMBOL_WARNINGS
> -	bool "Suppress duplicate symbol warnings" if !ENFORCE_UNIQUE_SYMBOLS
> -	default y if !ENFORCE_UNIQUE_SYMBOLS
> +	bool "Suppress duplicate symbol warnings"
> +	depends on !ENFORCE_UNIQUE_SYMBOLS
>  	---help---
>  	  Multiple symbols with the same name aren't generally a problem
>  	  unless Live patching is to be used, so these warnings can be
> --- a/xen/common/compat/domain.c
> +++ b/xen/common/compat/domain.c
> @@ -3,7 +3,7 @@
>   *
>   */
>  
> -asm(".file \"" __FILE__ "\"");
> +EMIT_FILE;
>  
>  #include <xen/lib.h>
>  #include <xen/sched.h>
> --- a/xen/common/compat/kernel.c
> +++ b/xen/common/compat/kernel.c
> @@ -2,7 +2,7 @@
>   * kernel.c
>   */
>  
> -asm(".file \"" __FILE__ "\"");
> +EMIT_FILE;
>  
>  #include <xen/init.h>
>  #include <xen/lib.h>
> --- a/xen/common/compat/memory.c
> +++ b/xen/common/compat/memory.c
> @@ -1,4 +1,4 @@
> -asm(".file \"" __FILE__ "\"");
> +EMIT_FILE;
>  
>  #include <xen/types.h>
>  #include <xen/hypercall.h>
> --- a/xen/common/compat/multicall.c
> +++ b/xen/common/compat/multicall.c
> @@ -2,7 +2,7 @@
>   * multicall.c
>   */
>  
> -asm(".file \"" __FILE__ "\"");
> +EMIT_FILE;
>  
>  #include <xen/types.h>
>  #include <xen/multicall.h>
> --- a/xen/include/xen/config.h
> +++ b/xen/include/xen/config.h
> @@ -11,7 +11,15 @@
>  
>  #ifndef __ASSEMBLY__
>  #include <xen/compiler.h>
> +
> +#if defined(CONFIG_ENFORCE_UNIQUE_SYMBOLS) || defined(__clang__)
> +# define EMIT_FILE asm ( "" )
> +#else
> +# define EMIT_FILE asm ( ".file \"" __FILE__ "\"" )
> +#endif
> +
>  #endif
> +
>  #include <asm/config.h>
>  
>  #define EXPORT_SYMBOL(var)
> --- a/xen/tools/kconfig/allrandom.config
> +++ b/xen/tools/kconfig/allrandom.config
> @@ -2,4 +2,3 @@
>  
>  CONFIG_GCOV_FORMAT_AUTODETECT=y
>  CONFIG_UBSAN=n
> -CONFIG_ENFORCE_UNIQUE_SYMBOLS=n

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

Re: [Xen-devel] [PATCH v2] build: provide option to disambiguate symbol names

[Wei Liu]

On Fri, Nov 08, 2019 at 12:18:40PM +0100, Jan Beulich wrote:
> The .file assembler directives generated by the compiler do not include
> any path components (gcc) or just the ones specified on the command line
> (clang, at least version 5), and hence multiple identically named source
> files (in different directories) may produce identically named static
> symbols (in their kallsyms representation). The binary diffing algorithm
> used by xen-livepatch, however, depends on having unique symbols.
> 
> Make the ENFORCE_UNIQUE_SYMBOLS Kconfig option control the (build)
> behavior, and if enabled use objcopy to prepend the (relative to the
> xen/ subdirectory) path to the compiler invoked STT_FILE symbols. Note
> that this build option is made no longer depend on LIVEPATCH, but merely
> defaults to its setting now.
> 
> Conditionalize explicit .file directive insertion in C files where it
> exists just to disambiguate names in a less generic manner; note that
> at the same time the redundant emission of STT_FILE symbols gets
> suppressed for clang. Assembler files as well as multiply compiled C
> ones using __OBJECT_FILE__ are left alone for the time being.
> 
> Since we now expect there not to be any duplicates anymore, also don't
> force the selection of the option to 'n' anymore in allrandom.config.
> Similarly COVERAGE no longer suppresses duplicate symbol warnings if
> enforcement is in effect, which in turn allows
> SUPPRESS_DUPLICATE_SYMBOL_WARNINGS to simply depend on
> !ENFORCE_UNIQUE_SYMBOLS.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Wei Liu <wl@xen.org>

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

[Xen-devel] Ping: [PATCH v2] build: provide option to disambiguate symbol names

[Jan Beulich]

On 08.11.2019 12:18, Jan Beulich wrote:
> The .file assembler directives generated by the compiler do not include
> any path components (gcc) or just the ones specified on the command line
> (clang, at least version 5), and hence multiple identically named source
> files (in different directories) may produce identically named static
> symbols (in their kallsyms representation). The binary diffing algorithm
> used by xen-livepatch, however, depends on having unique symbols.
> 
> Make the ENFORCE_UNIQUE_SYMBOLS Kconfig option control the (build)
> behavior, and if enabled use objcopy to prepend the (relative to the
> xen/ subdirectory) path to the compiler invoked STT_FILE symbols. Note
> that this build option is made no longer depend on LIVEPATCH, but merely
> defaults to its setting now.
> 
> Conditionalize explicit .file directive insertion in C files where it
> exists just to disambiguate names in a less generic manner; note that
> at the same time the redundant emission of STT_FILE symbols gets
> suppressed for clang. Assembler files as well as multiply compiled C
> ones using __OBJECT_FILE__ are left alone for the time being.
> 
> Since we now expect there not to be any duplicates anymore, also don't
> force the selection of the option to 'n' anymore in allrandom.config.
> Similarly COVERAGE no longer suppresses duplicate symbol warnings if
> enforcement is in effect, which in turn allows
> SUPPRESS_DUPLICATE_SYMBOL_WARNINGS to simply depend on
> !ENFORCE_UNIQUE_SYMBOLS.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I've got acks from Konrad and Wei, but still need an x86 and a release
one here. Andrew? Or alternatively - Jürgen, would you rather not see
this go in anymore?

Jan

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

Re: [Xen-devel] Ping: [PATCH v2] build: provide option to disambiguate symbol names

[Jürgen Groß]

On 20.11.19 17:30, Jan Beulich wrote:
> On 08.11.2019 12:18, Jan Beulich wrote:
>> The .file assembler directives generated by the compiler do not include
>> any path components (gcc) or just the ones specified on the command line
>> (clang, at least version 5), and hence multiple identically named source
>> files (in different directories) may produce identically named static
>> symbols (in their kallsyms representation). The binary diffing algorithm
>> used by xen-livepatch, however, depends on having unique symbols.
>>
>> Make the ENFORCE_UNIQUE_SYMBOLS Kconfig option control the (build)
>> behavior, and if enabled use objcopy to prepend the (relative to the
>> xen/ subdirectory) path to the compiler invoked STT_FILE symbols. Note
>> that this build option is made no longer depend on LIVEPATCH, but merely
>> defaults to its setting now.
>>
>> Conditionalize explicit .file directive insertion in C files where it
>> exists just to disambiguate names in a less generic manner; note that
>> at the same time the redundant emission of STT_FILE symbols gets
>> suppressed for clang. Assembler files as well as multiply compiled C
>> ones using __OBJECT_FILE__ are left alone for the time being.
>>
>> Since we now expect there not to be any duplicates anymore, also don't
>> force the selection of the option to 'n' anymore in allrandom.config.
>> Similarly COVERAGE no longer suppresses duplicate symbol warnings if
>> enforcement is in effect, which in turn allows
>> SUPPRESS_DUPLICATE_SYMBOL_WARNINGS to simply depend on
>> !ENFORCE_UNIQUE_SYMBOLS.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> I've got acks from Konrad and Wei, but still need an x86 and a release
> one here. Andrew? Or alternatively - Jürgen, would you rather not see
> this go in anymore?

In case the needed x86 Ack is coming in before RC3 I'm fine to give my
Release-ack, but I'm hesitant to take it later.


Juergen

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

Re: [Xen-devel] Ping: [PATCH v2] build: provide option to disambiguate symbol names

[Andrew Cooper]

On 20/11/2019 16:40, Jürgen Groß wrote:
> On 20.11.19 17:30, Jan Beulich wrote:
>> On 08.11.2019 12:18, Jan Beulich wrote:
>>> The .file assembler directives generated by the compiler do not include
>>> any path components (gcc) or just the ones specified on the command
>>> line
>>> (clang, at least version 5), and hence multiple identically named
>>> source
>>> files (in different directories) may produce identically named static
>>> symbols (in their kallsyms representation). The binary diffing
>>> algorithm
>>> used by xen-livepatch, however, depends on having unique symbols.
>>>
>>> Make the ENFORCE_UNIQUE_SYMBOLS Kconfig option control the (build)
>>> behavior, and if enabled use objcopy to prepend the (relative to the
>>> xen/ subdirectory) path to the compiler invoked STT_FILE symbols. Note
>>> that this build option is made no longer depend on LIVEPATCH, but
>>> merely
>>> defaults to its setting now.
>>>
>>> Conditionalize explicit .file directive insertion in C files where it
>>> exists just to disambiguate names in a less generic manner; note that
>>> at the same time the redundant emission of STT_FILE symbols gets
>>> suppressed for clang. Assembler files as well as multiply compiled C
>>> ones using __OBJECT_FILE__ are left alone for the time being.
>>>
>>> Since we now expect there not to be any duplicates anymore, also don't
>>> force the selection of the option to 'n' anymore in allrandom.config.
>>> Similarly COVERAGE no longer suppresses duplicate symbol warnings if
>>> enforcement is in effect, which in turn allows
>>> SUPPRESS_DUPLICATE_SYMBOL_WARNINGS to simply depend on
>>> !ENFORCE_UNIQUE_SYMBOLS.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> I've got acks from Konrad and Wei, but still need an x86 and a release
>> one here. Andrew? Or alternatively - Jürgen, would you rather not see
>> this go in anymore?
>
> In case the needed x86 Ack is coming in before RC3 I'm fine to give my
> Release-ack, but I'm hesitant to take it later.

Has anyone actually tried building a livepatch with this change in place?

I ask, because there is 0 testing of livepatches, and already one major
regression in 4.13 which forces XenServer to revert back to older build
tools.

~Andrew

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

Re: [Xen-devel] Ping: [PATCH v2] build: provide option to disambiguate symbol names

[Jan Beulich]

On 20.11.2019 18:13, Andrew Cooper wrote:
> On 20/11/2019 16:40, Jürgen Groß wrote:
>> On 20.11.19 17:30, Jan Beulich wrote:
>>> On 08.11.2019 12:18, Jan Beulich wrote:
>>>> The .file assembler directives generated by the compiler do not include
>>>> any path components (gcc) or just the ones specified on the command
>>>> line
>>>> (clang, at least version 5), and hence multiple identically named
>>>> source
>>>> files (in different directories) may produce identically named static
>>>> symbols (in their kallsyms representation). The binary diffing
>>>> algorithm
>>>> used by xen-livepatch, however, depends on having unique symbols.
>>>>
>>>> Make the ENFORCE_UNIQUE_SYMBOLS Kconfig option control the (build)
>>>> behavior, and if enabled use objcopy to prepend the (relative to the
>>>> xen/ subdirectory) path to the compiler invoked STT_FILE symbols. Note
>>>> that this build option is made no longer depend on LIVEPATCH, but
>>>> merely
>>>> defaults to its setting now.
>>>>
>>>> Conditionalize explicit .file directive insertion in C files where it
>>>> exists just to disambiguate names in a less generic manner; note that
>>>> at the same time the redundant emission of STT_FILE symbols gets
>>>> suppressed for clang. Assembler files as well as multiply compiled C
>>>> ones using __OBJECT_FILE__ are left alone for the time being.
>>>>
>>>> Since we now expect there not to be any duplicates anymore, also don't
>>>> force the selection of the option to 'n' anymore in allrandom.config.
>>>> Similarly COVERAGE no longer suppresses duplicate symbol warnings if
>>>> enforcement is in effect, which in turn allows
>>>> SUPPRESS_DUPLICATE_SYMBOL_WARNINGS to simply depend on
>>>> !ENFORCE_UNIQUE_SYMBOLS.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> I've got acks from Konrad and Wei, but still need an x86 and a release
>>> one here. Andrew? Or alternatively - Jürgen, would you rather not see
>>> this go in anymore?
>>
>> In case the needed x86 Ack is coming in before RC3 I'm fine to give my
>> Release-ack, but I'm hesitant to take it later.
> 
> Has anyone actually tried building a livepatch with this change in place?

I've never tried building any live patch, so I also didn't test
this angle of this change. But I did verify the results of what
the change here does.

I'm also a little puzzled about this response because I did the
change upon your request.

> I ask, because there is 0 testing of livepatches, and already one major
> regression in 4.13 which forces XenServer to revert back to older build
> tools.

That's a build tools regression, isn't it? I.e. not really related
to 4.13?

Jan

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

Re: [Xen-devel] Ping: [PATCH v2] build: provide option to disambiguate symbol names

[Andrew Cooper]

On 20/11/2019 17:19, Jan Beulich wrote:
> On 20.11.2019 18:13, Andrew Cooper wrote:
>> On 20/11/2019 16:40, Jürgen Groß wrote:
>>> On 20.11.19 17:30, Jan Beulich wrote:
>>>> On 08.11.2019 12:18, Jan Beulich wrote:
>>>>> The .file assembler directives generated by the compiler do not include
>>>>> any path components (gcc) or just the ones specified on the command
>>>>> line
>>>>> (clang, at least version 5), and hence multiple identically named
>>>>> source
>>>>> files (in different directories) may produce identically named static
>>>>> symbols (in their kallsyms representation). The binary diffing
>>>>> algorithm
>>>>> used by xen-livepatch, however, depends on having unique symbols.
>>>>>
>>>>> Make the ENFORCE_UNIQUE_SYMBOLS Kconfig option control the (build)
>>>>> behavior, and if enabled use objcopy to prepend the (relative to the
>>>>> xen/ subdirectory) path to the compiler invoked STT_FILE symbols. Note
>>>>> that this build option is made no longer depend on LIVEPATCH, but
>>>>> merely
>>>>> defaults to its setting now.
>>>>>
>>>>> Conditionalize explicit .file directive insertion in C files where it
>>>>> exists just to disambiguate names in a less generic manner; note that
>>>>> at the same time the redundant emission of STT_FILE symbols gets
>>>>> suppressed for clang. Assembler files as well as multiply compiled C
>>>>> ones using __OBJECT_FILE__ are left alone for the time being.
>>>>>
>>>>> Since we now expect there not to be any duplicates anymore, also don't
>>>>> force the selection of the option to 'n' anymore in allrandom.config.
>>>>> Similarly COVERAGE no longer suppresses duplicate symbol warnings if
>>>>> enforcement is in effect, which in turn allows
>>>>> SUPPRESS_DUPLICATE_SYMBOL_WARNINGS to simply depend on
>>>>> !ENFORCE_UNIQUE_SYMBOLS.
>>>>>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> I've got acks from Konrad and Wei, but still need an x86 and a release
>>>> one here. Andrew? Or alternatively - Jürgen, would you rather not see
>>>> this go in anymore?
>>> In case the needed x86 Ack is coming in before RC3 I'm fine to give my
>>> Release-ack, but I'm hesitant to take it later.
>> Has anyone actually tried building a livepatch with this change in place?
> I've never tried building any live patch, so I also didn't test
> this angle of this change. But I did verify the results of what
> the change here does.
>
> I'm also a little puzzled about this response because I did the
> change upon your request.
>
>> I ask, because there is 0 testing of livepatches, and already one major
>> regression in 4.13 which forces XenServer to revert back to older build
>> tools.
> That's a build tools regression, isn't it? I.e. not really related
> to 4.13?

I believe it is a build tools regression, rather than a 4.13 regression.

However, we are in the position that there is a supported tool with no
adequate testing in place, with one rather terminal regression in the
4.13 timeframe.  All I'm doing is being cautious about making changes
which have a real likelihood of affecting the status quo.

~Andrew

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

Re: [Xen-devel] Ping: [PATCH v2] build: provide option to disambiguate symbol names

[Jan Beulich]

On 20.11.2019 18:13, Andrew Cooper wrote:
> On 20/11/2019 16:40, Jürgen Groß wrote:
>> On 20.11.19 17:30, Jan Beulich wrote:
>>> On 08.11.2019 12:18, Jan Beulich wrote:
>>>> The .file assembler directives generated by the compiler do not include
>>>> any path components (gcc) or just the ones specified on the command
>>>> line
>>>> (clang, at least version 5), and hence multiple identically named
>>>> source
>>>> files (in different directories) may produce identically named static
>>>> symbols (in their kallsyms representation). The binary diffing
>>>> algorithm
>>>> used by xen-livepatch, however, depends on having unique symbols.
>>>>
>>>> Make the ENFORCE_UNIQUE_SYMBOLS Kconfig option control the (build)
>>>> behavior, and if enabled use objcopy to prepend the (relative to the
>>>> xen/ subdirectory) path to the compiler invoked STT_FILE symbols. Note
>>>> that this build option is made no longer depend on LIVEPATCH, but
>>>> merely
>>>> defaults to its setting now.
>>>>
>>>> Conditionalize explicit .file directive insertion in C files where it
>>>> exists just to disambiguate names in a less generic manner; note that
>>>> at the same time the redundant emission of STT_FILE symbols gets
>>>> suppressed for clang. Assembler files as well as multiply compiled C
>>>> ones using __OBJECT_FILE__ are left alone for the time being.
>>>>
>>>> Since we now expect there not to be any duplicates anymore, also don't
>>>> force the selection of the option to 'n' anymore in allrandom.config.
>>>> Similarly COVERAGE no longer suppresses duplicate symbol warnings if
>>>> enforcement is in effect, which in turn allows
>>>> SUPPRESS_DUPLICATE_SYMBOL_WARNINGS to simply depend on
>>>> !ENFORCE_UNIQUE_SYMBOLS.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> I've got acks from Konrad and Wei, but still need an x86 and a release
>>> one here. Andrew? Or alternatively - Jürgen, would you rather not see
>>> this go in anymore?
>>
>> In case the needed x86 Ack is coming in before RC3 I'm fine to give my
>> Release-ack, but I'm hesitant to take it later.
> 
> Has anyone actually tried building a livepatch with this change in place?

Actually - what is your concern here? The exact spelling of symbols
names should be of no interest to the tool. After all the compiler is
free to invent all sorts of names for its local symbols, including
the ones we would produce with this change in place. All the tool
cares about is that the names be unambiguous. Hence any (theoretical)
regression here would be a bug in the tools, which imo is no reason
to delay this change any further. (Granted I should have got to it
earlier, but it had been continuing to get deferred.)

Jan

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

Re: [Xen-devel] [PATCH v2] build: provide option to disambiguate symbol names

[Roger Pau Monné]

On Fri, Nov 08, 2019 at 12:18:40PM +0100, Jan Beulich wrote:
> The .file assembler directives generated by the compiler do not include
> any path components (gcc) or just the ones specified on the command line
> (clang, at least version 5), and hence multiple identically named source
> files (in different directories) may produce identically named static
> symbols (in their kallsyms representation). The binary diffing algorithm
> used by xen-livepatch, however, depends on having unique symbols.
> 
> Make the ENFORCE_UNIQUE_SYMBOLS Kconfig option control the (build)
> behavior, and if enabled use objcopy to prepend the (relative to the
> xen/ subdirectory) path to the compiler invoked STT_FILE symbols. Note
> that this build option is made no longer depend on LIVEPATCH, but merely
> defaults to its setting now.
> 
> Conditionalize explicit .file directive insertion in C files where it
> exists just to disambiguate names in a less generic manner; note that
> at the same time the redundant emission of STT_FILE symbols gets
> suppressed for clang. Assembler files as well as multiply compiled C
> ones using __OBJECT_FILE__ are left alone for the time being.
> 
> Since we now expect there not to be any duplicates anymore, also don't
> force the selection of the option to 'n' anymore in allrandom.config.
> Similarly COVERAGE no longer suppresses duplicate symbol warnings if
> enforcement is in effect, which in turn allows
> SUPPRESS_DUPLICATE_SYMBOL_WARNINGS to simply depend on
> !ENFORCE_UNIQUE_SYMBOLS.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

And tested on FreeBSD with clang 8 and elftoolchain objcopy.

Thanks, Roger.

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

Re: [Xen-devel] Ping: [PATCH v2] build: provide option to disambiguate symbol names

[Andrew Cooper]

On 21/11/2019 08:34, Jan Beulich wrote:
> On 20.11.2019 18:13, Andrew Cooper wrote:
>> On 20/11/2019 16:40, Jürgen Groß wrote:
>>> On 20.11.19 17:30, Jan Beulich wrote:
>>>> On 08.11.2019 12:18, Jan Beulich wrote:
>>>>> The .file assembler directives generated by the compiler do not include
>>>>> any path components (gcc) or just the ones specified on the command
>>>>> line
>>>>> (clang, at least version 5), and hence multiple identically named
>>>>> source
>>>>> files (in different directories) may produce identically named static
>>>>> symbols (in their kallsyms representation). The binary diffing
>>>>> algorithm
>>>>> used by xen-livepatch, however, depends on having unique symbols.
>>>>>
>>>>> Make the ENFORCE_UNIQUE_SYMBOLS Kconfig option control the (build)
>>>>> behavior, and if enabled use objcopy to prepend the (relative to the
>>>>> xen/ subdirectory) path to the compiler invoked STT_FILE symbols. Note
>>>>> that this build option is made no longer depend on LIVEPATCH, but
>>>>> merely
>>>>> defaults to its setting now.
>>>>>
>>>>> Conditionalize explicit .file directive insertion in C files where it
>>>>> exists just to disambiguate names in a less generic manner; note that
>>>>> at the same time the redundant emission of STT_FILE symbols gets
>>>>> suppressed for clang. Assembler files as well as multiply compiled C
>>>>> ones using __OBJECT_FILE__ are left alone for the time being.
>>>>>
>>>>> Since we now expect there not to be any duplicates anymore, also don't
>>>>> force the selection of the option to 'n' anymore in allrandom.config.
>>>>> Similarly COVERAGE no longer suppresses duplicate symbol warnings if
>>>>> enforcement is in effect, which in turn allows
>>>>> SUPPRESS_DUPLICATE_SYMBOL_WARNINGS to simply depend on
>>>>> !ENFORCE_UNIQUE_SYMBOLS.
>>>>>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> I've got acks from Konrad and Wei, but still need an x86 and a release
>>>> one here. Andrew? Or alternatively - Jürgen, would you rather not see
>>>> this go in anymore?
>>> In case the needed x86 Ack is coming in before RC3 I'm fine to give my
>>> Release-ack, but I'm hesitant to take it later.
>> Has anyone actually tried building a livepatch with this change in place?
> Actually - what is your concern here? The exact spelling of symbols
> names should be of no interest to the tool. After all the compiler is
> free to invent all sorts of names for its local symbols, including
> the ones we would produce with this change in place. All the tool
> cares about is that the names be unambiguous. Hence any (theoretical)
> regression here would be a bug in the tools, which imo is no reason
> to delay this change any further. (Granted I should have got to it
> earlier, but it had been continuing to get deferred.)

This might all be true (theoretically).

The livepatch build tools are fragile and very sensitive to how the
object files are laid out.  There is a very real risk that this change
accidentally breaks livepatching totally, even on GCC builds.

Were this to happen, it would be yet another 4.13 regression.

This is a change to fix a concrete livepatch issue with Clang.  Sure -
it resolves the symbol uniqueness failures for the in-tree build, but
considering the risks to the area you are modifying, the fact you
haven't even done a dev test of a livepatch build on GCC means that the
patch as a whole has not had what I would consider a reasonable amount
of testing.

Luckily for you, Ross and Sergey have agreed to smoke test this with
some livepatches.  They will report on this thread with their findings.

~Andrew

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

Re: [Xen-devel] Ping: [PATCH v2] build: provide option to disambiguate symbol names

[Jan Beulich]

On 27.11.2019 19:17, Andrew Cooper wrote:
> On 21/11/2019 08:34, Jan Beulich wrote:
>> On 20.11.2019 18:13, Andrew Cooper wrote:
>>> On 20/11/2019 16:40, Jürgen Groß wrote:
>>>> On 20.11.19 17:30, Jan Beulich wrote:
>>>>> On 08.11.2019 12:18, Jan Beulich wrote:
>>>>>> The .file assembler directives generated by the compiler do not include
>>>>>> any path components (gcc) or just the ones specified on the command
>>>>>> line
>>>>>> (clang, at least version 5), and hence multiple identically named
>>>>>> source
>>>>>> files (in different directories) may produce identically named static
>>>>>> symbols (in their kallsyms representation). The binary diffing
>>>>>> algorithm
>>>>>> used by xen-livepatch, however, depends on having unique symbols.
>>>>>>
>>>>>> Make the ENFORCE_UNIQUE_SYMBOLS Kconfig option control the (build)
>>>>>> behavior, and if enabled use objcopy to prepend the (relative to the
>>>>>> xen/ subdirectory) path to the compiler invoked STT_FILE symbols. Note
>>>>>> that this build option is made no longer depend on LIVEPATCH, but
>>>>>> merely
>>>>>> defaults to its setting now.
>>>>>>
>>>>>> Conditionalize explicit .file directive insertion in C files where it
>>>>>> exists just to disambiguate names in a less generic manner; note that
>>>>>> at the same time the redundant emission of STT_FILE symbols gets
>>>>>> suppressed for clang. Assembler files as well as multiply compiled C
>>>>>> ones using __OBJECT_FILE__ are left alone for the time being.
>>>>>>
>>>>>> Since we now expect there not to be any duplicates anymore, also don't
>>>>>> force the selection of the option to 'n' anymore in allrandom.config.
>>>>>> Similarly COVERAGE no longer suppresses duplicate symbol warnings if
>>>>>> enforcement is in effect, which in turn allows
>>>>>> SUPPRESS_DUPLICATE_SYMBOL_WARNINGS to simply depend on
>>>>>> !ENFORCE_UNIQUE_SYMBOLS.
>>>>>>
>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>> I've got acks from Konrad and Wei, but still need an x86 and a release
>>>>> one here. Andrew? Or alternatively - Jürgen, would you rather not see
>>>>> this go in anymore?
>>>> In case the needed x86 Ack is coming in before RC3 I'm fine to give my
>>>> Release-ack, but I'm hesitant to take it later.
>>> Has anyone actually tried building a livepatch with this change in place?
>> Actually - what is your concern here? The exact spelling of symbols
>> names should be of no interest to the tool. After all the compiler is
>> free to invent all sorts of names for its local symbols, including
>> the ones we would produce with this change in place. All the tool
>> cares about is that the names be unambiguous. Hence any (theoretical)
>> regression here would be a bug in the tools, which imo is no reason
>> to delay this change any further. (Granted I should have got to it
>> earlier, but it had been continuing to get deferred.)
> 
> This might all be true (theoretically).
> 
> The livepatch build tools are fragile and very sensitive to how the
> object files are laid out.  There is a very real risk that this change
> accidentally breaks livepatching totally, even on GCC builds.
> 
> Were this to happen, it would be yet another 4.13 regression.

It's perhaps a matter of perception, but I'd still call this a
live patching tools bug then, not a 4.13 regression.

> This is a change to fix a concrete livepatch issue with Clang.  Sure -
> it resolves the symbol uniqueness failures for the in-tree build, but
> considering the risks to the area you are modifying, the fact you
> haven't even done a dev test of a livepatch build on GCC means that the
> patch as a whole has not had what I would consider a reasonable amount
> of testing.

While Clang is the primary area where we need this change, it is
in no way limited to that environment. Gcc can, at any time,
start triggering the issue again as well - both because of changes
to the compiler itself, or because of (perhaps seemingly innocent)
changes we do to Xen. I can't imagine the workaround for this
would be to make it impossible altogether to select LIVEPATCH=y.
(In fact some of the recent always_inline may want dropping again
as well with this change in place, when they were added for
symbol collision reasons rather than code generation ones.)

> Luckily for you, Ross and Sergey have agreed to smoke test this with
> some livepatches.  They will report on this thread with their findings.

I appreciate this. And I do not consider it my responsibility to
do regression tests of the live patching tools. If they're so
extremely fragile, then I think this needs urgently taking care of
by their maintainers. As mentioned before - how exactly static
symbols get represented is up to the compiler, i.e. may change at
any time. As a result, any change whatsoever would need such
regression testing, no matter that I agree that a larger scope
change like this one has slightly higher potential of triggering
some issue.

Jan

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

Re: [Xen-devel] Ping: [PATCH v2] build: provide option to disambiguate symbol names

[George Dunlap]

> On Nov 28, 2019, at 9:55 AM, Jan Beulich <jbeulich@suse.com> wrote:
>>>> Has anyone actually tried building a livepatch with this change in place?
>>> Actually - what is your concern here? The exact spelling of symbols
>>> names should be of no interest to the tool. After all the compiler is
>>> free to invent all sorts of names for its local symbols, including
>>> the ones we would produce with this change in place. All the tool
>>> cares about is that the names be unambiguous. Hence any (theoretical)
>>> regression here would be a bug in the tools, which imo is no reason
>>> to delay this change any further. (Granted I should have got to it
>>> earlier, but it had been continuing to get deferred.)
>> 
>> This might all be true (theoretically).
>> 
>> The livepatch build tools are fragile and very sensitive to how the
>> object files are laid out.  There is a very real risk that this change
>> accidentally breaks livepatching totally, even on GCC builds.
>> 
>> Were this to happen, it would be yet another 4.13 regression.
> 
> It's perhaps a matter of perception, but I'd still call this a
> live patching tools bug then, not a 4.13 regression.

After the discussion yesterday, I was thinking a bit more about this, and I’m not happy with the principle Andy seems to be operating on, that it’s reasonable to completely block a bug-fixing patch to Xen, forcing a work-around to be used in a release, because it has unknown effects on livepatching.

Consider the relationship between Xen and Linux, for example.  Suppose that a patch were posted to Linux to fix an issue, and Juergen responded by saying that he wasn’t happy with it because it  might possibly break things running under Xen.  But he didn’t actually test it himself, nor propose some alternate way of fixing the original problem; rather, he expected the original patch submitter, who doesn’t use Xen, to set up a Xen system and test it themselves before accepting it.

Do you think anyone in the kernel community would stand for that?  Of course not.  Naturally the patch would be *paused* while *people in the Xen community* tested and or proposed alternate solutions; but if there was a delay, eventually it would be checked in.

I think the same principle should apply here.  If people using the livepatch code are afraid that Jan’s patch *may* affect livepatching on gcc, then they should be given time to review, test, and/or propose alternate solutions.  But it should be the responsibility of people working on that code, not the responsibility of developers who don’t use that code.

>  If they're so
> extremely fragile, then I think this needs urgently taking care of
> by their maintainers. As mentioned before - how exactly static
> symbols get represented is up to the compiler, i.e. may change at
> any time. As a result, any change whatsoever would need such
> regression testing, no matter that I agree that a larger scope
> change like this one has slightly higher potential of triggering
> some issue.

This is another argument I would agree with.

Given the closeness to the release, I’d favor checking in the patch today or tomorrow, regardless of testing status, so that it can get more testing in our automated systems; it can always be reverted if it is shown to break livepatching on gcc.

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

Re: [Xen-devel] Ping: [PATCH v2] build: provide option to disambiguate symbol names

[Jürgen Groß]

On 28.11.19 11:17, George Dunlap wrote:
> 
>> On Nov 28, 2019, at 9:55 AM, Jan Beulich <jbeulich@suse.com> wrote:
>>>>> Has anyone actually tried building a livepatch with this change in place?
>>>> Actually - what is your concern here? The exact spelling of symbols
>>>> names should be of no interest to the tool. After all the compiler is
>>>> free to invent all sorts of names for its local symbols, including
>>>> the ones we would produce with this change in place. All the tool
>>>> cares about is that the names be unambiguous. Hence any (theoretical)
>>>> regression here would be a bug in the tools, which imo is no reason
>>>> to delay this change any further. (Granted I should have got to it
>>>> earlier, but it had been continuing to get deferred.)
>>>
>>> This might all be true (theoretically).
>>>
>>> The livepatch build tools are fragile and very sensitive to how the
>>> object files are laid out.  There is a very real risk that this change
>>> accidentally breaks livepatching totally, even on GCC builds.
>>>
>>> Were this to happen, it would be yet another 4.13 regression.
>>
>> It's perhaps a matter of perception, but I'd still call this a
>> live patching tools bug then, not a 4.13 regression.
> 
> After the discussion yesterday, I was thinking a bit more about this, and I’m not happy with the principle Andy seems to be operating on, that it’s reasonable to completely block a bug-fixing patch to Xen, forcing a work-around to be used in a release, because it has unknown effects on livepatching.
> 
> Consider the relationship between Xen and Linux, for example.  Suppose that a patch were posted to Linux to fix an issue, and Juergen responded by saying that he wasn’t happy with it because it  might possibly break things running under Xen.  But he didn’t actually test it himself, nor propose some alternate way of fixing the original problem; rather, he expected the original patch submitter, who doesn’t use Xen, to set up a Xen system and test it themselves before accepting it.
> 
> Do you think anyone in the kernel community would stand for that?  Of course not.  Naturally the patch would be *paused* while *people in the Xen community* tested and or proposed alternate solutions; but if there was a delay, eventually it would be checked in.
> 
> I think the same principle should apply here.  If people using the livepatch code are afraid that Jan’s patch *may* affect livepatching on gcc, then they should be given time to review, test, and/or propose alternate solutions.  But it should be the responsibility of people working on that code, not the responsibility of developers who don’t use that code.
> 
>>   If they're so
>> extremely fragile, then I think this needs urgently taking care of
>> by their maintainers. As mentioned before - how exactly static
>> symbols get represented is up to the compiler, i.e. may change at
>> any time. As a result, any change whatsoever would need such
>> regression testing, no matter that I agree that a larger scope
>> change like this one has slightly higher potential of triggering
>> some issue.
> 
> This is another argument I would agree with.
> 
> Given the closeness to the release, I’d favor checking in the patch today or tomorrow, regardless of testing status, so that it can get more testing in our automated systems; it can always be reverted if it is shown to break livepatching on gcc.

In that case: please rather today than tomorrow. The earlier the
better.

Juergen


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

Re: [Xen-devel] Ping: [PATCH v2] build: provide option to disambiguate symbol names

[Sergey Dyasli]

On 27/11/2019 18:17, Andrew Cooper wrote:
> On 21/11/2019 08:34, Jan Beulich wrote:
>> On 20.11.2019 18:13, Andrew Cooper wrote:
>>> On 20/11/2019 16:40, Jürgen Groß wrote:
>>>> On 20.11.19 17:30, Jan Beulich wrote:
>>>>> On 08.11.2019 12:18, Jan Beulich wrote:
>>>>>> The .file assembler directives generated by the compiler do not include
>>>>>> any path components (gcc) or just the ones specified on the command
>>>>>> line
>>>>>> (clang, at least version 5), and hence multiple identically named
>>>>>> source
>>>>>> files (in different directories) may produce identically named static
>>>>>> symbols (in their kallsyms representation). The binary diffing
>>>>>> algorithm
>>>>>> used by xen-livepatch, however, depends on having unique symbols.
>>>>>>
>>>>>> Make the ENFORCE_UNIQUE_SYMBOLS Kconfig option control the (build)
>>>>>> behavior, and if enabled use objcopy to prepend the (relative to the
>>>>>> xen/ subdirectory) path to the compiler invoked STT_FILE symbols. Note
>>>>>> that this build option is made no longer depend on LIVEPATCH, but
>>>>>> merely
>>>>>> defaults to its setting now.
>>>>>>
>>>>>> Conditionalize explicit .file directive insertion in C files where it
>>>>>> exists just to disambiguate names in a less generic manner; note that
>>>>>> at the same time the redundant emission of STT_FILE symbols gets
>>>>>> suppressed for clang. Assembler files as well as multiply compiled C
>>>>>> ones using __OBJECT_FILE__ are left alone for the time being.
>>>>>>
>>>>>> Since we now expect there not to be any duplicates anymore, also don't
>>>>>> force the selection of the option to 'n' anymore in allrandom.config.
>>>>>> Similarly COVERAGE no longer suppresses duplicate symbol warnings if
>>>>>> enforcement is in effect, which in turn allows
>>>>>> SUPPRESS_DUPLICATE_SYMBOL_WARNINGS to simply depend on
>>>>>> !ENFORCE_UNIQUE_SYMBOLS.
>>>>>>
>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>> I've got acks from Konrad and Wei, but still need an x86 and a release
>>>>> one here. Andrew? Or alternatively - Jürgen, would you rather not see
>>>>> this go in anymore?
>>>> In case the needed x86 Ack is coming in before RC3 I'm fine to give my
>>>> Release-ack, but I'm hesitant to take it later.
>>> Has anyone actually tried building a livepatch with this change in place?
>> Actually - what is your concern here? The exact spelling of symbols
>> names should be of no interest to the tool. After all the compiler is
>> free to invent all sorts of names for its local symbols, including
>> the ones we would produce with this change in place. All the tool
>> cares about is that the names be unambiguous. Hence any (theoretical)
>> regression here would be a bug in the tools, which imo is no reason
>> to delay this change any further. (Granted I should have got to it
>> earlier, but it had been continuing to get deferred.)
> 
> This might all be true (theoretically).
> 
> The livepatch build tools are fragile and very sensitive to how the
> object files are laid out.  There is a very real risk that this change
> accidentally breaks livepatching totally, even on GCC builds.
> 
> Were this to happen, it would be yet another 4.13 regression.
> 
> This is a change to fix a concrete livepatch issue with Clang.  Sure -
> it resolves the symbol uniqueness failures for the in-tree build, but
> considering the risks to the area you are modifying, the fact you
> haven't even done a dev test of a livepatch build on GCC means that the
> patch as a whole has not had what I would consider a reasonable amount
> of testing.
> 
> Luckily for you, Ross and Sergey have agreed to smoke test this with
> some livepatches.  They will report on this thread with their findings.

Applying the patch didn't end up well for my test LP (from another thread):

Perform full initial build with 8 CPU(s)...
Reading special section data
Apply patch and build with 8 CPU(s)...
Unapply patch and build with 8 CPU(s)...
Extracting new and modified ELF sections...
Processing xen/arch/x86/mm/shadow/guest_2.o
Processing xen/arch/x86/mm/shadow/guest_4.o
Processing xen/arch/x86/mm/shadow/guest_3.o
Processing xen/arch/x86/mm/guest_walk_3.o
Processing xen/arch/x86/mm/hap/guest_walk_3level.o
Processing xen/arch/x86/mm/hap/guest_walk_4level.o
Processing xen/arch/x86/mm/hap/guest_walk_2level.o
Processing xen/arch/x86/mm/guest_walk_2.o
Processing xen/arch/x86/mm/guest_walk_4.o
Processing xen/arch/x86/efi/efi/check.o
Processing xen/arch/x86/pv/gpr_switch.o
Processing xen/arch/x86/indirect-thunk.o
Processing xen/arch/x86/boot/head.o
Processing xen/arch/x86/x86_64/kexec_reloc.o
Processing xen/arch/x86/x86_64/compat/entry.o
Processing xen/arch/x86/x86_64/entry.o
Processing xen/arch/x86/hvm/vmx/entry.o
Processing xen/arch/x86/hvm/svm/entry.o
Processing xen/arch/x86/mnt/media/git/upstream/xen/xen/mnt/media/git/upstream/xen/xen/.xen.efi.0s.o
Processing xen/arch/x86/mnt/media/git/upstream/xen/xen/mnt/media/git/upstream/xen/xen/.xen.efi.0r.o
Processing xen/arch/x86/mnt/media/git/upstream/xen/xen/mnt/media/git/upstream/xen/xen/.xen.efi.1s.o
Processing xen/arch/x86/mnt/media/git/upstream/xen/xen/mnt/media/git/upstream/xen/xen/.xen.efi.1r.o
ERROR: no functional changes found.

So this looks like a regression.

--
Thanks,
Sergey


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

Re: [Xen-devel] Ping: [PATCH v2] build: provide option to disambiguate symbol names

[Andrew Cooper]

On 28/11/2019 10:17, George Dunlap wrote:
>> On Nov 28, 2019, at 9:55 AM, Jan Beulich <jbeulich@suse.com> wrote:
>>>>> Has anyone actually tried building a livepatch with this change in place?
>>>> Actually - what is your concern here? The exact spelling of symbols
>>>> names should be of no interest to the tool. After all the compiler is
>>>> free to invent all sorts of names for its local symbols, including
>>>> the ones we would produce with this change in place. All the tool
>>>> cares about is that the names be unambiguous. Hence any (theoretical)
>>>> regression here would be a bug in the tools, which imo is no reason
>>>> to delay this change any further. (Granted I should have got to it
>>>> earlier, but it had been continuing to get deferred.)
>>> This might all be true (theoretically).
>>>
>>> The livepatch build tools are fragile and very sensitive to how the
>>> object files are laid out.  There is a very real risk that this change
>>> accidentally breaks livepatching totally, even on GCC builds.
>>>
>>> Were this to happen, it would be yet another 4.13 regression.
>> It's perhaps a matter of perception, but I'd still call this a
>> live patching tools bug then, not a 4.13 regression.
> After the discussion yesterday, I was thinking a bit more about this, and I’m not happy with the principle Andy seems to be operating on,

I'm sorry that you feel that way.

> that it’s reasonable to completely block a bug-fixing patch to Xen, forcing a work-around to be used in a release, because it has unknown effects on livepatching.

This is not a fair characterisation of what is going on here.  Ignore
the specifics of this patch - they are not relevant to my objection.

As a maintainer, it is my responsibility to ensure that crap doesn't get
committed.

As a consequence, it is up to me to judge whether I believe that the
submitter of a patch has provided adequate thought/testing to what they
are changing.  Mostly this is judged on comments provided (or usually,
their absence), weighed up against the risk of what it might be likely
to break.

In the case that I don't believe adequate through/testing/etc has been
done, I'm not going to ack the patch.  I'd be failing as a maintainer if
I did.

Ergo, I am not inclined to change my position.


In this case, all I asked was "has anyone done a livepatch build?"

I'd be entirely happy with a reply of "yes [I or someone else did] and
it seems ok".

I'd even be happy with "There does seem to be an issue with
livepatching, but I've engaged the relevant people in this other thread".

What I'm not happy with is "I haven't even done a single build to see
whether it might have problems", and what is definitely not acceptable
is, and I quote:

> And I do not consider it my responsibility to
> do regression tests of the live patching tools.

Yes.  Yes it really is, when you're making a material change
specifically to this area, with a high chance of adverse impact.

I don't expect you necessarily to fix the issue, but I do expect you to
have some idea of whether you're trading one 4.13 blocker for a
different one.

>>  If they're so
>> extremely fragile, then I think this needs urgently taking care of
>> by their maintainers. As mentioned before - how exactly static
>> symbols get represented is up to the compiler, i.e. may change at
>> any time. As a result, any change whatsoever would need such
>> regression testing, no matter that I agree that a larger scope
>> change like this one has slightly higher potential of triggering
>> some issue.
> This is another argument I would agree with.
>
> Given the closeness to the release, I’d favor checking in the patch today or tomorrow, regardless of testing status, so that it can get more testing in our automated systems; it can always be reverted if it is shown to break livepatching on gcc.

Oh, and shocker in what is apparently a surprise to everyone but me...

Sergey went and did the work Jan believed to be "not his
responsibility", and yes - this really does break livepatching.

Do I expect Jan to fix it?  No, but I do expect a discussion and an
understanding of the issue before this patch gets anywhere near staging.

~Andrew, quite irritated at the total lack of due diligence being
demonstrated here.

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

Re: [Xen-devel] Ping: [PATCH v2] build: provide option to disambiguate symbol names

[Jan Beulich]

On 28.11.2019 14:10, Andrew Cooper wrote:
> On 28/11/2019 10:17, George Dunlap wrote:
>>> On Nov 28, 2019, at 9:55 AM, Jan Beulich <jbeulich@suse.com> wrote:
>>>>>> Has anyone actually tried building a livepatch with this change in place?
>>>>> Actually - what is your concern here? The exact spelling of symbols
>>>>> names should be of no interest to the tool. After all the compiler is
>>>>> free to invent all sorts of names for its local symbols, including
>>>>> the ones we would produce with this change in place. All the tool
>>>>> cares about is that the names be unambiguous. Hence any (theoretical)
>>>>> regression here would be a bug in the tools, which imo is no reason
>>>>> to delay this change any further. (Granted I should have got to it
>>>>> earlier, but it had been continuing to get deferred.)
>>>> This might all be true (theoretically).
>>>>
>>>> The livepatch build tools are fragile and very sensitive to how the
>>>> object files are laid out.  There is a very real risk that this change
>>>> accidentally breaks livepatching totally, even on GCC builds.
>>>>
>>>> Were this to happen, it would be yet another 4.13 regression.
>>> It's perhaps a matter of perception, but I'd still call this a
>>> live patching tools bug then, not a 4.13 regression.
>> After the discussion yesterday, I was thinking a bit more about
> this, and I’m not happy with the principle Andy seems to be operating on,
> 
> I'm sorry that you feel that way.
> 
>> that it’s reasonable to completely block a bug-fixing patch to
> Xen, forcing a work-around to be used in a release, because it
> has unknown effects on livepatching.
> 
> This is not a fair characterisation of what is going on here.  Ignore
> the specifics of this patch - they are not relevant to my objection.
> 
> As a maintainer, it is my responsibility to ensure that crap doesn't get
> committed.

While above you say to ignore the specifics of this patch, I'm
afraid I still take "crap" as a qualification of the submitted
patch (and possibly other parts of my work). Perhaps I should
go look for another job if it's like this.

> As a consequence, it is up to me to judge whether I believe that the
> submitter of a patch has provided adequate thought/testing to what they
> are changing.  Mostly this is judged on comments provided (or usually,
> their absence), weighed up against the risk of what it might be likely
> to break.
> 
> In the case that I don't believe adequate through/testing/etc has been
> done, I'm not going to ack the patch.  I'd be failing as a maintainer if
> I did.
> 
> Ergo, I am not inclined to change my position.
> 
> 
> In this case, all I asked was "has anyone done a livepatch build?"
> 
> I'd be entirely happy with a reply of "yes [I or someone else did] and
> it seems ok".
> 
> I'd even be happy with "There does seem to be an issue with
> livepatching, but I've engaged the relevant people in this other thread".
> 
> What I'm not happy with is "I haven't even done a single build to see
> whether it might have problems", and what is definitely not acceptable
> is, and I quote:
> 
>> And I do not consider it my responsibility to
>> do regression tests of the live patching tools.
> 
> Yes.  Yes it really is, when you're making a material change
> specifically to this area, with a high chance of adverse impact.
> 
> I don't expect you necessarily to fix the issue, but I do expect you to
> have some idea of whether you're trading one 4.13 blocker for a
> different one.

Once again - no. If there is a problem with this patch, then
please point it out by review comments. If there is a problem
with the live patching tools, it is there where things need
fixing. Just to remind you of what I'm hearing/seeing you and
others say/do: Quite often issues with the tools arise when
dealing with particular XSAs. That's still no reason to
delay/reject the fixes for those XSAs.

And once again - please accept that views if different people
may vary.

>>>  If they're so
>>> extremely fragile, then I think this needs urgently taking care of
>>> by their maintainers. As mentioned before - how exactly static
>>> symbols get represented is up to the compiler, i.e. may change at
>>> any time. As a result, any change whatsoever would need such
>>> regression testing, no matter that I agree that a larger scope
>>> change like this one has slightly higher potential of triggering
>>> some issue.
>> This is another argument I would agree with.
>>
>> Given the closeness to the release, I’d favor checking in the
> patch today or tomorrow, regardless of testing status, so that
> it can get more testing in our automated systems; it can always
> be reverted if it is shown to break livepatching on gcc.
> 
> Oh, and shocker in what is apparently a surprise to everyone but me...
> 
> Sergey went and did the work Jan believed to be "not his
> responsibility", and yes - this really does break livepatching.
> 
> Do I expect Jan to fix it?  No, but I do expect a discussion and an
> understanding of the issue before this patch gets anywhere near staging.

Which I'd have been fine with if issues _the patch_ causes had
been pointed out in the form of review comments. But as in so
many other cases, the patch was just left hanging in the air.
Despite Sergey's reply I'm none the wiser what may be wrong, or
where. But I'll reply to that effect there, to keep technical
things separated from this unfortunate discussion.

Jan

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

Re: [Xen-devel] Ping: [PATCH v2] build: provide option to disambiguate symbol names

[Jan Beulich]

On 28.11.2019 13:15, Sergey Dyasli wrote:
> Applying the patch didn't end up well for my test LP (from another thread):
> 
> Perform full initial build with 8 CPU(s)...
> Reading special section data
> Apply patch and build with 8 CPU(s)...
> Unapply patch and build with 8 CPU(s)...
> Extracting new and modified ELF sections...
> Processing xen/arch/x86/mm/shadow/guest_2.o
> Processing xen/arch/x86/mm/shadow/guest_4.o
> Processing xen/arch/x86/mm/shadow/guest_3.o
> Processing xen/arch/x86/mm/guest_walk_3.o
> Processing xen/arch/x86/mm/hap/guest_walk_3level.o
> Processing xen/arch/x86/mm/hap/guest_walk_4level.o
> Processing xen/arch/x86/mm/hap/guest_walk_2level.o
> Processing xen/arch/x86/mm/guest_walk_2.o
> Processing xen/arch/x86/mm/guest_walk_4.o
> Processing xen/arch/x86/efi/efi/check.o
> Processing xen/arch/x86/pv/gpr_switch.o
> Processing xen/arch/x86/indirect-thunk.o
> Processing xen/arch/x86/boot/head.o
> Processing xen/arch/x86/x86_64/kexec_reloc.o
> Processing xen/arch/x86/x86_64/compat/entry.o
> Processing xen/arch/x86/x86_64/entry.o
> Processing xen/arch/x86/hvm/vmx/entry.o
> Processing xen/arch/x86/hvm/svm/entry.o
> Processing xen/arch/x86/mnt/media/git/upstream/xen/xen/mnt/media/git/upstream/xen/xen/.xen.efi.0s.o
> Processing xen/arch/x86/mnt/media/git/upstream/xen/xen/mnt/media/git/upstream/xen/xen/.xen.efi.0r.o
> Processing xen/arch/x86/mnt/media/git/upstream/xen/xen/mnt/media/git/upstream/xen/xen/.xen.efi.1s.o
> Processing xen/arch/x86/mnt/media/git/upstream/xen/xen/mnt/media/git/upstream/xen/xen/.xen.efi.1r.o
> ERROR: no functional changes found.
> 
> So this looks like a regression.

Thanks for doing the testing. But what am I to conclude from
the above? I can't even tell why "no functional changes found"
is an error.

Jan

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

Re: [Xen-devel] Ping: [PATCH v2] build: provide option to disambiguate symbol names

[Ross Lagerwall]

On 11/28/19 1:49 PM, Jan Beulich wrote:
> On 28.11.2019 13:15, Sergey Dyasli wrote:
>> Applying the patch didn't end up well for my test LP (from another thread):
>>
>> Perform full initial build with 8 CPU(s)...
>> Reading special section data
>> Apply patch and build with 8 CPU(s)...
>> Unapply patch and build with 8 CPU(s)...
>> Extracting new and modified ELF sections...
>> Processing xen/arch/x86/mm/shadow/guest_2.o
>> Processing xen/arch/x86/mm/shadow/guest_4.o
>> Processing xen/arch/x86/mm/shadow/guest_3.o
>> Processing xen/arch/x86/mm/guest_walk_3.o
>> Processing xen/arch/x86/mm/hap/guest_walk_3level.o
>> Processing xen/arch/x86/mm/hap/guest_walk_4level.o
>> Processing xen/arch/x86/mm/hap/guest_walk_2level.o
>> Processing xen/arch/x86/mm/guest_walk_2.o
>> Processing xen/arch/x86/mm/guest_walk_4.o
>> Processing xen/arch/x86/efi/efi/check.o
>> Processing xen/arch/x86/pv/gpr_switch.o
>> Processing xen/arch/x86/indirect-thunk.o
>> Processing xen/arch/x86/boot/head.o
>> Processing xen/arch/x86/x86_64/kexec_reloc.o
>> Processing xen/arch/x86/x86_64/compat/entry.o
>> Processing xen/arch/x86/x86_64/entry.o
>> Processing xen/arch/x86/hvm/vmx/entry.o
>> Processing xen/arch/x86/hvm/svm/entry.o
>> Processing xen/arch/x86/mnt/media/git/upstream/xen/xen/mnt/media/git/upstream/xen/xen/.xen.efi.0s.o
>> Processing xen/arch/x86/mnt/media/git/upstream/xen/xen/mnt/media/git/upstream/xen/xen/.xen.efi.0r.o
>> Processing xen/arch/x86/mnt/media/git/upstream/xen/xen/mnt/media/git/upstream/xen/xen/.xen.efi.1s.o
>> Processing xen/arch/x86/mnt/media/git/upstream/xen/xen/mnt/media/git/upstream/xen/xen/.xen.efi.1r.o
>> ERROR: no functional changes found.
>>
>> So this looks like a regression.
> 
> Thanks for doing the testing. But what am I to conclude from
> the above? I can't even tell why "no functional changes found"
> is an error.
> 

It's due to the way livepatch-build tool interposes on the build to capture
changed object files for later comparison.  Now that objcopy writes out the
proper object files rather than gcc (which just writes a temporary one), the
livepatch-build tool needs some adjustment otherwise it doesn't capture any
changed files. I'm working on a patch.

Thanks,
-- 
Ross Lagerwall

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

Re: [Xen-devel] Ping: [PATCH v2] build: provide option to disambiguate symbol names

[Jan Beulich]

On 28.11.2019 14:54, Ross Lagerwall wrote:
> On 11/28/19 1:49 PM, Jan Beulich wrote:
>> On 28.11.2019 13:15, Sergey Dyasli wrote:
>>> Applying the patch didn't end up well for my test LP (from another thread):
>>>
>>> Perform full initial build with 8 CPU(s)...
>>> Reading special section data
>>> Apply patch and build with 8 CPU(s)...
>>> Unapply patch and build with 8 CPU(s)...
>>> Extracting new and modified ELF sections...
>>> Processing xen/arch/x86/mm/shadow/guest_2.o
>>> Processing xen/arch/x86/mm/shadow/guest_4.o
>>> Processing xen/arch/x86/mm/shadow/guest_3.o
>>> Processing xen/arch/x86/mm/guest_walk_3.o
>>> Processing xen/arch/x86/mm/hap/guest_walk_3level.o
>>> Processing xen/arch/x86/mm/hap/guest_walk_4level.o
>>> Processing xen/arch/x86/mm/hap/guest_walk_2level.o
>>> Processing xen/arch/x86/mm/guest_walk_2.o
>>> Processing xen/arch/x86/mm/guest_walk_4.o
>>> Processing xen/arch/x86/efi/efi/check.o
>>> Processing xen/arch/x86/pv/gpr_switch.o
>>> Processing xen/arch/x86/indirect-thunk.o
>>> Processing xen/arch/x86/boot/head.o
>>> Processing xen/arch/x86/x86_64/kexec_reloc.o
>>> Processing xen/arch/x86/x86_64/compat/entry.o
>>> Processing xen/arch/x86/x86_64/entry.o
>>> Processing xen/arch/x86/hvm/vmx/entry.o
>>> Processing xen/arch/x86/hvm/svm/entry.o
>>> Processing xen/arch/x86/mnt/media/git/upstream/xen/xen/mnt/media/git/upstream/xen/xen/.xen.efi.0s.o
>>> Processing xen/arch/x86/mnt/media/git/upstream/xen/xen/mnt/media/git/upstream/xen/xen/.xen.efi.0r.o
>>> Processing xen/arch/x86/mnt/media/git/upstream/xen/xen/mnt/media/git/upstream/xen/xen/.xen.efi.1s.o
>>> Processing xen/arch/x86/mnt/media/git/upstream/xen/xen/mnt/media/git/upstream/xen/xen/.xen.efi.1r.o
>>> ERROR: no functional changes found.
>>>
>>> So this looks like a regression.
>>
>> Thanks for doing the testing. But what am I to conclude from
>> the above? I can't even tell why "no functional changes found"
>> is an error.
>>
> 
> It's due to the way livepatch-build tool interposes on the build to capture
> changed object files for later comparison.  Now that objcopy writes out the
> proper object files rather than gcc (which just writes a temporary one), the
> livepatch-build tool needs some adjustment otherwise it doesn't capture any
> changed files. I'm working on a patch.

For my own education, and just if you have the time: Why would there
be any dependency on which build utility produces the object file?

Jan

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

Re: [Xen-devel] Ping: [PATCH v2] build: provide option to disambiguate symbol names

[Jan Beulich]

On 28.11.2019 10:55, Jan Beulich wrote:
> On 27.11.2019 19:17, Andrew Cooper wrote:
>> On 21/11/2019 08:34, Jan Beulich wrote:
>>> On 20.11.2019 18:13, Andrew Cooper wrote:
>>>> On 20/11/2019 16:40, Jürgen Groß wrote:
>>>>> On 20.11.19 17:30, Jan Beulich wrote:
>>>>>> On 08.11.2019 12:18, Jan Beulich wrote:
>>>>>>> The .file assembler directives generated by the compiler do not include
>>>>>>> any path components (gcc) or just the ones specified on the command
>>>>>>> line
>>>>>>> (clang, at least version 5), and hence multiple identically named
>>>>>>> source
>>>>>>> files (in different directories) may produce identically named static
>>>>>>> symbols (in their kallsyms representation). The binary diffing
>>>>>>> algorithm
>>>>>>> used by xen-livepatch, however, depends on having unique symbols.
>>>>>>>
>>>>>>> Make the ENFORCE_UNIQUE_SYMBOLS Kconfig option control the (build)
>>>>>>> behavior, and if enabled use objcopy to prepend the (relative to the
>>>>>>> xen/ subdirectory) path to the compiler invoked STT_FILE symbols. Note
>>>>>>> that this build option is made no longer depend on LIVEPATCH, but
>>>>>>> merely
>>>>>>> defaults to its setting now.
>>>>>>>
>>>>>>> Conditionalize explicit .file directive insertion in C files where it
>>>>>>> exists just to disambiguate names in a less generic manner; note that
>>>>>>> at the same time the redundant emission of STT_FILE symbols gets
>>>>>>> suppressed for clang. Assembler files as well as multiply compiled C
>>>>>>> ones using __OBJECT_FILE__ are left alone for the time being.
>>>>>>>
>>>>>>> Since we now expect there not to be any duplicates anymore, also don't
>>>>>>> force the selection of the option to 'n' anymore in allrandom.config.
>>>>>>> Similarly COVERAGE no longer suppresses duplicate symbol warnings if
>>>>>>> enforcement is in effect, which in turn allows
>>>>>>> SUPPRESS_DUPLICATE_SYMBOL_WARNINGS to simply depend on
>>>>>>> !ENFORCE_UNIQUE_SYMBOLS.
>>>>>>>
>>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>> I've got acks from Konrad and Wei, but still need an x86 and a release
>>>>>> one here. Andrew? Or alternatively - Jürgen, would you rather not see
>>>>>> this go in anymore?
>>>>> In case the needed x86 Ack is coming in before RC3 I'm fine to give my
>>>>> Release-ack, but I'm hesitant to take it later.
>>>> Has anyone actually tried building a livepatch with this change in place?
>>> Actually - what is your concern here? The exact spelling of symbols
>>> names should be of no interest to the tool. After all the compiler is
>>> free to invent all sorts of names for its local symbols, including
>>> the ones we would produce with this change in place. All the tool
>>> cares about is that the names be unambiguous. Hence any (theoretical)
>>> regression here would be a bug in the tools, which imo is no reason
>>> to delay this change any further. (Granted I should have got to it
>>> earlier, but it had been continuing to get deferred.)
>>
>> This might all be true (theoretically).
>>
>> The livepatch build tools are fragile and very sensitive to how the
>> object files are laid out.  There is a very real risk that this change
>> accidentally breaks livepatching totally, even on GCC builds.
>>
>> Were this to happen, it would be yet another 4.13 regression.
> 
> It's perhaps a matter of perception, but I'd still call this a
> live patching tools bug then, not a 4.13 regression.
> 
>> This is a change to fix a concrete livepatch issue with Clang.  Sure -
>> it resolves the symbol uniqueness failures for the in-tree build, but
>> considering the risks to the area you are modifying, the fact you
>> haven't even done a dev test of a livepatch build on GCC means that the
>> patch as a whole has not had what I would consider a reasonable amount
>> of testing.
> 
> While Clang is the primary area where we need this change, it is
> in no way limited to that environment. Gcc can, at any time,
> start triggering the issue again as well - both because of changes
> to the compiler itself, or because of (perhaps seemingly innocent)
> changes we do to Xen. I can't imagine the workaround for this
> would be to make it impossible altogether to select LIVEPATCH=y.

And indeed - on my box with gcc 4.3, a full fresh build fails with
two duplicate symbols. Roger's workaround therefore is not enough
in any event for 4.13, unless we want to - in the last minute -
raise the bar of what gcc versions we claim compatibility with.

Jan

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

Re: [Xen-devel] Ping: [PATCH v2] build: provide option to disambiguate symbol names

[George Dunlap]

On 11/28/19 1:10 PM, Andrew Cooper wrote:
> On 28/11/2019 10:17, George Dunlap wrote:
>>> On Nov 28, 2019, at 9:55 AM, Jan Beulich <jbeulich@suse.com> wrote:
>>>>>> Has anyone actually tried building a livepatch with this change in place?
>>>>> Actually - what is your concern here? The exact spelling of symbols
>>>>> names should be of no interest to the tool. After all the compiler is
>>>>> free to invent all sorts of names for its local symbols, including
>>>>> the ones we would produce with this change in place. All the tool
>>>>> cares about is that the names be unambiguous. Hence any (theoretical)
>>>>> regression here would be a bug in the tools, which imo is no reason
>>>>> to delay this change any further. (Granted I should have got to it
>>>>> earlier, but it had been continuing to get deferred.)
>>>> This might all be true (theoretically).
>>>>
>>>> The livepatch build tools are fragile and very sensitive to how the
>>>> object files are laid out.  There is a very real risk that this change
>>>> accidentally breaks livepatching totally, even on GCC builds.
>>>>
>>>> Were this to happen, it would be yet another 4.13 regression.
>>> It's perhaps a matter of perception, but I'd still call this a
>>> live patching tools bug then, not a 4.13 regression.
>> After the discussion yesterday, I was thinking a bit more about this, and I’m not happy with the principle Andy seems to be operating on,
> 
> I'm sorry that you feel that way.
> 
>> that it’s reasonable to completely block a bug-fixing patch to Xen, forcing a work-around to be used in a release, because it has unknown effects on livepatching.
> 
> This is not a fair characterisation of what is going on here.  Ignore
> the specifics of this patch - they are not relevant to my objection.
> 
> As a maintainer, it is my responsibility to ensure that crap doesn't get
> committed.

Jan's patch is not crap; this is out of line.

>> And I do not consider it my responsibility to
>> do regression tests of the live patching tools.
> 
> Yes.  Yes it really is, when you're making a material change
> specifically to this area, with a high chance of adverse impact.

Then it looks like we need to have a wider discussion about this,
because my answer is, "No, not it's really not."

And I don't think you would think so either, except that you happen to
use livepatching.  Imagine if you posted a patch trying to fix nested
HVM, and out of the blue Olaf turned up and said, "Have you tested this
with hypervisor paging?"  And when you said, "No, I have no idea how to
test that", Jan simply blocked the patch indefinitely?  You would be
angry, and rightly so.

As a general principle, the cost of features should be borne by the
people who use them.  That includes the cost of determining
authoritatively whether a change is safe or not -- either by review, or
by doing manual testing, or by having automated tests in place to flag
up issues.

If you had questions about the patch, *you*, with your developer hat on,
should have either done testing, or arranged for someone who uses it
regularly to do testing to make sure it works.

 -George

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

Re: [Xen-devel] Ping: [PATCH v2] build: provide option to disambiguate symbol names

[Ross Lagerwall]

On 11/28/19 1:57 PM, Jan Beulich wrote:
> On 28.11.2019 14:54, Ross Lagerwall wrote:
>> On 11/28/19 1:49 PM, Jan Beulich wrote:
>>> On 28.11.2019 13:15, Sergey Dyasli wrote:
>>>> Applying the patch didn't end up well for my test LP (from another thread):
>>>>
>>>> Perform full initial build with 8 CPU(s)...
>>>> Reading special section data
>>>> Apply patch and build with 8 CPU(s)...
>>>> Unapply patch and build with 8 CPU(s)...
>>>> Extracting new and modified ELF sections...
>>>> Processing xen/arch/x86/mm/shadow/guest_2.o
>>>> Processing xen/arch/x86/mm/shadow/guest_4.o
>>>> Processing xen/arch/x86/mm/shadow/guest_3.o
>>>> Processing xen/arch/x86/mm/guest_walk_3.o
>>>> Processing xen/arch/x86/mm/hap/guest_walk_3level.o
>>>> Processing xen/arch/x86/mm/hap/guest_walk_4level.o
>>>> Processing xen/arch/x86/mm/hap/guest_walk_2level.o
>>>> Processing xen/arch/x86/mm/guest_walk_2.o
>>>> Processing xen/arch/x86/mm/guest_walk_4.o
>>>> Processing xen/arch/x86/efi/efi/check.o
>>>> Processing xen/arch/x86/pv/gpr_switch.o
>>>> Processing xen/arch/x86/indirect-thunk.o
>>>> Processing xen/arch/x86/boot/head.o
>>>> Processing xen/arch/x86/x86_64/kexec_reloc.o
>>>> Processing xen/arch/x86/x86_64/compat/entry.o
>>>> Processing xen/arch/x86/x86_64/entry.o
>>>> Processing xen/arch/x86/hvm/vmx/entry.o
>>>> Processing xen/arch/x86/hvm/svm/entry.o
>>>> Processing xen/arch/x86/mnt/media/git/upstream/xen/xen/mnt/media/git/upstream/xen/xen/.xen.efi.0s.o
>>>> Processing xen/arch/x86/mnt/media/git/upstream/xen/xen/mnt/media/git/upstream/xen/xen/.xen.efi.0r.o
>>>> Processing xen/arch/x86/mnt/media/git/upstream/xen/xen/mnt/media/git/upstream/xen/xen/.xen.efi.1s.o
>>>> Processing xen/arch/x86/mnt/media/git/upstream/xen/xen/mnt/media/git/upstream/xen/xen/.xen.efi.1r.o
>>>> ERROR: no functional changes found.
>>>>
>>>> So this looks like a regression.
>>>
>>> Thanks for doing the testing. But what am I to conclude from
>>> the above? I can't even tell why "no functional changes found"
>>> is an error.
>>>
>>
>> It's due to the way livepatch-build tool interposes on the build to capture
>> changed object files for later comparison.  Now that objcopy writes out the
>> proper object files rather than gcc (which just writes a temporary one), the
>> livepatch-build tool needs some adjustment otherwise it doesn't capture any
>> changed files. I'm working on a patch.
> 
> For my own education, and just if you have the time: Why would there
> be any dependency on which build utility produces the object file?
> 

It uses CROSS_COMPILE to funnel all build output to a script
(https://xenbits.xen.org/gitweb/?p=livepatch-build-tools.git;a=blob;f=livepatch-gcc)
which then notes changed object files by processing gcc's command-line.

If objcopy is used instead of gcc to produce the final output then the script
processes gcc's command-line but doesn't get the output it expects so no
changes are detected.

Yes, this is hacky -- improvements are welcome!

-- 
Ross Lagerwall

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

Re: [Xen-devel] Ping: [PATCH v2] build: provide option to disambiguate symbol names

[Wei Liu]

On Thu, Nov 28, 2019 at 01:10:05PM +0000, Andrew Cooper wrote:
> On 28/11/2019 10:17, George Dunlap wrote:
> >> On Nov 28, 2019, at 9:55 AM, Jan Beulich <jbeulich@suse.com> wrote:
> >>>>> Has anyone actually tried building a livepatch with this change in place?
> >>>> Actually - what is your concern here? The exact spelling of symbols
> >>>> names should be of no interest to the tool. After all the compiler is
> >>>> free to invent all sorts of names for its local symbols, including
> >>>> the ones we would produce with this change in place. All the tool
> >>>> cares about is that the names be unambiguous. Hence any (theoretical)
> >>>> regression here would be a bug in the tools, which imo is no reason
> >>>> to delay this change any further. (Granted I should have got to it
> >>>> earlier, but it had been continuing to get deferred.)
> >>> This might all be true (theoretically).
> >>>
> >>> The livepatch build tools are fragile and very sensitive to how the
> >>> object files are laid out.  There is a very real risk that this change
> >>> accidentally breaks livepatching totally, even on GCC builds.
> >>>
> >>> Were this to happen, it would be yet another 4.13 regression.
> >> It's perhaps a matter of perception, but I'd still call this a
> >> live patching tools bug then, not a 4.13 regression.
> > After the discussion yesterday, I was thinking a bit more about this, and I’m not happy with the principle Andy seems to be operating on,
> 
> I'm sorry that you feel that way.
> 
> > that it’s reasonable to completely block a bug-fixing patch to Xen, forcing a work-around to be used in a release, because it has unknown effects on livepatching.
> 
> This is not a fair characterisation of what is going on here.  Ignore
> the specifics of this patch - they are not relevant to my objection.
> 
> As a maintainer, it is my responsibility to ensure that crap doesn't get
> committed.
> 

It is fine to have differing opinions; it is not fine to make an
emotionally charged comment like this. It serves no-one's interest.

Wei.

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