Xen devel - Aug 2011 - [PATCH] Fix non-debug build after c/s 23767:80e9fcdaef36

The hd variable in iommu_update_pde_count() and iommu_merge_pages() is
only used in the ASSERT, causing gcc to complain about the variable being
unused in non-debug builds.

Only define the variable for debug builds.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

diff -r 2029263c501c -r ac4de6b4c457 xen/drivers/passthrough/amd/iommu_map.c
--- a/xen/drivers/passthrough/amd/iommu_map.c	Mon Aug 22 10:12:36 2011 +0100
+++ b/xen/drivers/passthrough/amd/iommu_map.c	Mon Aug 22 12:54:42 2011 +0100
@@ -462,7 +462,9 @@ static int iommu_update_pde_count(struct
     unsigned long first_mfn;
     u64 *table, *pde, *ntable;
     u64 ntable_maddr, mask;
+#ifndef NDEBUG
     struct hvm_iommu *hd = domain_hvm_iommu(d);
+#endif
     bool_t ok = 0;
 
     ASSERT( spin_is_locked(&hd->mapping_lock) && pt_mfn );
@@ -517,7 +519,9 @@ static int iommu_merge_pages(struct doma
     u64 *table, *pde, *ntable;
     u64 ntable_mfn;
     unsigned long first_mfn;
+#ifndef NDEBUG
     struct hvm_iommu *hd = domain_hvm_iommu(d);
+#endif
 
     ASSERT( spin_is_locked(&hd->mapping_lock) && pt_mfn );
 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

George,

Would something like this work more generically for the non-debug case?

#define ASSERT(p) do { if (p); } while (0)

 -- Keir

On 22/08/2011 14:23, "George Dunlap"
<george.dunlap@eu.citrix.com> wrote:
> The hd variable in iommu_update_pde_count() and iommu_merge_pages() is
> only used in the ASSERT, causing gcc to complain about the variable being
> unused in non-debug builds.
> 
> Only define the variable for debug builds.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> 
> diff -r 2029263c501c -r ac4de6b4c457
xen/drivers/passthrough/amd/iommu_map.c
> --- a/xen/drivers/passthrough/amd/iommu_map.c Mon Aug 22 10:12:36 2011
+0100
> +++ b/xen/drivers/passthrough/amd/iommu_map.c Mon Aug 22 12:54:42 2011
+0100
> @@ -462,7 +462,9 @@ static int iommu_update_pde_count(struct
>      unsigned long first_mfn;
>      u64 *table, *pde, *ntable;
>      u64 ntable_maddr, mask;
> +#ifndef NDEBUG
>      struct hvm_iommu *hd = domain_hvm_iommu(d);
> +#endif
>      bool_t ok = 0;
>  
>      ASSERT( spin_is_locked(&hd->mapping_lock) && pt_mfn );
> @@ -517,7 +519,9 @@ static int iommu_merge_pages(struct doma
>      u64 *table, *pde, *ntable;
>      u64 ntable_mfn;
>      unsigned long first_mfn;
> +#ifndef NDEBUG
>      struct hvm_iommu *hd = domain_hvm_iommu(d);
> +#endif
>  
>      ASSERT( spin_is_locked(&hd->mapping_lock) && pt_mfn );
>  
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

That seems to compile just fine, and obviously makes the code a lot
cleaner.  The only thing is that this will still cause functions
inside ASSERTS (spin_is_locked() in this case) to be called; I thought
part of the reason for having ASSERTs in the debug build only was to
reduce the cost of all the checks (the other reason to avoid
unnecessary crashes on production builds)?

 -George

On Mon, Aug 22, 2011 at 2:35 PM, Keir Fraser <keir.xen@gmail.com>
wrote:
> George,
>
> Would something like this work more generically for the non-debug case?
>
> #define ASSERT(p) do { if (p); } while (0)
>
>  -- Keir
>
> On 22/08/2011 14:23, "George Dunlap"
<george.dunlap@eu.citrix.com> wrote:
>
>> The hd variable in iommu_update_pde_count() and iommu_merge_pages() is
>> only used in the ASSERT, causing gcc to complain about the variable
being
>> unused in non-debug builds.
>>
>> Only define the variable for debug builds.
>>
>> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
>>
>> diff -r 2029263c501c -r ac4de6b4c457
xen/drivers/passthrough/amd/iommu_map.c
>> --- a/xen/drivers/passthrough/amd/iommu_map.c Mon Aug 22 10:12:36 2011
+0100
>> +++ b/xen/drivers/passthrough/amd/iommu_map.c Mon Aug 22 12:54:42 2011
+0100
>> @@ -462,7 +462,9 @@ static int iommu_update_pde_count(struct
>>      unsigned long first_mfn;
>>      u64 *table, *pde, *ntable;
>>      u64 ntable_maddr, mask;
>> +#ifndef NDEBUG
>>      struct hvm_iommu *hd = domain_hvm_iommu(d);
>> +#endif
>>      bool_t ok = 0;
>>
>>      ASSERT( spin_is_locked(&hd->mapping_lock) && pt_mfn
);
>> @@ -517,7 +519,9 @@ static int iommu_merge_pages(struct doma
>>      u64 *table, *pde, *ntable;
>>      u64 ntable_mfn;
>>      unsigned long first_mfn;
>> +#ifndef NDEBUG
>>      struct hvm_iommu *hd = domain_hvm_iommu(d);
>> +#endif
>>
>>      ASSERT( spin_is_locked(&hd->mapping_lock) && pt_mfn
);
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

At 15:02 +0100 on 22 Aug (1314025327), George Dunlap
wrote:
> That seems to compile just fine, and obviously makes the code a lot
> cleaner.  The only thing is that this will still cause functions
> inside ASSERTS (spin_is_locked() in this case) to be called; I thought
> part of the reason for having ASSERTs in the debug build only was to
> reduce the cost of all the checks (the other reason to avoid
> unnecessary crashes on production builds)?
How about #define ASSERT(p) do { if (0 && (p)); } while (0) ?

Tim.
> On Mon, Aug 22, 2011 at 2:35 PM, Keir Fraser <keir.xen@gmail.com>
wrote:
> > George,
> >
> > Would something like this work more generically for the non-debug
case?
> >
> > #define ASSERT(p) do { if (p); } while (0)
> >
> >  -- Keir
> >
-- 
Tim Deegan <tim@xen.org>
Principal Software Engineer, Xen Platform Team
Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> On 22.08.11 at 15:35, Keir Fraser <keir.xen@gmail.com> wrote:
> George,
> 
> Would something like this work more generically for the non-debug case?
> 
> #define ASSERT(p) do { if (p); } while (0)
I''d expect this to work, but the general expectation is that ASSERT()
behaves as it does now (i.e. not evaluating its argument in NDEBUG
builds), and making Xen deviate from that can certainly be expected to
cause problems down the road (and would require auditing of all
current uses of ASSERT()).

Jan
>  -- Keir
> 
> On 22/08/2011 14:23, "George Dunlap"
<george.dunlap@eu.citrix.com> wrote:
> 
>> The hd variable in iommu_update_pde_count() and iommu_merge_pages() is
>> only used in the ASSERT, causing gcc to complain about the variable
being
>> unused in non-debug builds.
>> 
>> Only define the variable for debug builds.
>> 
>> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
>> 
>> diff -r 2029263c501c -r ac4de6b4c457
xen/drivers/passthrough/amd/iommu_map.c
>> --- a/xen/drivers/passthrough/amd/iommu_map.c Mon Aug 22 10:12:36 2011
+0100
>> +++ b/xen/drivers/passthrough/amd/iommu_map.c Mon Aug 22 12:54:42 2011
+0100
>> @@ -462,7 +462,9 @@ static int iommu_update_pde_count(struct
>>      unsigned long first_mfn;
>>      u64 *table, *pde, *ntable;
>>      u64 ntable_maddr, mask;
>> +#ifndef NDEBUG
>>      struct hvm_iommu *hd = domain_hvm_iommu(d);
>> +#endif
>>      bool_t ok = 0;
>>  
>>      ASSERT( spin_is_locked(&hd->mapping_lock) && pt_mfn
);
>> @@ -517,7 +519,9 @@ static int iommu_merge_pages(struct doma
>>      u64 *table, *pde, *ntable;
>>      u64 ntable_mfn;
>>      unsigned long first_mfn;
>> +#ifndef NDEBUG
>>      struct hvm_iommu *hd = domain_hvm_iommu(d);
>> +#endif
>>  
>>      ASSERT( spin_is_locked(&hd->mapping_lock) && pt_mfn
);
>>  
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com 
>> http://lists.xensource.com/xen-devel 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com 
> http://lists.xensource.com/xen-devel 



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> On 22.08.11 at 16:07, Tim Deegan <tim@xen.org> wrote:
> At 15:02 +0100 on 22 Aug (1314025327), George Dunlap wrote:
>> That seems to compile just fine, and obviously makes the code a lot
>> cleaner.  The only thing is that this will still cause functions
>> inside ASSERTS (spin_is_locked() in this case) to be called; I thought
>> part of the reason for having ASSERTs in the debug build only was to
>> reduce the cost of all the checks (the other reason to avoid
>> unnecessary crashes on production builds)?
> 
> How about #define ASSERT(p) do { if (0 && (p)); } while (0) ?
That seems reasonable to me (but doesn''t guarantee a more clever
compiler would notice that any variable read only in ASSERT()
expressions may now be initialized but never used - doesn''t gcc 4.6
already do this to some degree?).

Jan
> Tim.
> 
>> On Mon, Aug 22, 2011 at 2:35 PM, Keir Fraser <keir.xen@gmail.com>
wrote:
>> > George,
>> >
>> > Would something like this work more generically for the non-debug
case?
>> >
>> > #define ASSERT(p) do { if (p); } while (0)
>> >
>> >  -- Keir
>> >




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 22/08/2011 15:07, "Tim Deegan" <tim@xen.org> wrote:
> At 15:02 +0100 on 22 Aug (1314025327), George Dunlap wrote:
>> That seems to compile just fine, and obviously makes the code a lot
>> cleaner.  The only thing is that this will still cause functions
>> inside ASSERTS (spin_is_locked() in this case) to be called; I thought
>> part of the reason for having ASSERTs in the debug build only was to
>> reduce the cost of all the checks (the other reason to avoid
>> unnecessary crashes on production builds)?
> 
> How about #define ASSERT(p) do { if (0 && (p)); } while (0) ?
Yes, if this also works then it is preferable.

 -- Keir
> Tim.
> 
>> On Mon, Aug 22, 2011 at 2:35 PM, Keir Fraser <keir.xen@gmail.com>
wrote:
>>> George,
>>> 
>>> Would something like this work more generically for the non-debug
case?
>>> 
>>> #define ASSERT(p) do { if (p); } while (0)
>>> 
>>>  -- Keir
>>> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Mon, 2011-08-22 at 15:07 +0100, Tim Deegan wrote:
> At 15:02 +0100 on 22 Aug (1314025327), George Dunlap wrote:
> > That seems to compile just fine, and obviously makes the code a lot
> > cleaner.  The only thing is that this will still cause functions
> > inside ASSERTS (spin_is_locked() in this case) to be called; I thought
> > part of the reason for having ASSERTs in the debug build only was to
> > reduce the cost of all the checks (the other reason to avoid
> > unnecessary crashes on production builds)?
> 
> How about #define ASSERT(p) do { if (0 && (p)); } while (0) ?
That makes at least two different compilers happy, and results in no
code to call spin_is_locked().

 -George
> 
> Tim.
> 
> > On Mon, Aug 22, 2011 at 2:35 PM, Keir Fraser
<keir.xen@gmail.com> wrote:
> > > George,
> > >
> > > Would something like this work more generically for the non-debug
case?
> > >
> > > #define ASSERT(p) do { if (p); } while (0)
> > >
> > >  -- Keir
> > >
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel