| Message ID | 20170407143453.46310-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 7 Apr 2017, Roger Pau Monne wrote: ... > + return xsm_default_action(XSM_TARGET, current->domain, d); > case XEN_ALTP2M_external: > - a = XSM_DM_PRIV; > + return xsm_default_action(XSM_DM_PRIV, current->domain, d); > break; > case XEN_ALTP2M_limited: It looks like you left a break; in there which will never be reached. Michael Young
>>> On 07.04.17 at 16:34, <roger.pau@citrix.com> wrote: > The changes introduced on c47d1d broke the clang build due to undefined > references to __xsm_action_mismatch_detected, because clang hasn't optimized > the code properly. This starts to become annoying. > --- a/xen/include/xsm/dummy.h > +++ b/xen/include/xsm/dummy.h > @@ -557,25 +557,23 @@ static XSM_INLINE int xsm_hvm_param_altp2mhvm(XSM_DEFAULT_ARG struct domain *d) > > static XSM_INLINE int xsm_hvm_altp2mhvm_op(XSM_DEFAULT_ARG struct domain *d, uint64_t mode, uint32_t op) > { > - xsm_default_t a; > XSM_ASSERT_ACTION(XSM_OTHER); > > switch ( mode ) > { > case XEN_ALTP2M_mixed: > - a = XSM_TARGET; > - break; > + return xsm_default_action(XSM_TARGET, current->domain, d); > case XEN_ALTP2M_external: > - a = XSM_DM_PRIV; > + return xsm_default_action(XSM_DM_PRIV, current->domain, d); > break; Just like above, this break wants to be eliminated. > case XEN_ALTP2M_limited: > - a = (HVMOP_altp2m_vcpu_enable_notify == op) ? XSM_TARGET : XSM_DM_PRIV; > - break; > + if ( HVMOP_altp2m_vcpu_enable_notify == op ) > + return xsm_default_action(XSM_TARGET, current->domain, d); > + else > + return xsm_default_action(XSM_DM_PRIV, current->domain, d); Could you either keep the conditional expression at least (as a direct function parameter), or if that still doesn't work at the minimum drop the pointless "else"? Jan
Hi, On 04/07/2017 03:54 PM, Jan Beulich wrote: >>>> On 07.04.17 at 16:34, <roger.pau@citrix.com> wrote: >> The changes introduced on c47d1d broke the clang build due to undefined >> references to __xsm_action_mismatch_detected, because clang hasn't optimized >> the code properly. > > This starts to become annoying. This is also breaking ARM32 build with GCC (4.9 and 5.4) (see [1]): ld -EL -T xen.lds -N prelink.o \ /home/osstest/build.107275.build-armhf-xsm/xen/xen/common/symbols-dummy.o -o /home/osstest/build.107275.build-armhf-xsm/xen/xen/.xen-syms.0 prelink.o: In function `xsm_default_action': /home/osstest/build.107275.build-armhf-xsm/xen/xen/include/xsm/dummy.h:80: undefined reference to `__xsm_action_mismatch_detected' gcc -DPIC -marm -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs -g3 -O0 -fno-omit-frame-pointer -D__XEN_INTERFACE_VERSION__=__XEN_LATEST_INTERFACE_VERSION__ -MMD -MF .xc_dom_boot.opic.d -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_GNU_SOURCE -I../../xen/common/libelf -Werror -Wmissing-prototypes -I. -I./include -I/home/osstest/build.107275.build-armhf-xsm/xen/tools/libxc/../../tools/include -D__XEN_TOOLS__ -pthread -I/home/osstest/build.107275.build-armhf-xsm/xen/tools/libxc/../../tools/libs/toollog/include -I/home/osstest/build.107275.build-armhf-xsm/xen/tools/libxc/../../tools/include -I/home/osstest/build.107275.build-armhf-xsm/xen/tools/libxc/../../tools/libs/evtchn/include -I/home/osstest/build.107275.build-armhf-xsm/xen/tools/libxc/../../tools/include -I/home/osstest/build.107275.build-armhf-xsm/xen/tools/libxc/../../tools/libs/devicemodel/include -I/home/osstest/build.107275.build-armhf-xsm/xen/tools/libxc/../../tools/include -include /home/osstest/build.107275.build-armhf-xsm/xen/tools/libxc/../../tools/config.h -I/home/osstest/build.107275.build-armhf-xsm/xen/tools/libxc/../../tools/libs/call/include -I/home/osstest/build.107275.build-armhf-xsm/xen/tools/libxc/../../tools/include -I/home/osstest/build.107275.build-armhf-xsm/xen/tools/libxc/../../tools/libs/foreignmemory/include -I/home/osstest/build.107275.build-armhf-xsm/xen/tools/libxc/../../tools/include -fPIC -c -o xc_dom_boot.opic xc_dom_boot.c ld: /home/osstest/build.107275.build-armhf-xsm/xen/xen/.xen-syms.0: hidden symbol `__xsm_action_mismatch_detected' isn't defined ld: final link failed: Bad value make[3]: *** [/home/osstest/build.107275.build-armhf-xsm/xen/xen/xen-syms] Error 1 Makefile:96: recipe for target '/home/osstest/build.107275.build-armhf-xsm/xen/xen/xen-syms' failed make[3]: Leaving directory '/home/osstest/build.107275.build-armhf-xsm/xen/xen/arch/arm' make[2]: *** [/home/osstest/build.107275.build-armhf-xsm/xen/xen/xen] Error 2 Makefile:136: recipe for target '/home/osstest/build.107275.build-armhf-xsm/xen/xen/xen' failed make[2]: Leaving directory '/home/osstest/build.107275.build-armhf-xsm/xen/xen' Makefile:45: recipe for target 'install' failed make[1]: Leaving directory '/home/osstest/build.107275.build-armhf-xsm/xen/xen' Makefile:97: recipe for target 'install-xen' failed make[1]: *** [install] Error 2 make: *** [install-xen] Error 2 make: *** Waiting for unfinished jobs.... [1] http://logs.test-lab.xenproject.org/osstest/logs/107275/build-armhf-xsm/5.ts-xen-build.log -- Julien Grall
Hi Roger, On 04/07/2017 03:34 PM, Roger Pau Monne wrote: > The changes introduced on c47d1d broke the clang build due to undefined > references to __xsm_action_mismatch_detected, because clang hasn't optimized > the code properly. The following patch allows the clang build to work again, > while keeping the same functionality. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> FWIW: this is fixing ARM32 build with XSM (blocks osstest). Tested-by: Julien Grall <julien.grall@arm.com> Cheers, > --- > Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov> > Cc: Julien Grall <julien.grall@arm.com> > Cc: Tamas K Lengyel <tamas.lengyel@zentific.com> > --- > NB: this fixes travis build: https://travis-ci.org/royger/xen/builds/219697038 > --- > xen/include/xsm/dummy.h | 16 +++++++--------- > 1 file changed, 7 insertions(+), 9 deletions(-) > > diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h > index 56a8814d82..7d35744afe 100644 > --- a/xen/include/xsm/dummy.h > +++ b/xen/include/xsm/dummy.h > @@ -557,25 +557,23 @@ static XSM_INLINE int xsm_hvm_param_altp2mhvm(XSM_DEFAULT_ARG struct domain *d) > > static XSM_INLINE int xsm_hvm_altp2mhvm_op(XSM_DEFAULT_ARG struct domain *d, uint64_t mode, uint32_t op) > { > - xsm_default_t a; > XSM_ASSERT_ACTION(XSM_OTHER); > > switch ( mode ) > { > case XEN_ALTP2M_mixed: > - a = XSM_TARGET; > - break; > + return xsm_default_action(XSM_TARGET, current->domain, d); > case XEN_ALTP2M_external: > - a = XSM_DM_PRIV; > + return xsm_default_action(XSM_DM_PRIV, current->domain, d); > break; > case XEN_ALTP2M_limited: > - a = (HVMOP_altp2m_vcpu_enable_notify == op) ? XSM_TARGET : XSM_DM_PRIV; > - break; > + if ( HVMOP_altp2m_vcpu_enable_notify == op ) > + return xsm_default_action(XSM_TARGET, current->domain, d); > + else > + return xsm_default_action(XSM_DM_PRIV, current->domain, d); > default: > return -EPERM; > - }; > - > - return xsm_default_action(a, current->domain, d); > + } > } > > static XSM_INLINE int xsm_vm_event_control(XSM_DEFAULT_ARG struct domain *d, int mode, int op) >
>>> On 09.04.17 at 21:01, <julien.grall@arm.com> wrote: > On 04/07/2017 03:54 PM, Jan Beulich wrote: >>>>> On 07.04.17 at 16:34, <roger.pau@citrix.com> wrote: >>> The changes introduced on c47d1d broke the clang build due to undefined >>> references to __xsm_action_mismatch_detected, because clang hasn't optimized >>> the code properly. >> >> This starts to become annoying. > > This is also breaking ARM32 build with GCC (4.9 and 5.4) (see [1]): That's certainly unexpected, as we're building with -O1 even for debug builds (for this very reason). Jan
On Fri, Apr 07, 2017 at 08:54:59AM -0600, Jan Beulich wrote: > >>> On 07.04.17 at 16:34, <roger.pau@citrix.com> wrote: > > The changes introduced on c47d1d broke the clang build due to undefined > > references to __xsm_action_mismatch_detected, because clang hasn't optimized > > the code properly. > > This starts to become annoying. Yes, tell me about it... I don't think we can blame clang for those failures anyway. Xen is relying on optimizations to do compile time validations, and AFAIK this is a grey area. There's nothing in the C spec that guarantees you that those calls will be removed. Anyway, will send a new version shortly. Roger.
On 04/10/2017 04:41 AM, Roger Pau Monne wrote: > On Fri, Apr 07, 2017 at 08:54:59AM -0600, Jan Beulich wrote: >>>>> On 07.04.17 at 16:34, <roger.pau@citrix.com> wrote: >>> The changes introduced on c47d1d broke the clang build due to undefined >>> references to __xsm_action_mismatch_detected, because clang hasn't optimized >>> the code properly. >> >> This starts to become annoying. > > Yes, tell me about it... I don't think we can blame clang for those failures > anyway. Xen is relying on optimizations to do compile time validations, and > AFAIK this is a grey area. There's nothing in the C spec that guarantees you > that those calls will be removed. > > Anyway, will send a new version shortly. If this problem expands more, I think it would be best to restrict the check to a particular compiler or #define (as long as it's one used in the build bot); there's no need to do this kind of check on every build as long as it's done on occasional builds. Alternatively, it could be done by a static analysis tool, but I've not looked into how to do that with Coverity.
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h index 56a8814d82..7d35744afe 100644 --- a/xen/include/xsm/dummy.h +++ b/xen/include/xsm/dummy.h @@ -557,25 +557,23 @@ static XSM_INLINE int xsm_hvm_param_altp2mhvm(XSM_DEFAULT_ARG struct domain *d) static XSM_INLINE int xsm_hvm_altp2mhvm_op(XSM_DEFAULT_ARG struct domain *d, uint64_t mode, uint32_t op) { - xsm_default_t a; XSM_ASSERT_ACTION(XSM_OTHER); switch ( mode ) { case XEN_ALTP2M_mixed: - a = XSM_TARGET; - break; + return xsm_default_action(XSM_TARGET, current->domain, d); case XEN_ALTP2M_external: - a = XSM_DM_PRIV; + return xsm_default_action(XSM_DM_PRIV, current->domain, d); break; case XEN_ALTP2M_limited: - a = (HVMOP_altp2m_vcpu_enable_notify == op) ? XSM_TARGET : XSM_DM_PRIV; - break; + if ( HVMOP_altp2m_vcpu_enable_notify == op ) + return xsm_default_action(XSM_TARGET, current->domain, d); + else + return xsm_default_action(XSM_DM_PRIV, current->domain, d); default: return -EPERM; - }; - - return xsm_default_action(a, current->domain, d); + } } static XSM_INLINE int xsm_vm_event_control(XSM_DEFAULT_ARG struct domain *d, int mode, int op)
The changes introduced on c47d1d broke the clang build due to undefined references to __xsm_action_mismatch_detected, because clang hasn't optimized the code properly. The following patch allows the clang build to work again, while keeping the same functionality. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov> Cc: Julien Grall <julien.grall@arm.com> Cc: Tamas K Lengyel <tamas.lengyel@zentific.com> --- NB: this fixes travis build: https://travis-ci.org/royger/xen/builds/219697038 --- xen/include/xsm/dummy.h | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)