| Message ID | 20170209113312.63204-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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é <roger.pau@citrix.com> Sorry about that. I could have sworn I build tested with clang, but it appears I didn't. > --- > Cc: Jun Nakajima <jun.nakajima@intel.com> > Cc: Kevin Tian <kevin.tian@intel.com> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > --- > 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 <andrew.cooper3@citrix.com> 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
>>> On 09.02.17 at 13:49, <andrew.cooper3@citrix.com> 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
On 09/02/17 13:01, Jan Beulich wrote: >>>> On 09.02.17 at 13:49, <andrew.cooper3@citrix.com> 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
>>> On 09.02.17 at 14:05, <andrew.cooper3@citrix.com> wrote: > On 09/02/17 13:01, Jan Beulich wrote: >>>>> On 09.02.17 at 13:49, <andrew.cooper3@citrix.com> 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
On 09/02/17 13:14, Jan Beulich wrote: >>>> On 09.02.17 at 14:05, <andrew.cooper3@citrix.com> wrote: >> On 09/02/17 13:01, Jan Beulich wrote: >>>>>> On 09.02.17 at 13:49, <andrew.cooper3@citrix.com> 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
On Thu, Feb 09, 2017 at 06:14:54AM -0700, Jan Beulich wrote: > >>> On 09.02.17 at 14:05, <andrew.cooper3@citrix.com> wrote: > > On 09/02/17 13:01, Jan Beulich wrote: > >>>>> On 09.02.17 at 13:49, <andrew.cooper3@citrix.com> 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.
>>> On 09.02.17 at 14:42, <andrew.cooper3@citrix.com> wrote: > On 09/02/17 13:14, Jan Beulich wrote: >>>>> On 09.02.17 at 14:05, <andrew.cooper3@citrix.com> wrote: >>> On 09/02/17 13:01, Jan Beulich wrote: >>>>>>> On 09.02.17 at 13:49, <andrew.cooper3@citrix.com> 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
>>> On 09.02.17 at 14:45, <roger.pau@citrix.com> wrote: > On Thu, Feb 09, 2017 at 06:14:54AM -0700, Jan Beulich wrote: >> >>> On 09.02.17 at 14:05, <andrew.cooper3@citrix.com> wrote: >> > On 09/02/17 13:01, Jan Beulich wrote: >> >>>>> On 09.02.17 at 13:49, <andrew.cooper3@citrix.com> 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
On 09/02/17 14:20, Jan Beulich wrote: >>>> On 09.02.17 at 14:42, <andrew.cooper3@citrix.com> wrote: >> On 09/02/17 13:14, Jan Beulich wrote: >>>>>> On 09.02.17 at 14:05, <andrew.cooper3@citrix.com> wrote: >>>> On 09/02/17 13:01, Jan Beulich wrote: >>>>>>>> On 09.02.17 at 13:49, <andrew.cooper3@citrix.com> 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
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 { 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
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é <roger.pau@citrix.com> --- Cc: Jun Nakajima <jun.nakajima@intel.com> Cc: Kevin Tian <kevin.tian@intel.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> --- xen/include/asm-x86/hvm/vmx/vmx.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)