[PATCH 0/3] build: fixes for clang 10

[PATCH 0/3] build: fixes for clang 10

[Roger Pau Monne]

Hello,

Patches 1 and 3 are fixes in order to build Xen with Clang 10. Patch 2
is a fix for a configure bug I've found while attempting to fix Clang
10.

Thanks, Roger.

Roger Pau Monne (3):
  x86/mm: do not attempt to convert _PAGE_GNTTAB to a boolean
  configure: also add EXTRA_PREFIX to {CPP/LD}FLAGS
  tools/libxl: disable clang indentation check for the disk parser

 m4/set_cflags_ldflags.m4    |  4 ++++
 tools/libxl/libxlu_disk_l.l | 11 +++++++++++
 xen/arch/x86/mm.c           |  2 +-
 3 files changed, 16 insertions(+), 1 deletion(-)

-- 
2.26.2

[PATCH 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, since the and
operation performed afterwards will already return false if the value
of the macro is 0.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/mm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 355c50ff91..27069d2451 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1236,7 +1236,7 @@ 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 ( (l1e_get_flags(l1e) & _PAGE_GNTTAB) &&
          !l1e_owner->is_shutting_down && !l1e_owner->is_dying )
     {
         gdprintk(XENLOG_WARNING,
-- 
2.26.2

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

[Jan Beulich]

On 05.05.2020 11:24, 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) &&
>          ^
> xen/include/asm/x86_64/page.h:161:25: note: expanded from macro '_PAGE_GNTTAB'
> #define _PAGE_GNTTAB (1U<<22)
>                         ^

This is a rather odd warning. Do they also warn for "if ( 0 )"
or "do { } while ( 0 )", as we use in various places? There's
no difference to me between a plain number and a constant
composed via an expression.

> Remove the conversion of _PAGE_GNTTAB to a boolean, since the and
> operation performed afterwards will already return false if the value
> of the macro is 0.

I'm sorry, but no. The expression was put there on purpose by
0932210ac095 ("x86: Address "Bitwise-and with zero
CONSTANT_EXPRESSION_RESULT" Coverity issues"), and the
description there is clearly telling us that this wants to stay
unless Coverity changed in the meantime. Otherwise I'm afraid
a more elaborate solution will be needed to please both. Or a
more simplistic one, like using "#if _PAGE_GNTTAB" around the
construct.

Jan

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

[Roger Pau Monné]

On Tue, May 05, 2020 at 03:47:43PM +0200, Jan Beulich wrote:
> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe.
> 
> On 05.05.2020 11:24, 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) &&
> >          ^
> > xen/include/asm/x86_64/page.h:161:25: note: expanded from macro '_PAGE_GNTTAB'
> > #define _PAGE_GNTTAB (1U<<22)
> >                         ^
> 
> This is a rather odd warning. Do they also warn for "if ( 0 )"
> or "do { } while ( 0 )", as we use in various places? There's
> no difference to me between a plain number and a constant
> composed via an expression.

Using plain 0 is fine, they just seem to dislike using << for some
reason that escapes me. Seems like it might be useful to catch bugs
where || is wrongly used instead of | when setting flags, ie:

https://github.com/haproxy/haproxy/issues/588

> 
> > Remove the conversion of _PAGE_GNTTAB to a boolean, since the and
> > operation performed afterwards will already return false if the value
> > of the macro is 0.
> 
> I'm sorry, but no. The expression was put there on purpose by
> 0932210ac095 ("x86: Address "Bitwise-and with zero
> CONSTANT_EXPRESSION_RESULT" Coverity issues"), and the
> description there is clearly telling us that this wants to stay
> unless Coverity changed in the meantime. Otherwise I'm afraid
> a more elaborate solution will be needed to please both.

Clang is fine with changing this to _PAGE_GNTTAB != 0. Would you be
OK with this approach?

> Or a
> more simplistic one, like using "#if _PAGE_GNTTAB" around the
> construct.

Yes, that's the other solution I had in mind.

Thanks, Roger.

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

[Jan Beulich]

On 05.05.2020 16:11, Roger Pau Monné wrote:
> On Tue, May 05, 2020 at 03:47:43PM +0200, Jan Beulich wrote:
>> On 05.05.2020 11:24, Roger Pau Monne wrote:
>>> Remove the conversion of _PAGE_GNTTAB to a boolean, since the and
>>> operation performed afterwards will already return false if the value
>>> of the macro is 0.
>>
>> I'm sorry, but no. The expression was put there on purpose by
>> 0932210ac095 ("x86: Address "Bitwise-and with zero
>> CONSTANT_EXPRESSION_RESULT" Coverity issues"), and the
>> description there is clearly telling us that this wants to stay
>> unless Coverity changed in the meantime. Otherwise I'm afraid
>> a more elaborate solution will be needed to please both.
> 
> Clang is fine with changing this to _PAGE_GNTTAB != 0. Would you be
> OK with this approach?

I'd be okay with it, but then I guess I'd prefer ...

>> Or a
>> more simplistic one, like using "#if _PAGE_GNTTAB" around the
>> construct.
> 
> Yes, that's the other solution I had in mind.

.... this one. Let's see if Andrew has a clear opinion either
way - it was him to address the original Coverity issue after
all.

Jan

[PATCH 2/3] configure: also add EXTRA_PREFIX to {CPP/LD}FLAGS

[Roger Pau Monne]

The path provided by EXTRA_PREFIX should be added to the search path
of the configure script, like it's done in Config.mk. Not doing so
makes the search path for configure differ from the search path used
by the build.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Please re-run autoconf.sh after applying.
---
 m4/set_cflags_ldflags.m4 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/m4/set_cflags_ldflags.m4 b/m4/set_cflags_ldflags.m4
index cbad3c10b0..08f5c983cc 100644
--- a/m4/set_cflags_ldflags.m4
+++ b/m4/set_cflags_ldflags.m4
@@ -15,6 +15,10 @@ for ldflag in $APPEND_LIB
 do
     APPEND_LDFLAGS="$APPEND_LDFLAGS -L$ldflag"
 done
+if [ ! -z $EXTRA_PREFIX ]; then
+    CPPFLAGS="$CPPFLAGS -I$EXTRA_PREFIX/include"
+    LDFLAGS="$LDFLAGS -L$EXTRA_PREFIX/lib"
+fi
 CPPFLAGS="$PREPEND_CPPFLAGS $CPPFLAGS $APPEND_CPPFLAGS"
 LDFLAGS="$PREPEND_LDFLAGS $LDFLAGS $APPEND_LDFLAGS"])
 
