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

[Xen-devel] [PATCH] 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.

Provide a Kconfig option to 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.

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.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Kconfig change taken from "[PATCH v3 5/7] x86/livepatch: Fail the build
if duplicate symbols exist". When re-basing onto that other patch I
think we will also want to drop that other patch'es adjustment to
allrandom.config again.

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

--- 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
@@ -338,9 +338,23 @@ config FAST_SYMBOL_LOOKUP
 
 	  If unsure, say Y.
 
+config ENFORCE_UNIQUE_SYMBOLS
+	bool "Enforce unique symbols"
+	default LIVEPATCH
+	---help---
+	  Multiple symbols with the same name aren't generally a problem
+	  unless Live patching is to be used.
+
+	  Livepatch loading involves resolving relocations against symbol
+	  names, and attempting to a duplicate symbol in a livepatch will
+	  result in incorrect livepatch application.
+
+	  This option should be used to ensure that a build of Xen can have a
+	  livepatch build and apply correctly.
+
 config SUPPRESS_DUPLICATE_SYMBOL_WARNINGS
-	bool "Suppress duplicate symbol warnings" if !LIVEPATCH
-	default y if !LIVEPATCH
+	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)

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

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

[Jan Beulich]

On 24.10.2019 15:31, 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.
> 
> Provide a Kconfig option to 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.
> 
> 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.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Ping? I realize I need to re-base this now that ...

> ---
> Kconfig change taken from "[PATCH v3 5/7] x86/livepatch: Fail the build
> if duplicate symbols exist". When re-basing onto that other patch I
> think we will also want to drop that other patch'es adjustment to
> allrandom.config again.

... the other patch mentioned here has gone in, but preferably I'd
do so alongside incorporating other review feedback.

Jan

> The clang behavior may require further tweaking if different versions
> behave differently. Alternatively we could pass two --redefine-sym
> arguments to objcopy.
> 
> --- 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
> @@ -338,9 +338,23 @@ config FAST_SYMBOL_LOOKUP
>  
>  	  If unsure, say Y.
>  
> +config ENFORCE_UNIQUE_SYMBOLS
> +	bool "Enforce unique symbols"
> +	default LIVEPATCH
> +	---help---
> +	  Multiple symbols with the same name aren't generally a problem
> +	  unless Live patching is to be used.
> +
> +	  Livepatch loading involves resolving relocations against symbol
> +	  names, and attempting to a duplicate symbol in a livepatch will
> +	  result in incorrect livepatch application.
> +
> +	  This option should be used to ensure that a build of Xen can have a
> +	  livepatch build and apply correctly.
> +
>  config SUPPRESS_DUPLICATE_SYMBOL_WARNINGS
> -	bool "Suppress duplicate symbol warnings" if !LIVEPATCH
> -	default y if !LIVEPATCH
> +	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)
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
> 


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

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

[George Dunlap]

On 11/7/19 7:20 AM, Jan Beulich wrote:
> On 24.10.2019 15:31, 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.
>>
>> Provide a Kconfig option to 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.

This is a good explanation, and I think the changes make sense.  But
unfortunately...

>> 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.

...I don't follow this at all.  What does the .file directive do in
those places, and why is it an issue?  And why do we always disable it
in clang?

 -George


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

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

[Jan Beulich]

On 07.11.2019 12:03, George Dunlap wrote:
> On 11/7/19 7:20 AM, Jan Beulich wrote:
>> On 24.10.2019 15:31, 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.
>>>
>>> Provide a Kconfig option to 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.
> 
> This is a good explanation, and I think the changes make sense.  But
> unfortunately...
> 
>>> 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.
> 
> ...I don't follow this at all.  What does the .file directive do in
> those places, and why is it an issue?

As explained at the beginning of the description, for some dir/file.c
passed to the compiler,
- gcc emits ".file file.c",
- clang emits ".file dir/file.c".
It was a long time ago that we had noticed issues with static symbols
because of gcc omitting the directory part. Hence some .file
directives got inserted in source files where we noticed it would
matter.

As to the "why is it an issue part" - these directives get in the way
of the new mechanism (because we ask for "file.c" symbols to be
renamed, not "dir/file.c" ones).

> And why do we always disable it in clang?

Because, as per above, it's redundant with what the compiler inserts.

Jan

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