[PATCH v2 0/3] Clang/LLVM build fixes

[PATCH v2 0/3] Clang/LLVM build fixes

[Roger Pau Monne]

Hello,

A couple of build fixes for Clang/LLVM. First patch was already sent,
patches #2 and #3 are new as a result of recent commits.

Thanks, Roger.

Roger Pau Monne (3):
  x86/mm: do not attempt to convert _PAGE_GNTTAB to a boolean
  build32: don't discard .shstrtab in linker script
  clang: don't define nocall

 xen/arch/x86/boot/build32.lds | 5 +++++
 xen/arch/x86/mm.c             | 4 +++-
 xen/include/xen/compiler.h    | 6 +++++-
 3 files changed, 13 insertions(+), 2 deletions(-)

-- 
2.26.2

[PATCH v2 1/3] x86/mm: do not attempt to convert _PAGE_GNTTAB to a boolean

[Roger Pau Monne]

Clang 10 complains with:

mm.c:1239:10: error: converting the result of '<<' to a boolean always evaluates to true
      [-Werror,-Wtautological-constant-compare]
    if ( _PAGE_GNTTAB && (l1e_get_flags(l1e) & _PAGE_GNTTAB) &&
         ^
xen/include/asm/x86_64/page.h:161:25: note: expanded from macro '_PAGE_GNTTAB'
#define _PAGE_GNTTAB (1U<<22)
                        ^

Remove the conversion of _PAGE_GNTTAB to a boolean and instead use a
preprocessor conditional to check if _PAGE_GNTTAB is defined.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Use a preprocessor conditional.
---
 xen/arch/x86/mm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 54980b4eb1..7bcac17e2e 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1236,7 +1236,8 @@ void put_page_from_l1e(l1_pgentry_t l1e, struct domain *l1e_owner)
      * (Note that the undestroyable active grants are not a security hole in
      * Xen. All active grants can safely be cleaned up when the domain dies.)
      */
-    if ( _PAGE_GNTTAB && (l1e_get_flags(l1e) & _PAGE_GNTTAB) &&
+#if _PAGE_GNTTAB
+    if ( (l1e_get_flags(l1e) & _PAGE_GNTTAB) &&
          !l1e_owner->is_shutting_down && !l1e_owner->is_dying )
     {
         gdprintk(XENLOG_WARNING,
@@ -1244,6 +1245,7 @@ void put_page_from_l1e(l1_pgentry_t l1e, struct domain *l1e_owner)
                  l1e_get_intpte(l1e));
         domain_crash(l1e_owner);
     }
+#endif
 
     /*
      * Remember we didn't take a type-count of foreign writable mappings
-- 
2.26.2

Re: [PATCH v2 1/3] x86/mm: do not attempt to convert _PAGE_GNTTAB to a boolean

[Jan Beulich]

On 28.05.2020 16:40, Roger Pau Monne wrote:
> Clang 10 complains with:
> 
> mm.c:1239:10: error: converting the result of '<<' to a boolean always evaluates to true
>       [-Werror,-Wtautological-constant-compare]
>     if ( _PAGE_GNTTAB && (l1e_get_flags(l1e) & _PAGE_GNTTAB) &&

And taking the wording of the message exactly as it is, it is wrong
(if the shifted value is zero, or if all set bits get shifted out).
But the warning is bogus imo anyway - we have it like this for a
reason. I'd then wonder whether -Wtautological-constant-compare
actually does any good, or whether as an alternative we don't want
to disable it.

I further wonder whether they might not warn about the same in #if
down the road.

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -1236,7 +1236,8 @@ void put_page_from_l1e(l1_pgentry_t l1e, struct domain *l1e_owner)
>       * (Note that the undestroyable active grants are not a security hole in
>       * Xen. All active grants can safely be cleaned up when the domain dies.)
>       */
> -    if ( _PAGE_GNTTAB && (l1e_get_flags(l1e) & _PAGE_GNTTAB) &&
> +#if _PAGE_GNTTAB
> +    if ( (l1e_get_flags(l1e) & _PAGE_GNTTAB) &&
>           !l1e_owner->is_shutting_down && !l1e_owner->is_dying )

Us moving in general(?) away from #if / #ifdef to constructs including
the condition in a suitable form in a non-preprocessor expression, I
think we want a comment here clarifying that this shouldn't be
converted back to how it is right now. With that added, for the
immediate purpose:
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

[PATCH v2 2/3] build32: don't discard .shstrtab in linker script

[Roger Pau Monne]

LLVM linker doesn't support discarding .shstrtab, and complains with:

ld -melf_i386_fbsd -N -T build32.lds -o reloc.lnk reloc.o
ld: error: discarding .shstrtab section is not allowed

Add an explicit .shstrtab section to the linker script after the text
section in order to make LLVM LD happy.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/boot/build32.lds | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/xen/arch/x86/boot/build32.lds b/xen/arch/x86/boot/build32.lds
index 97454b40ff..5bd42ee4d9 100644
--- a/xen/arch/x86/boot/build32.lds
+++ b/xen/arch/x86/boot/build32.lds
@@ -50,6 +50,11 @@ SECTIONS
         *(.got.plt)
   }
 
+  .shstrtab : {
+        /* Discarding .shstrtab is not supported by LLD (LLVM LD). */
+        *(.shstrtab)
+  }
+
   /DISCARD/ : {
         /*
          * Discard everything else, to prevent linkers from putting
-- 
2.26.2

Re: [PATCH v2 2/3] build32: don't discard .shstrtab in linker script

[Jan Beulich]

On 28.05.2020 16:40, Roger Pau Monne wrote:
> LLVM linker doesn't support discarding .shstrtab, and complains with:
> 
> ld -melf_i386_fbsd -N -T build32.lds -o reloc.lnk reloc.o
> ld: error: discarding .shstrtab section is not allowed

Well, yes, GNU ld is more intelligent and doesn't extend the
discarding to the control sections in the first place. All
of .symtab, .strtab, and .shstrtab are still there.

> Add an explicit .shstrtab section to the linker script after the text
> section in order to make LLVM LD happy.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Assuming the change was tested to not confuse GNU ld
Acked-by: Jan Beulich <jbeulich@suse.com>

I wouldn't mind extending this to the other two control
sections named above. In case the binaries need picking
apart, them being present is surely helpful.

Jan

Re: [PATCH v2 2/3] build32: don't discard .shstrtab in linker script

[Roger Pau Monné]

On Fri, May 29, 2020 at 05:45:44PM +0200, Jan Beulich wrote:
> On 28.05.2020 16:40, Roger Pau Monne wrote:
> > LLVM linker doesn't support discarding .shstrtab, and complains with:
> > 
> > ld -melf_i386_fbsd -N -T build32.lds -o reloc.lnk reloc.o
> > ld: error: discarding .shstrtab section is not allowed
> 
> Well, yes, GNU ld is more intelligent and doesn't extend the
> discarding to the control sections in the first place. All
> of .symtab, .strtab, and .shstrtab are still there.
> 
> > Add an explicit .shstrtab section to the linker script after the text
> > section in order to make LLVM LD happy.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Assuming the change was tested to not confuse GNU ld
> Acked-by: Jan Beulich <jbeulich@suse.com>

Yes, it's been tested on the gitlab CI, no issues reported on any of
the builds:

https://gitlab.com/xen-project/people/royger/xen/pipelines/151009839

> I wouldn't mind extending this to the other two control
> sections named above. In case the binaries need picking
> apart, them being present is surely helpful.

I don't mind extending, it might make sense in case linkers start
complaining about trying to discard those too.

Thanks, Roger.

[PATCH v2 3/3] clang: don't define nocall

[Roger Pau Monne]

Clang doesn't support attribute error, and the possible equivalents
like diagnose_if don't seem to work well in this case as they trigger
when when the function is not called (just by being used by the
APPEND_CALL macro).

Define nocall to a noop on clang until a proper solution can be found.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/include/xen/compiler.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h
index c22439b7a4..225e09e5f7 100644
--- a/xen/include/xen/compiler.h
+++ b/xen/include/xen/compiler.h
@@ -20,7 +20,11 @@
 
 #define __weak        __attribute__((__weak__))
 
-#define nocall        __attribute__((error("Nonstandard ABI")))
+#if !defined(__clang__)
+# define nocall        __attribute__((error("Nonstandard ABI")))
+#else
+# define nocall
+#endif
 
 #if (!defined(__clang__) && (__GNUC__ == 4) && (__GNUC_MINOR__ < 5))
 #define unreachable() do {} while (1)
-- 
2.26.2

Re: [PATCH v2 3/3] clang: don't define nocall

[Julien Grall]

Hi Roger,

On 28/05/2020 15:40, Roger Pau Monne wrote:
> Clang doesn't support attribute error, and the possible equivalents
> like diagnose_if don't seem to work well in this case as they trigger
> when when the function is not called (just by being used by the
> APPEND_CALL macro).

OOI, could you share the diagnose_if change you tried?

> 
> Define nocall to a noop on clang until a proper solution can be found.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Julien Grall <jgrall@amazon.com>

> ---
>   xen/include/xen/compiler.h | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h
> index c22439b7a4..225e09e5f7 100644
> --- a/xen/include/xen/compiler.h
> +++ b/xen/include/xen/compiler.h
> @@ -20,7 +20,11 @@
>   
>   #define __weak        __attribute__((__weak__))
>   
> -#define nocall        __attribute__((error("Nonstandard ABI")))
> +#if !defined(__clang__)
> +# define nocall        __attribute__((error("Nonstandard ABI")))
> +#else
> +# define nocall
> +#endif
>   
>   #if (!defined(__clang__) && (__GNUC__ == 4) && (__GNUC_MINOR__ < 5))
>   #define unreachable() do {} while (1)
> 

Cheers,

-- 
Julien Grall

Re: [PATCH v2 3/3] clang: don't define nocall

[Roger Pau Monné]

On Fri, May 29, 2020 at 08:25:44AM +0100, Julien Grall wrote:
> Hi Roger,
> 
> On 28/05/2020 15:40, Roger Pau Monne wrote:
> > Clang doesn't support attribute error, and the possible equivalents
> > like diagnose_if don't seem to work well in this case as they trigger
> > when when the function is not called (just by being used by the
> > APPEND_CALL macro).
> 
> OOI, could you share the diagnose_if change you tried?

I've sent a reduced version to the llvm-dev mailing list, because I
think the documentation should be fixed for diagnose_if. The email
with the example is at:

http://lists.llvm.org/pipermail/llvm-dev/2020-May/141908.html

FWIW, using the deprecated attribute will also trigger the same
error/warning even when not calling the function from C.

> > 
> > Define nocall to a noop on clang until a proper solution can be found.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Acked-by: Julien Grall <jgrall@amazon.com>

Thanks!

Re: [PATCH v2 3/3] clang: don't define nocall

[Julien Grall]

Hi,

On 29/05/2020 09:31, Roger Pau Monné wrote:
> On Fri, May 29, 2020 at 08:25:44AM +0100, Julien Grall wrote:
>> Hi Roger,
>>
>> On 28/05/2020 15:40, Roger Pau Monne wrote:
>>> Clang doesn't support attribute error, and the possible equivalents
>>> like diagnose_if don't seem to work well in this case as they trigger
>>> when when the function is not called (just by being used by the
>>> APPEND_CALL macro).
>>
>> OOI, could you share the diagnose_if change you tried?
> 
> I've sent a reduced version to the llvm-dev mailing list, because I
> think the documentation should be fixed for diagnose_if. The email
> with the example is at:
> 
> http://lists.llvm.org/pipermail/llvm-dev/2020-May/141908.html

Thanks!

> 
> FWIW, using the deprecated attribute will also trigger the same
> error/warning even when not calling the function from C.

I read the documentation before asking the code and I did wonder why 
diagnose_if wouldn't work.

I am guessing the behavior wasn't intended.

Cheers,

-- 
Julien Grall

Re: [PATCH v2 3/3] clang: don't define nocall

[Jan Beulich]

On 28.05.2020 16:40, Roger Pau Monne wrote:
> --- a/xen/include/xen/compiler.h
> +++ b/xen/include/xen/compiler.h
> @@ -20,7 +20,11 @@
>  
>  #define __weak        __attribute__((__weak__))
>  
> -#define nocall        __attribute__((error("Nonstandard ABI")))
> +#if !defined(__clang__)
> +# define nocall        __attribute__((error("Nonstandard ABI")))

I think a blank wants dropping from here. I also should have noticed
already on Andrew's putting in of this construct that in a header
we'd better use __error__. Both can easily be taken care of while
committing.

Jan