-- 
2.26.2

Re: [PATCH 2/3] configure: also add EXTRA_PREFIX to {CPP/LD}FLAGS

[Wei Liu]

On Tue, May 05, 2020 at 11:24:53AM +0200, Roger Pau Monne wrote:
> The path provided by EXTRA_PREFIX should be added to the search path
> of the configure script, like it's done in Config.mk. Not doing so
> makes the search path for configure differ from the search path used
> by the build.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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

Re: [PATCH 2/3] configure: also add EXTRA_PREFIX to {CPP/LD}FLAGS

[Bertrand Marquis]

Hi,

As a consequence of this fix, the following has been committed (I guess as a consequence of regenerating the configure scripts):
diff --git a/tools/configure b/tools/configure
index 375430df3f..36596389b8 100755
--- a/tools/configure
+++ b/tools/configure
@@ -4678,6 +4678,10 @@ for ldflag in $APPEND_LIB
 do
     APPEND_LDFLAGS="$APPEND_LDFLAGS -L$ldflag"
 done
+if  ! -z $EXTRA_PREFIX ; then
+    CPPFLAGS="$CPPFLAGS -I$EXTRA_PREFIX/include"
+    LDFLAGS="$LDFLAGS -L$EXTRA_PREFIX/lib"
+fi
 CPPFLAGS="$PREPEND_CPPFLAGS $CPPFLAGS $APPEND_CPPFLAGS"
 LDFLAGS="$PREPEND_LDFLAGS $LDFLAGS $APPEND_LDFLAGS”

This should be:
if  [ ! -z $EXTRA_PREFIX ]; then

As on other configure scripts.

During configure I have not the following error:
./configure: line 4681: -z: command not found

Which is ignored but is adding -L/lib and -I/include to the CPPFLAGS and LDFLAGS

