Re: [Xen-devel] [PATCH] x86/vmx: fix build with clang 3.8.0

[Andrew Cooper]

On 09/02/17 14:20, Jan Beulich wrote:
 On 09.02.17 at 14:42,  wrote:
>> On 09/02/17 13:14, Jan Beulich wrote:
>> On 09.02.17 at 14:05,  wrote:
 On 09/02/17 13:01, Jan Beulich wrote:
 On 09.02.17 at 13:49,  wrote:
>> On 09/02/17 11:33, Roger Pau Monne wrote:
>>> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
>>> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
>>> @@ -602,15 +602,16 @@ void vmx_pi_hooks_assign(struct domain *d);
>>>  void vmx_pi_hooks_deassign(struct domain *d);
>>>  
>>>  /* EPT violation qualifications definitions */
>>> -typedef union __transparent__ ept_qual {
>>> +typedef union ept_qual {
>> Please can we use
>>
>> typedef __transparent__ union ept_qual {
>>
>> which clang is happy with, and will help avoid problems such as the
>> cper_mce_record issue in c/s f8be76e2fe
> Would clang also be happy with it moved near the end of that
> line
>
> typedef union ept_qual __transparent__ {
>
> Having the attribute ahead of "union" is, I think, strictly speaking
> undefined behavior, as it then may as well apply to "typedef".
 No.  The result is

 /local/xen.git/xen/include/asm/hvm/vmx/vmx.h:605:40: error: expected
 identifier or '('
 typedef union ept_qual __transparent__ {
^
 /local/xen.git/xen/include/asm/hvm/vmx/vmx.h:614:3: error: type
 specifier missing, defaults to 'int' [-Werror,-Wimplicit-int]
 } ept_qual_t;
   ^~
 2 errors generated.

 In which case the original patch as proposed will probably do.  It turns
 out the presence of ept_qual_t does cause a compiler error if
 __transparent__ is missing from scope.
>>> But then the question is what the attribute applies to in the original
>>> version - the union, or just the typedef? The placement would
>>> suggest the latter, so I'd again be afraid of undefined behavior. Can
>>> it be moved ahead on that line?
>> You mean this?
>>
>> } __transparent__ ept_qual_t;
> Yes.
>
>> Clang is happy with that.
> Good.

Ok.  Fixed up and pushed.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] x86/vmx: fix build with clang 3.8.0

[Jan Beulich]

>>> On 09.02.17 at 14:45,  wrote:
> On Thu, Feb 09, 2017 at 06:14:54AM -0700, Jan Beulich wrote:
>> >>> On 09.02.17 at 14:05,  wrote:
>> > On 09/02/17 13:01, Jan Beulich wrote:
>> > On 09.02.17 at 13:49,  wrote:
>> >>> On 09/02/17 11:33, Roger Pau Monne wrote:
>>  --- a/xen/include/asm-x86/hvm/vmx/vmx.h
>>  +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
>>  @@ -602,15 +602,16 @@ void vmx_pi_hooks_assign(struct domain *d);
>>   void vmx_pi_hooks_deassign(struct domain *d);
>>   
>>   /* EPT violation qualifications definitions */
>>  -typedef union __transparent__ ept_qual {
>>  +typedef union ept_qual {
>> >>> Please can we use
>> >>>
>> >>> typedef __transparent__ union ept_qual {
>> >>>
>> >>> which clang is happy with, and will help avoid problems such as the
>> >>> cper_mce_record issue in c/s f8be76e2fe
>> >> Would clang also be happy with it moved near the end of that
>> >> line
>> >>
>> >> typedef union ept_qual __transparent__ {
>> >>
>> >> Having the attribute ahead of "union" is, I think, strictly speaking
>> >> undefined behavior, as it then may as well apply to "typedef".
>> > 
>> > No.  The result is
>> > 
>> > /local/xen.git/xen/include/asm/hvm/vmx/vmx.h:605:40: error: expected
>> > identifier or '('
>> > typedef union ept_qual __transparent__ {
>> >^
>> > /local/xen.git/xen/include/asm/hvm/vmx/vmx.h:614:3: error: type
>> > specifier missing, defaults to 'int' [-Werror,-Wimplicit-int]
>> > } ept_qual_t;
>> >   ^~
>> > 2 errors generated.
>> > 
>> > In which case the original patch as proposed will probably do.  It turns
>> > out the presence of ept_qual_t does cause a compiler error if
>> > __transparent__ is missing from scope.
>> 
>> But then the question is what the attribute applies to in the original
>> version - the union, or just the typedef? The placement would
>> suggest the latter, so I'd again be afraid of undefined behavior. Can
>> it be moved ahead on that line?
> 
> This is what the clang folks seem to test:
> 
> https://github.com/llvm-mirror/clang/blob/master/test/Sema/transparent-union.c
>  
> 
> So I would keep it with the current semantics, to stay in line with what 
> they do.

At the risk of tripping over a future gcc change in behavior? Better
not ...

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] x86/vmx: fix build with clang 3.8.0

[Jan Beulich]

>>> On 09.02.17 at 14:42,  wrote:
> On 09/02/17 13:14, Jan Beulich wrote:
> On 09.02.17 at 14:05,  wrote:
>>> On 09/02/17 13:01, Jan Beulich wrote:
>>> On 09.02.17 at 13:49,  wrote:
> On 09/02/17 11:33, Roger Pau Monne wrote:
>> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
>> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
>> @@ -602,15 +602,16 @@ void vmx_pi_hooks_assign(struct domain *d);
>>  void vmx_pi_hooks_deassign(struct domain *d);
>>  
>>  /* EPT violation qualifications definitions */
>> -typedef union __transparent__ ept_qual {
>> +typedef union ept_qual {
> Please can we use
>
> typedef __transparent__ union ept_qual {
>
> which clang is happy with, and will help avoid problems such as the
> cper_mce_record issue in c/s f8be76e2fe
 Would clang also be happy with it moved near the end of that
 line

 typedef union ept_qual __transparent__ {

 Having the attribute ahead of "union" is, I think, strictly speaking
 undefined behavior, as it then may as well apply to "typedef".
>>> No.  The result is
>>>
>>> /local/xen.git/xen/include/asm/hvm/vmx/vmx.h:605:40: error: expected
>>> identifier or '('
>>> typedef union ept_qual __transparent__ {
>>>^
>>> /local/xen.git/xen/include/asm/hvm/vmx/vmx.h:614:3: error: type
>>> specifier missing, defaults to 'int' [-Werror,-Wimplicit-int]
>>> } ept_qual_t;
>>>   ^~
>>> 2 errors generated.
>>>
>>> In which case the original patch as proposed will probably do.  It turns
>>> out the presence of ept_qual_t does cause a compiler error if
>>> __transparent__ is missing from scope.
>> But then the question is what the attribute applies to in the original
>> version - the union, or just the typedef? The placement would
>> suggest the latter, so I'd again be afraid of undefined behavior. Can
>> it be moved ahead on that line?
> 
> You mean this?
> 
> } __transparent__ ept_qual_t;

Yes.

> Clang is happy with that.

Good.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] x86/vmx: fix build with clang 3.8.0

[Roger Pau Monne]

On Thu, Feb 09, 2017 at 06:14:54AM -0700, Jan Beulich wrote:
> >>> On 09.02.17 at 14:05,  wrote:
> > On 09/02/17 13:01, Jan Beulich wrote:
> > On 09.02.17 at 13:49,  wrote:
> >>> On 09/02/17 11:33, Roger Pau Monne wrote:
>  --- a/xen/include/asm-x86/hvm/vmx/vmx.h
>  +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
>  @@ -602,15 +602,16 @@ void vmx_pi_hooks_assign(struct domain *d);
>   void vmx_pi_hooks_deassign(struct domain *d);
>   
>   /* EPT violation qualifications definitions */
>  -typedef union __transparent__ ept_qual {
>  +typedef union ept_qual {
> >>> Please can we use
> >>>
> >>> typedef __transparent__ union ept_qual {
> >>>
> >>> which clang is happy with, and will help avoid problems such as the
> >>> cper_mce_record issue in c/s f8be76e2fe
> >> Would clang also be happy with it moved near the end of that
> >> line
> >>
> >> typedef union ept_qual __transparent__ {
> >>
> >> Having the attribute ahead of "union" is, I think, strictly speaking
> >> undefined behavior, as it then may as well apply to "typedef".
> > 
> > No.  The result is
> > 
> > /local/xen.git/xen/include/asm/hvm/vmx/vmx.h:605:40: error: expected
> > identifier or '('
> > typedef union ept_qual __transparent__ {
> >^
> > /local/xen.git/xen/include/asm/hvm/vmx/vmx.h:614:3: error: type
> > specifier missing, defaults to 'int' [-Werror,-Wimplicit-int]
> > } ept_qual_t;
> >   ^~
> > 2 errors generated.
> > 
> > In which case the original patch as proposed will probably do.  It turns
> > out the presence of ept_qual_t does cause a compiler error if
> > __transparent__ is missing from scope.
> 
> But then the question is what the attribute applies to in the original
> version - the union, or just the typedef? The placement would
> suggest the latter, so I'd again be afraid of undefined behavior. Can
> it be moved ahead on that line?

This is what the clang folks seem to test:

https://github.com/llvm-mirror/clang/blob/master/test/Sema/transparent-union.c

So I would keep it with the current semantics, to stay in line with what they
do.

Roger.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] x86/vmx: fix build with clang 3.8.0

[Andrew Cooper]

On 09/02/17 13:14, Jan Beulich wrote:
 On 09.02.17 at 14:05,  wrote:
>> On 09/02/17 13:01, Jan Beulich wrote:
>> On 09.02.17 at 13:49,  wrote:
 On 09/02/17 11:33, Roger Pau Monne wrote:
> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
> @@ -602,15 +602,16 @@ void vmx_pi_hooks_assign(struct domain *d);
>  void vmx_pi_hooks_deassign(struct domain *d);
>  
>  /* EPT violation qualifications definitions */
> -typedef union __transparent__ ept_qual {
> +typedef union ept_qual {
 Please can we use

 typedef __transparent__ union ept_qual {

 which clang is happy with, and will help avoid problems such as the
 cper_mce_record issue in c/s f8be76e2fe
>>> Would clang also be happy with it moved near the end of that
>>> line
>>>
>>> typedef union ept_qual __transparent__ {
>>>
>>> Having the attribute ahead of "union" is, I think, strictly speaking
>>> undefined behavior, as it then may as well apply to "typedef".
>> No.  The result is
>>
>> /local/xen.git/xen/include/asm/hvm/vmx/vmx.h:605:40: error: expected
>> identifier or '('
>> typedef union ept_qual __transparent__ {
>>^
>> /local/xen.git/xen/include/asm/hvm/vmx/vmx.h:614:3: error: type
>> specifier missing, defaults to 'int' [-Werror,-Wimplicit-int]
>> } ept_qual_t;
>>   ^~
>> 2 errors generated.
>>
>> In which case the original patch as proposed will probably do.  It turns
>> out the presence of ept_qual_t does cause a compiler error if
>> __transparent__ is missing from scope.
> But then the question is what the attribute applies to in the original
> version - the union, or just the typedef? The placement would
> suggest the latter, so I'd again be afraid of undefined behavior. Can
> it be moved ahead on that line?

You mean this?

} __transparent__ ept_qual_t;

Clang is happy with that.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] x86/vmx: fix build with clang 3.8.0

[Jan Beulich]

>>> On 09.02.17 at 14:05,  wrote:
> On 09/02/17 13:01, Jan Beulich wrote:
> On 09.02.17 at 13:49,  wrote:
>>> On 09/02/17 11:33, Roger Pau Monne wrote:
 --- a/xen/include/asm-x86/hvm/vmx/vmx.h
 +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
 @@ -602,15 +602,16 @@ void vmx_pi_hooks_assign(struct domain *d);
  void vmx_pi_hooks_deassign(struct domain *d);
  
  /* EPT violation qualifications definitions */
 -typedef union __transparent__ ept_qual {
 +typedef union ept_qual {
>>> Please can we use
>>>
>>> typedef __transparent__ union ept_qual {
>>>
>>> which clang is happy with, and will help avoid problems such as the
>>> cper_mce_record issue in c/s f8be76e2fe
>> Would clang also be happy with it moved near the end of that
>> line
>>
>> typedef union ept_qual __transparent__ {
>>
>> Having the attribute ahead of "union" is, I think, strictly speaking
>> undefined behavior, as it then may as well apply to "typedef".
> 
> No.  The result is
> 
> /local/xen.git/xen/include/asm/hvm/vmx/vmx.h:605:40: error: expected
> identifier or '('
> typedef union ept_qual __transparent__ {
>^
> /local/xen.git/xen/include/asm/hvm/vmx/vmx.h:614:3: error: type
> specifier missing, defaults to 'int' [-Werror,-Wimplicit-int]
> } ept_qual_t;
>   ^~
> 2 errors generated.
> 
> In which case the original patch as proposed will probably do.  It turns
> out the presence of ept_qual_t does cause a compiler error if
> __transparent__ is missing from scope.

But then the question is what the attribute applies to in the original
version - the union, or just the typedef? The placement would
suggest the latter, so I'd again be afraid of undefined behavior. Can
it be moved ahead on that line?

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] x86/vmx: fix build with clang 3.8.0

[Andrew Cooper]

On 09/02/17 13:01, Jan Beulich wrote:
 On 09.02.17 at 13:49,  wrote:
>> On 09/02/17 11:33, Roger Pau Monne wrote:
>>> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
>>> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
>>> @@ -602,15 +602,16 @@ void vmx_pi_hooks_assign(struct domain *d);
>>>  void vmx_pi_hooks_deassign(struct domain *d);
>>>  
>>>  /* EPT violation qualifications definitions */
>>> -typedef union __transparent__ ept_qual {
>>> +typedef union ept_qual {
>> Please can we use
>>
>> typedef __transparent__ union ept_qual {
>>
>> which clang is happy with, and will help avoid problems such as the
>> cper_mce_record issue in c/s f8be76e2fe
> Would clang also be happy with it moved near the end of that
> line
>
> typedef union ept_qual __transparent__ {
>
> Having the attribute ahead of "union" is, I think, strictly speaking
> undefined behavior, as it then may as well apply to "typedef".

No.  The result is

/local/xen.git/xen/include/asm/hvm/vmx/vmx.h:605:40: error: expected
identifier or '('
typedef union ept_qual __transparent__ {
   ^
/local/xen.git/xen/include/asm/hvm/vmx/vmx.h:614:3: error: type
specifier missing, defaults to 'int' [-Werror,-Wimplicit-int]
} ept_qual_t;
  ^~
2 errors generated.

In which case the original patch as proposed will probably do.  It turns
out the presence of ept_qual_t does cause a compiler error if
__transparent__ is missing from scope.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] x86/vmx: fix build with clang 3.8.0

[Jan Beulich]

>>> On 09.02.17 at 13:49,  wrote:
> On 09/02/17 11:33, Roger Pau Monne wrote:
>> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
>> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
>> @@ -602,15 +602,16 @@ void vmx_pi_hooks_assign(struct domain *d);
>>  void vmx_pi_hooks_deassign(struct domain *d);
>>  
>>  /* EPT violation qualifications definitions */
>> -typedef union __transparent__ ept_qual {
>> +typedef union ept_qual {
> 
> Please can we use
> 
> typedef __transparent__ union ept_qual {
> 
> which clang is happy with, and will help avoid problems such as the
> cper_mce_record issue in c/s f8be76e2fe

Would clang also be happy with it moved near the end of that
line

typedef union ept_qual __transparent__ {

Having the attribute ahead of "union" is, I think, strictly speaking
undefined behavior, as it then may as well apply to "typedef".

Jan



___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] x86/vmx: fix build with clang 3.8.0

[Andrew Cooper]

On 09/02/17 11:33, Roger Pau Monne wrote:
> The usage of the __transparent__ attribute in 991033fa introduces some issues
> when compiled with clang 3.8.0:
>
> xen/include/asm/hvm/vmx/vmx.h:605:15: error: transparent_union attribute can 
> only be
>   applied to a union definition; attribute ignored 
> [-Werror,-Wignored-attributes]
> typedef union __transparent__ ept_qual {
>   ^
> xen/include/xen/compiler.h:50:44: note: expanded from macro '__transparent__'
> #define __transparent__ __attribute__((__transparent_union__))
>
> This can be easily fixed by moving the attribute to the end of the definition,
> but then the following error triggers:
>
> xen/include/asm/hvm/vmx/vmx.h:607:5: error: size of field '' (16 bits) does 
> not
>   match the size of the first field in transparent union; 
> transparent_union attribute ignored
>   [-Werror,-Wignored-attributes]
> struct {
> ^
> xen/include/asm/hvm/vmx/vmx.h:606:19: note: size of first field is 64 bits
> unsigned long raw;
>   ^
>
> Which can be fixed by introducing a new field in the nested structure that
> contains the padding in order to match the size of an unsigned long.
>
> Signed-off-by: Roger Pau Monné 

Sorry about that.  I could have sworn I build tested with clang, but it
appears I didn't.

> ---
> Cc: Jun Nakajima 
> Cc: Kevin Tian 
> Cc: Jan Beulich 
> Cc: Andrew Cooper 
> ---
>  xen/include/asm-x86/hvm/vmx/vmx.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h 
> b/xen/include/asm-x86/hvm/vmx/vmx.h
> index 5f7512b..52646b7 100644
> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
> @@ -602,15 +602,16 @@ void vmx_pi_hooks_assign(struct domain *d);
>  void vmx_pi_hooks_deassign(struct domain *d);
>  
>  /* EPT violation qualifications definitions */
> -typedef union __transparent__ ept_qual {
> +typedef union ept_qual {

Please can we use

typedef __transparent__ union ept_qual {

which clang is happy with, and will help avoid problems such as the
cper_mce_record issue in c/s f8be76e2fe

Otherwise, Reviewed-by: Andrew Cooper  and I
can fix this up on commit.

~Andrew

>  unsigned long raw;
>  struct {
>  bool read:1, write:1, fetch:1,
>  eff_read:1, eff_write:1, eff_exec:1, /* eff_user_exec */:1,
>  gla_valid:1,
>  gla_fault:1; /* Valid iff gla_valid. */
> +unsigned long /* pad */:55;
>  };
> -} ept_qual_t;
> +} ept_qual_t __transparent__;
>  
>  #define EPT_L4_PAGETABLE_SHIFT  39
>  #define EPT_PAGETABLE_ENTRIES   512


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel