translate-all: wrapped map_exec() in #ifdef

On 31 October 2014 04:59, SeokYeon Hwang <syeon.hwang@samsung.com> wrote:
> Moved map_exec() and wrapped it in #ifdef to avoid "-Wunused-function" on clang 3.4 or later.
>
> Signed-off-by: SeokYeon Hwang <syeon.hwang@samsung.com>

I had this kind of on my todo list too, but I didn't much like
the nested ifdefs which are really only because of what the
different implementations of alloc_code_gen_buffer() happen
to do. I think it would be more robust to just mark the
functions with the 'unused' attribute instead of relying on
'inline' to implicitly do this for us:

thanks
-- PMM

I posted a new patch which your suggestion is applied.

Thanks.

> -----Original Message-----
> From: Peter Maydell [mailto:peter.maydell@linaro.org]
> Sent: Saturday, November 01, 2014 3:30 AM
> To: SeokYeon Hwang
> Cc: QEMU Developers
> Subject: Re: [Qemu-devel] [PATCH] translate-all: wrapped map_exec() in
> #ifdef
> 
> On 31 October 2014 04:59, SeokYeon Hwang <syeon.hwang@samsung.com> wrote:
> > Moved map_exec() and wrapped it in #ifdef to avoid "-Wunused-function"
> on clang 3.4 or later.
> >
> > Signed-off-by: SeokYeon Hwang <syeon.hwang@samsung.com>
> 
> I had this kind of on my todo list too, but I didn't much like the nested
> ifdefs which are really only because of what the different implementations
> of alloc_code_gen_buffer() happen to do. I think it would be more robust
> to just mark the functions with the 'unused' attribute instead of relying
> on 'inline' to implicitly do this for us:
> 
> --- 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);  }  #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;
> 
> thanks
> -- PMM

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.

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

-- PMM

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

On 3 November 2014 14:27, Michael Tokarev <mjt@tls.msk.ru> wrote:

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

You could equally ask why we've marked the function "inline"
for both, when it's currently only required for non-win32
and only needed because it silences the warning on gcc.

> 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.. :)

Yeah, that's the "close coupling between warning-suppressing
and the implementation of a different bit of the code" approach.
As you say, personal taste; I've applied my r-by tag to the
other one ;-)

-- PMM