What should be the procedure to actually fix that (as the problem is coming from the configure script regeneration I guess) ? 

Bertrand

> On 5 May 2020, at 10:24, Roger Pau Monne <roger.pau@citrix.com> wrote:
> 
> The path provided by EXTRA_PREFIX should be added to the search path
> of the configure script, like it's done in Config.mk. Not doing so
> makes the search path for configure differ from the search path used
> by the build.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Please re-run autoconf.sh after applying.
> ---
> m4/set_cflags_ldflags.m4 | 4 ++++
> 1 file changed, 4 insertions(+)
> 
> diff --git a/m4/set_cflags_ldflags.m4 b/m4/set_cflags_ldflags.m4
> index cbad3c10b0..08f5c983cc 100644
> --- a/m4/set_cflags_ldflags.m4
> +++ b/m4/set_cflags_ldflags.m4
> @@ -15,6 +15,10 @@ for ldflag in $APPEND_LIB
> do
>     APPEND_LDFLAGS="$APPEND_LDFLAGS -L$ldflag"
> done
> +if [ ! -z $EXTRA_PREFIX ]; then
> +    CPPFLAGS="$CPPFLAGS -I$EXTRA_PREFIX/include"
> +    LDFLAGS="$LDFLAGS -L$EXTRA_PREFIX/lib"
> +fi
> CPPFLAGS="$PREPEND_CPPFLAGS $CPPFLAGS $APPEND_CPPFLAGS"
> LDFLAGS="$PREPEND_LDFLAGS $LDFLAGS $APPEND_LDFLAGS"])
> 
> -- 
> 2.26.2
> 
> 

Re: [PATCH 2/3] configure: also add EXTRA_PREFIX to {CPP/LD}FLAGS

[Wei Liu]

On Fri, May 22, 2020 at 08:41:17AM +0000, Bertrand Marquis wrote:
> Hi,
> 
> As a consequence of this fix, the following has been committed (I guess as a consequence of regenerating the configure scripts):
> diff --git a/tools/configure b/tools/configure
> index 375430df3f..36596389b8 100755
> --- a/tools/configure
> +++ b/tools/configure
> @@ -4678,6 +4678,10 @@ for ldflag in $APPEND_LIB
>  do
>      APPEND_LDFLAGS="$APPEND_LDFLAGS -L$ldflag"
>  done
> +if  ! -z $EXTRA_PREFIX ; then
> +    CPPFLAGS="$CPPFLAGS -I$EXTRA_PREFIX/include"
> +    LDFLAGS="$LDFLAGS -L$EXTRA_PREFIX/lib"
> +fi
>  CPPFLAGS="$PREPEND_CPPFLAGS $CPPFLAGS $APPEND_CPPFLAGS"
>  LDFLAGS="$PREPEND_LDFLAGS $LDFLAGS $APPEND_LDFLAGS”
> 
> This should be:
> if  [ ! -z $EXTRA_PREFIX ]; then
> 
> As on other configure scripts.
> 
> During configure I have not the following error:
> ./configure: line 4681: -z: command not found
> 
> Which is ignored but is adding -L/lib and -I/include to the CPPFLAGS and LDFLAGS
> 
> What should be the procedure to actually fix that (as the problem is coming from the configure script regeneration I guess) ? 

Does the following patch work for you?

diff --git a/m4/set_cflags_ldflags.m4 b/m4/set_cflags_ldflags.m4
index 08f5c983cc63..cd34c139bc94 100644
--- a/m4/set_cflags_ldflags.m4
+++ b/m4/set_cflags_ldflags.m4
@@ -15,7 +15,7 @@ for ldflag in $APPEND_LIB
 do
     APPEND_LDFLAGS="$APPEND_LDFLAGS -L$ldflag"
 done
-if [ ! -z $EXTRA_PREFIX ]; then
+if test ! -z $EXTRA_PREFIX ; then
     CPPFLAGS="$CPPFLAGS -I$EXTRA_PREFIX/include"
     LDFLAGS="$LDFLAGS -L$EXTRA_PREFIX/lib"
 fi


You will need to run autogen.sh to regenerate tools/configure.

Wei.

> 
> Bertrand
> 
> > On 5 May 2020, at 10:24, Roger Pau Monne <roger.pau@citrix.com> wrote:
> > 
> > The path provided by EXTRA_PREFIX should be added to the search path
> > of the configure script, like it's done in Config.mk. Not doing so
> > makes the search path for configure differ from the search path used
> > by the build.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Please re-run autoconf.sh after applying.
> > ---
> > m4/set_cflags_ldflags.m4 | 4 ++++
> > 1 file changed, 4 insertions(+)
> > 
> > diff --git a/m4/set_cflags_ldflags.m4 b/m4/set_cflags_ldflags.m4
> > index cbad3c10b0..08f5c983cc 100644
> > --- a/m4/set_cflags_ldflags.m4
> > +++ b/m4/set_cflags_ldflags.m4
> > @@ -15,6 +15,10 @@ for ldflag in $APPEND_LIB
> > do
> >     APPEND_LDFLAGS="$APPEND_LDFLAGS -L$ldflag"
> > done
> > +if [ ! -z $EXTRA_PREFIX ]; then
> > +    CPPFLAGS="$CPPFLAGS -I$EXTRA_PREFIX/include"
> > +    LDFLAGS="$LDFLAGS -L$EXTRA_PREFIX/lib"
> > +fi
> > CPPFLAGS="$PREPEND_CPPFLAGS $CPPFLAGS $APPEND_CPPFLAGS"
> > LDFLAGS="$PREPEND_LDFLAGS $LDFLAGS $APPEND_LDFLAGS"])
> > 
> > -- 
> > 2.26.2
> > 
> > 
> 

Re: [PATCH 2/3] configure: also add EXTRA_PREFIX to {CPP/LD}FLAGS

[Bertrand Marquis]

Hi,

> On 22 May 2020, at 10:05, Wei Liu <wl@xen.org> wrote:
> 
> On Fri, May 22, 2020 at 08:41:17AM +0000, Bertrand Marquis wrote:
>> Hi,
>> 
>> As a consequence of this fix, the following has been committed (I guess as a consequence of regenerating the configure scripts):
>> diff --git a/tools/configure b/tools/configure
>> index 375430df3f..36596389b8 100755
>> --- a/tools/configure
>> +++ b/tools/configure
>> @@ -4678,6 +4678,10 @@ for ldflag in $APPEND_LIB
>> do
>>     APPEND_LDFLAGS="$APPEND_LDFLAGS -L$ldflag"
>> done
>> +if  ! -z $EXTRA_PREFIX ; then
>> +    CPPFLAGS="$CPPFLAGS -I$EXTRA_PREFIX/include"
>> +    LDFLAGS="$LDFLAGS -L$EXTRA_PREFIX/lib"
>> +fi
>> CPPFLAGS="$PREPEND_CPPFLAGS $CPPFLAGS $APPEND_CPPFLAGS"
>> LDFLAGS="$PREPEND_LDFLAGS $LDFLAGS $APPEND_LDFLAGS”
>> 
>> This should be:
>> if  [ ! -z $EXTRA_PREFIX ]; then
>> 
>> As on other configure scripts.
>> 
>> During configure I have not the following error:
>> ./configure: line 4681: -z: command not found
>> 
>> Which is ignored but is adding -L/lib and -I/include to the CPPFLAGS and LDFLAGS
>> 
>> What should be the procedure to actually fix that (as the problem is coming from the configure script regeneration I guess) ? 
> 
> Does the following patch work for you?
> 
> diff --git a/m4/set_cflags_ldflags.m4 b/m4/set_cflags_ldflags.m4
> index 08f5c983cc63..cd34c139bc94 100644
> --- a/m4/set_cflags_ldflags.m4
> +++ b/m4/set_cflags_ldflags.m4
> @@ -15,7 +15,7 @@ for ldflag in $APPEND_LIB
> do
>     APPEND_LDFLAGS="$APPEND_LDFLAGS -L$ldflag"
> done
> -if [ ! -z $EXTRA_PREFIX ]; then
> +if test ! -z $EXTRA_PREFIX ; then
>     CPPFLAGS="$CPPFLAGS -I$EXTRA_PREFIX/include"
>     LDFLAGS="$LDFLAGS -L$EXTRA_PREFIX/lib"
> fi
> 
> 
> You will need to run autogen.sh to regenerate tools/configure.
> 

Yes that works on my side and generate tools/configure using “test”

But why are the [] being removed when generating tools/configure ?

Bertrand

> Wei.
> 
>> 
>> Bertrand
>> 
>>> On 5 May 2020, at 10:24, Roger Pau Monne <roger.pau@citrix.com> wrote:
>>> 
>>> The path provided by EXTRA_PREFIX should be added to the search path
>>> of the configure script, like it's done in Config.mk. Not doing so
>>> makes the search path for configure differ from the search path used
>>> by the build.
>>> 
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> ---
>>> Please re-run autoconf.sh after applying.
>>> ---
>>> m4/set_cflags_ldflags.m4 | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>> 
>>> diff --git a/m4/set_cflags_ldflags.m4 b/m4/set_cflags_ldflags.m4
>>> index cbad3c10b0..08f5c983cc 100644
>>> --- a/m4/set_cflags_ldflags.m4
>>> +++ b/m4/set_cflags_ldflags.m4
>>> @@ -15,6 +15,10 @@ for ldflag in $APPEND_LIB
>>> do
>>>    APPEND_LDFLAGS="$APPEND_LDFLAGS -L$ldflag"
>>> done
>>> +if [ ! -z $EXTRA_PREFIX ]; then
>>> +    CPPFLAGS="$CPPFLAGS -I$EXTRA_PREFIX/include"
>>> +    LDFLAGS="$LDFLAGS -L$EXTRA_PREFIX/lib"
>>> +fi
>>> CPPFLAGS="$PREPEND_CPPFLAGS $CPPFLAGS $APPEND_CPPFLAGS"
>>> LDFLAGS="$PREPEND_LDFLAGS $LDFLAGS $APPEND_LDFLAGS"])
>>> 
>>> -- 
>>> 2.26.2

Re: [PATCH 2/3] configure: also add EXTRA_PREFIX to {CPP/LD}FLAGS

[Wei Liu]

On Fri, May 22, 2020 at 09:37:51AM +0000, Bertrand Marquis wrote:
> Hi,
> 
> > On 22 May 2020, at 10:05, Wei Liu <wl@xen.org> wrote:
> > 
> > On Fri, May 22, 2020 at 08:41:17AM +0000, Bertrand Marquis wrote:
> >> Hi,
> >> 
> >> As a consequence of this fix, the following has been committed (I guess as a consequence of regenerating the configure scripts):
> >> diff --git a/tools/configure b/tools/configure
> >> index 375430df3f..36596389b8 100755
> >> --- a/tools/configure
> >> +++ b/tools/configure
> >> @@ -4678,6 +4678,10 @@ for ldflag in $APPEND_LIB
> >> do
> >>     APPEND_LDFLAGS="$APPEND_LDFLAGS -L$ldflag"
> >> done
> >> +if  ! -z $EXTRA_PREFIX ; then
> >> +    CPPFLAGS="$CPPFLAGS -I$EXTRA_PREFIX/include"
> >> +    LDFLAGS="$LDFLAGS -L$EXTRA_PREFIX/lib"
> >> +fi
> >> CPPFLAGS="$PREPEND_CPPFLAGS $CPPFLAGS $APPEND_CPPFLAGS"
> >> LDFLAGS="$PREPEND_LDFLAGS $LDFLAGS $APPEND_LDFLAGS”
> >> 
> >> This should be:
> >> if  [ ! -z $EXTRA_PREFIX ]; then
> >> 
> >> As on other configure scripts.
> >> 
> >> During configure I have not the following error:
> >> ./configure: line 4681: -z: command not found
> >> 
> >> Which is ignored but is adding -L/lib and -I/include to the CPPFLAGS and LDFLAGS
> >> 
> >> What should be the procedure to actually fix that (as the problem is coming from the configure script regeneration I guess) ? 
> > 
> > Does the following patch work for you?
> > 
> > diff --git a/m4/set_cflags_ldflags.m4 b/m4/set_cflags_ldflags.m4
> > index 08f5c983cc63..cd34c139bc94 100644
> > --- a/m4/set_cflags_ldflags.m4
> > +++ b/m4/set_cflags_ldflags.m4
> > @@ -15,7 +15,7 @@ for ldflag in $APPEND_LIB
> > do
> >     APPEND_LDFLAGS="$APPEND_LDFLAGS -L$ldflag"
> > done
> > -if [ ! -z $EXTRA_PREFIX ]; then
> > +if test ! -z $EXTRA_PREFIX ; then
> >     CPPFLAGS="$CPPFLAGS -I$EXTRA_PREFIX/include"
> >     LDFLAGS="$LDFLAGS -L$EXTRA_PREFIX/lib"
> > fi
> > 
> > 
> > You will need to run autogen.sh to regenerate tools/configure.
> > 
> 
> Yes that works on my side and generate tools/configure using “test”
> 
> But why are the [] being removed when generating tools/configure ?

