Mailing List Archive: [PATCH] Fix non-debug build after c/s 23767:80e9fcdaef36

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

Aug 22, 2011, 6:23 AM

Post #1 of 8 (328 views)

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

Re: [PATCH] Fix non-debug build after c/s 23767:80e9fcdaef36 [ In reply to ]

keir.xen at gmail

Aug 22, 2011, 6:35 AM

Post #2 of 8 (328 views)

Permalink

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

Re: [PATCH] Fix non-debug build after c/s 23767:80e9fcdaef36 [ In reply to ]

George.Dunlap at eu

Aug 22, 2011, 7:02 AM

Post #3 of 8 (329 views)

Permalink

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

Re: [PATCH] Fix non-debug build after c/s 23767:80e9fcdaef36 [ In reply to ]

tim at xen

Aug 22, 2011, 7:07 AM

Post #4 of 8 (328 views)

Permalink

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

Re: [PATCH] Fix non-debug build after c/s 23767:80e9fcdaef36 [ In reply to ]

JBeulich at novell

Aug 22, 2011, 7:09 AM

Post #5 of 8 (329 views)

Permalink

>>> 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

Re: [PATCH] Fix non-debug build after c/s 23767:80e9fcdaef36 [ In reply to ]

JBeulich at novell

Aug 22, 2011, 7:12 AM

Post #6 of 8 (328 views)

Permalink

>>> 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

Re: [PATCH] Fix non-debug build after c/s 23767:80e9fcdaef36 [ In reply to ]

keir.xen at gmail

Aug 22, 2011, 7:18 AM

Post #7 of 8 (326 views)

Permalink

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

Re: [PATCH] Fix non-debug build after c/s 23767:80e9fcdaef36 [ In reply to ]

george.dunlap at citrix

Aug 22, 2011, 7:44 AM

Post #8 of 8 (327 views)

Permalink

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