Re: [Qemu-devel] [PATCH] translate-all: wrapped map_exec() in #ifdef

[Michael Tokarev <mjt@tls.msk.ru>]

03.11.2014 15:25, Peter Maydell wrote:
> On 3 November 2014 09:16, Michael Tokarev <mjt@tls.msk.ru> wrote:
>> 03.11.2014 11:18, SeokYeon Hwang wrote:
>>>> --- a/translate-all.c
>>>> +++ b/translate-all.c
>>>> @@ -270,14 +270,14 @@ bool cpu_restore_state(CPUState *cpu, uintptr_t
>>>> retaddr)  }
>>>>
>>>>  #ifdef _WIN32
>>>> -static inline void map_exec(void *addr, long size)
>>>> +static __attribute__((unused)) void map_exec(void *addr, long size)
>>>>  {
>>>>      DWORD old_protect;
>>>>      VirtualProtect(addr, size,
>>>>                     PAGE_EXECUTE_READWRITE, &old_protect);  }
>>
>> In which case this function isn't used on windows?  I mean, is it really
>> necessary to mark it as unused for win32 case?
> 
> It's not necessary, but it means we're consistent between
> windows and not-windows. It avoids unnecessary coupling between
> how the calling function happens to be implemented and
> what defines we put around this utility function.

I usually reluctant to add these unused attributes and shut up other
warnings by force, because with time this cruft does not help, and
more and more code becomes "forgotten".  If it is not needed for
windows, why add it?

>>>> #else
>>>> -static inline void map_exec(void *addr, long size)
>>>> +static __attribute__((unused)) void map_exec(void *addr, long size)
>>>>  {
>>>>      unsigned long start, end, page_size;
>>
>> How about this instead:
>>
>> --- a/translate-all.c
>> +++ b/translate-all.c
>> @@ -276,7 +276,7 @@ static inline void map_exec(void *addr, long size)
>>      VirtualProtect(addr, size,
>>                     PAGE_EXECUTE_READWRITE, &old_protect);
>>  }
>> -#else
>> +#elif !defined(USE_MMAP)
>>  static inline void map_exec(void *addr, long size)
>>  {
>>      unsigned long start, end, page_size;
>>
>> ? (Untested, but just to show an idea)... ;)
> 
> This breaks for linux-user, where we will define both
> USE_STATIC_CODE_GEN_BUFFER and USE_MMAP, and your change won't
> define map_exec() but the later ifdef ladder will pick a version
> of alloc_code_gen_buffer() that calls it.

Hence the "untested" warning by me... So the test can be rewritten
like this:

#elif defined(USE_STATIC_CODE_GEN_BUFFER) || !defined(USE_MMAP)

(untested again! ;) and everything should be well without adding
extra attributes...  Which one is better is, well, a matter of
personal taste really.. :)

(Not that all this is hugely important, we already spent too much
time discussing various trivial/unimportant things and had much
less time to do the real work, but... ;)

Thanks,

/mjt