No idea why autoconf removed [] really.

I think switching to test is better anyway since that's what is used
throughout tools/configure.

Wei.

Re: [PATCH 2/3] configure: also add EXTRA_PREFIX to {CPP/LD}FLAGS

[Roger Pau Monné]

On Fri, May 22, 2020 at 10:05:53AM +0100, Wei Liu wrote:
> On Fri, May 22, 2020 at 08:41:17AM +0000, Bertrand Marquis wrote:
> > Hi,
> > 
> > As a consequence of this fix, the following has been committed (I guess as a consequence of regenerating the configure scripts):
> > diff --git a/tools/configure b/tools/configure
> > index 375430df3f..36596389b8 100755
> > --- a/tools/configure
> > +++ b/tools/configure
> > @@ -4678,6 +4678,10 @@ for ldflag in $APPEND_LIB
> >  do
> >      APPEND_LDFLAGS="$APPEND_LDFLAGS -L$ldflag"
> >  done
> > +if  ! -z $EXTRA_PREFIX ; then
> > +    CPPFLAGS="$CPPFLAGS -I$EXTRA_PREFIX/include"
> > +    LDFLAGS="$LDFLAGS -L$EXTRA_PREFIX/lib"
> > +fi
> >  CPPFLAGS="$PREPEND_CPPFLAGS $CPPFLAGS $APPEND_CPPFLAGS"
> >  LDFLAGS="$PREPEND_LDFLAGS $LDFLAGS $APPEND_LDFLAGS”
> > 
> > This should be:
> > if  [ ! -z $EXTRA_PREFIX ]; then
> > 
> > As on other configure scripts.
> > 
> > During configure I have not the following error:
> > ./configure: line 4681: -z: command not found
> > 
> > Which is ignored but is adding -L/lib and -I/include to the CPPFLAGS and LDFLAGS
> > 
> > What should be the procedure to actually fix that (as the problem is coming from the configure script regeneration I guess) ? 
> 
> Does the following patch work for you?
> 
> diff --git a/m4/set_cflags_ldflags.m4 b/m4/set_cflags_ldflags.m4
> index 08f5c983cc63..cd34c139bc94 100644
> --- a/m4/set_cflags_ldflags.m4
> +++ b/m4/set_cflags_ldflags.m4
> @@ -15,7 +15,7 @@ for ldflag in $APPEND_LIB
>  do
>      APPEND_LDFLAGS="$APPEND_LDFLAGS -L$ldflag"
>  done
> -if [ ! -z $EXTRA_PREFIX ]; then
> +if test ! -z $EXTRA_PREFIX ; then
>      CPPFLAGS="$CPPFLAGS -I$EXTRA_PREFIX/include"
>      LDFLAGS="$LDFLAGS -L$EXTRA_PREFIX/lib"
>  fi

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

My bad, I assume [] is expanded by m4, as that seems to be part of the
language?

Thanks, Roger.

Re: [PATCH 2/3] configure: also add EXTRA_PREFIX to {CPP/LD}FLAGS

[Bertrand Marquis]

