Re: [Xen-devel] [PATCH v3 09/24] x86/emul: Provide a wrapper to x86_emulate() to ASSERT() certain behaviour

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 01/12/16 10:40, Jan Beulich wrote:
>
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -2404,6 +2404,11 @@ x86_decode(
>>  #undef insn_fetch_bytes
>>  #undef insn_fetch_type
>>  
>> +/* Undo DEBUG wrapper. */
>> +#ifdef x86_emulate
>> +#undef x86_emulate
>> +#endif
> I don't see the need for the #ifdef here.

It will break the non-debug build if removed, as x86_emulate wouldn't be
a define.

>
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.h
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h
>> @@ -23,6 +23,10 @@
>>  #ifndef __X86_EMULATE_H__
>>  #define __X86_EMULATE_H__
>>  
>> +#ifndef ASSERT
>> +#define ASSERT assert
>> +#endif
> This doesn't seem to belong here (as there's nothing making sure
> assert is defined), and duplicates an existing #define in the test
> harness'es x86_emulate.c. I could agree to deleting that other one
> and wrapping the one here with #ifndef __XEN__.

Ok.

>
>> @@ -554,6 +558,27 @@ x86_emulate(
>>      const struct x86_emulate_ops *ops);
>>  
>>  /*
>> + * In debug builds, wrap x86_emulate() with some assertions about its 
>> expected
>> + * behaviour.
>> + */
>> +#ifndef NDEBUG
> Mind swapping the order of comment and #ifndef, to make it more
> reasonable to possibly add further things into this guarded block?

Ok.

~Andrew

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