> On 22 May 2020, at 12:19, Roger Pau Monné <roger.pau@citrix.com> wrote:
> 
> On Fri, May 22, 2020 at 10:05:53AM +0100, Wei Liu wrote:
>> On Fri, May 22, 2020 at 08:41:17AM +0000, Bertrand Marquis wrote:
>>> Hi,
>>> 
>>> As a consequence of this fix, the following has been committed (I guess as a consequence of regenerating the configure scripts):
>>> diff --git a/tools/configure b/tools/configure
>>> index 375430df3f..36596389b8 100755
>>> --- a/tools/configure
>>> +++ b/tools/configure
>>> @@ -4678,6 +4678,10 @@ for ldflag in $APPEND_LIB
>>> do
>>>     APPEND_LDFLAGS="$APPEND_LDFLAGS -L$ldflag"
>>> done
>>> +if  ! -z $EXTRA_PREFIX ; then
>>> +    CPPFLAGS="$CPPFLAGS -I$EXTRA_PREFIX/include"
>>> +    LDFLAGS="$LDFLAGS -L$EXTRA_PREFIX/lib"
>>> +fi
>>> CPPFLAGS="$PREPEND_CPPFLAGS $CPPFLAGS $APPEND_CPPFLAGS"
>>> LDFLAGS="$PREPEND_LDFLAGS $LDFLAGS $APPEND_LDFLAGS”
>>> 
>>> This should be:
>>> if  [ ! -z $EXTRA_PREFIX ]; then
>>> 
>>> As on other configure scripts.
>>> 
>>> During configure I have not the following error:
>>> ./configure: line 4681: -z: command not found
>>> 
>>> Which is ignored but is adding -L/lib and -I/include to the CPPFLAGS and LDFLAGS
>>> 
>>> What should be the procedure to actually fix that (as the problem is coming from the configure script regeneration I guess) ? 
>> 
>> Does the following patch work for you?
>> 
>> diff --git a/m4/set_cflags_ldflags.m4 b/m4/set_cflags_ldflags.m4
>> index 08f5c983cc63..cd34c139bc94 100644
>> --- a/m4/set_cflags_ldflags.m4
>> +++ b/m4/set_cflags_ldflags.m4
>> @@ -15,7 +15,7 @@ for ldflag in $APPEND_LIB
>> do
>>     APPEND_LDFLAGS="$APPEND_LDFLAGS -L$ldflag"
>> done
>> -if [ ! -z $EXTRA_PREFIX ]; then
>> +if test ! -z $EXTRA_PREFIX ; then
>>     CPPFLAGS="$CPPFLAGS -I$EXTRA_PREFIX/include"
>>     LDFLAGS="$LDFLAGS -L$EXTRA_PREFIX/lib"
>> fi
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

> 
> My bad, I assume [] is expanded by m4, as that seems to be part of the
> language?
> 
> Thanks, Roger.

Re: [PATCH 2/3] configure: also add EXTRA_PREFIX to {CPP/LD}FLAGS

[Wei Liu]

On Fri, May 22, 2020 at 11:40:06AM +0000, Bertrand Marquis wrote:
> 
> 
> > On 22 May 2020, at 12:19, Roger Pau Monné <roger.pau@citrix.com> wrote:
> > 
> > On Fri, May 22, 2020 at 10:05:53AM +0100, Wei Liu wrote:
> >> On Fri, May 22, 2020 at 08:41:17AM +0000, Bertrand Marquis wrote:
> >>> Hi,
> >>> 
> >>> As a consequence of this fix, the following has been committed (I guess as a consequence of regenerating the configure scripts):
> >>> diff --git a/tools/configure b/tools/configure
> >>> index 375430df3f..36596389b8 100755
> >>> --- a/tools/configure
> >>> +++ b/tools/configure
> >>> @@ -4678,6 +4678,10 @@ for ldflag in $APPEND_LIB
> >>> do
> >>>     APPEND_LDFLAGS="$APPEND_LDFLAGS -L$ldflag"
> >>> done
> >>> +if  ! -z $EXTRA_PREFIX ; then
> >>> +    CPPFLAGS="$CPPFLAGS -I$EXTRA_PREFIX/include"
> >>> +    LDFLAGS="$LDFLAGS -L$EXTRA_PREFIX/lib"
> >>> +fi
> >>> CPPFLAGS="$PREPEND_CPPFLAGS $CPPFLAGS $APPEND_CPPFLAGS"
> >>> LDFLAGS="$PREPEND_LDFLAGS $LDFLAGS $APPEND_LDFLAGS”
> >>> 
> >>> This should be:
> >>> if  [ ! -z $EXTRA_PREFIX ]; then
> >>> 
> >>> As on other configure scripts.
> >>> 
> >>> During configure I have not the following error:
> >>> ./configure: line 4681: -z: command not found
> >>> 
> >>> Which is ignored but is adding -L/lib and -I/include to the CPPFLAGS and LDFLAGS
> >>> 
> >>> What should be the procedure to actually fix that (as the problem is coming from the configure script regeneration I guess) ? 
> >> 
> >> Does the following patch work for you?
> >> 
> >> diff --git a/m4/set_cflags_ldflags.m4 b/m4/set_cflags_ldflags.m4
> >> index 08f5c983cc63..cd34c139bc94 100644
> >> --- a/m4/set_cflags_ldflags.m4
> >> +++ b/m4/set_cflags_ldflags.m4
> >> @@ -15,7 +15,7 @@ for ldflag in $APPEND_LIB
> >> do
> >>     APPEND_LDFLAGS="$APPEND_LDFLAGS -L$ldflag"
> >> done
> >> -if [ ! -z $EXTRA_PREFIX ]; then
> >> +if test ! -z $EXTRA_PREFIX ; then
> >>     CPPFLAGS="$CPPFLAGS -I$EXTRA_PREFIX/include"
> >>     LDFLAGS="$LDFLAGS -L$EXTRA_PREFIX/lib"
> >> fi
> > 
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Thanks. I will transfer your tag to the proper patch I just sent.

Wei.

[PATCH 3/3] tools/libxl: disable clang indentation check for the disk parser

[Roger Pau Monne]

Clang 10 complains with:

13: error: misleading indentation; statement is not part of the previous 'if'
      [-Werror,-Wmisleading-indentation]
            if ( ! yyg->yy_state_buf )
            ^
libxlu_disk_l.c:1259:9: note: previous statement is here
        if ( ! yyg->yy_state_buf )
        ^

Due to the missing braces in single line statements and the wrong
indentation. Fix this by disabling the warning for that specific file.
I haven't found a way to force flex to add braces around single line
statements in conditional blocks.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Please re-generate libxlu_disk_l.c before committing.
---
 tools/libxl/libxlu_disk_l.l | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/tools/libxl/libxlu_disk_l.l b/tools/libxl/libxlu_disk_l.l
index 97039a2800..7a46f4a30c 100644
--- a/tools/libxl/libxlu_disk_l.l
+++ b/tools/libxl/libxlu_disk_l.l
@@ -36,6 +36,17 @@
 
 #define YY_NO_INPUT
 
+/* The code generated by flex is missing braces in single line expressions and
+ * is not properly indented, which triggers the clang misleading-indentation
+ * check that has been made part of -Wall since clang 10. In order to safely
+ * disable it on clang versions that don't have the diagnostic implemented
+ * also disable the unknown option and pragma warning. */
+#ifdef __clang__
+# pragma clang diagnostic ignored "-Wunknown-pragmas"
+# pragma clang diagnostic ignored "-Wunknown-warning-option"
+# pragma clang diagnostic ignored "-Wmisleading-indentation"
+#endif
+
 /* Some versions of flex have a bug (Fedora bugzilla 612465) which causes
  * it to fail to declare these functions, which it defines.  So declare
  * them ourselves.  Hopefully we won't have to simultaneously support
-- 
2.26.2

Re: [PATCH 3/3] tools/libxl: disable clang indentation check for the disk parser

[Wei Liu]

On Tue, May 05, 2020 at 11:24:54AM +0200, Roger Pau Monne wrote:
> Clang 10 complains with:
> 
> 13: error: misleading indentation; statement is not part of the previous 'if'
>       [-Werror,-Wmisleading-indentation]
>             if ( ! yyg->yy_state_buf )
>             ^
> libxlu_disk_l.c:1259:9: note: previous statement is here
>         if ( ! yyg->yy_state_buf )
>         ^
> 
> Due to the missing braces in single line statements and the wrong
> indentation. Fix this by disabling the warning for that specific file.
> I haven't found a way to force flex to add braces around single line
> statements in conditional blocks.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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