[Qemu-devel] [PATCH 2/2] Add __noreturn function attribute

[Qemu-devel] [PATCH 2/2] Add __noreturn function attribute

[Jan Kiszka]

Introduce __noreturn attribute and attach it to cpu_loop_exit as well as
interrupt/exception helpers for i386. This avoids a bunch of gcc4
warnings.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 exec-all.h         |    5 ++++-
 target-i386/exec.h |    8 ++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/exec-all.h b/exec-all.h
index ca97f57..4934932 100644
--- a/exec-all.h
+++ b/exec-all.h
@@ -20,6 +20,9 @@
 
 #ifndef _EXEC_ALL_H_
 #define _EXEC_ALL_H_
+
+#define __noreturn __attribute__ ((__noreturn__))
+
 /* allow to see translation results - the slowdown should be negligible, so we leave it */
 #define DEBUG_DISAS
 
@@ -82,7 +85,7 @@ TranslationBlock *tb_gen_code(CPUState *env,
                               target_ulong pc, target_ulong cs_base, int flags,
                               int cflags);
 void cpu_exec_init(CPUState *env);
-void cpu_loop_exit(void);
+void __noreturn cpu_loop_exit(void);
 int page_unprotect(target_ulong address, unsigned long pc, void *puc);
 void tb_invalidate_phys_page_range(target_phys_addr_t start, target_phys_addr_t end,
                                    int is_cpu_write_access);
diff --git a/target-i386/exec.h b/target-i386/exec.h
index 3663166..b35f08b 100644
--- a/target-i386/exec.h
+++ b/target-i386/exec.h
@@ -67,10 +67,10 @@ void do_interrupt(int intno, int is_int, int error_code,
                   target_ulong next_eip, int is_hw);
 void do_interrupt_user(int intno, int is_int, int error_code,
                        target_ulong next_eip);
-void raise_interrupt(int intno, int is_int, int error_code,
-                     int next_eip_addend);
-void raise_exception_err(int exception_index, int error_code);
-void raise_exception(int exception_index);
+void __noreturn raise_interrupt(int intno, int is_int, int error_code,
+                                int next_eip_addend);
+void __noreturn raise_exception_err(int exception_index, int error_code);
+void __noreturn raise_exception(int exception_index);
 void do_smm_enter(void);
 
 /* n must be a constant to be efficient */

Re: [Qemu-devel] [PATCH 2/2] Add __noreturn function attribute

[malc]

On Fri, 28 Nov 2008, Jan Kiszka wrote:

> Introduce __noreturn attribute and attach it to cpu_loop_exit as well as
> interrupt/exception helpers for i386. This avoids a bunch of gcc4
> warnings.

ISO/IEC 9899:1990

7.1.3
          -- All  identifiers  that  begin  with  an  underscore and
             either an uppercase letter or  another  underscore  are
             always reserved for any use.

Breaking the standard is what brings us the joys of recently (re)posted
patch for NetBSD and [u]intXX fun.

[..snip..]

-- 
mailto:av1474@comtv.ru

[Qemu-devel] Re: [PATCH 2/2] Add __noreturn function attribute

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2948 bytes --]

malc wrote:
> On Fri, 28 Nov 2008, Jan Kiszka wrote:
> 
>> Introduce __noreturn attribute and attach it to cpu_loop_exit as well as
>> interrupt/exception helpers for i386. This avoids a bunch of gcc4
>> warnings.
> 
> ISO/IEC 9899:1990
> 
> 7.1.3
>          -- All  identifiers  that  begin  with  an  underscore and
>             either an uppercase letter or  another  underscore  are
>             always reserved for any use.

...but commonly used in practice (which is no excuse to do so here, just
an explanation). Given that we have __hidden already, I just followed
the existing pattern. 

> 
> Breaking the standard is what brings us the joys of recently (re)posted
> patch for NetBSD and [u]intXX fun.

I have no problem with calling it 'noreturn' instead.

Jan

----------->

Introduce noreturn attribute and attach it to cpu_loop_exit as well as
interrupt/exception helpers for i386. This avoids a bunch of gcc4
warnings.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 exec-all.h         |    5 ++++-
 target-i386/exec.h |    8 ++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/exec-all.h b/exec-all.h
index ca97f57..4934932 100644
--- a/exec-all.h
+++ b/exec-all.h
@@ -20,6 +20,9 @@
 
 #ifndef _EXEC_ALL_H_
 #define _EXEC_ALL_H_
+
+#define noreturn __attribute__ ((__noreturn__))
+
 /* allow to see translation results - the slowdown should be negligible, so we leave it */
 #define DEBUG_DISAS
 
@@ -82,7 +85,7 @@ TranslationBlock *tb_gen_code(CPUState *env,
                               target_ulong pc, target_ulong cs_base, int flags,
                               int cflags);
 void cpu_exec_init(CPUState *env);
-void cpu_loop_exit(void);
+void noreturn cpu_loop_exit(void);
 int page_unprotect(target_ulong address, unsigned long pc, void *puc);
 void tb_invalidate_phys_page_range(target_phys_addr_t start, target_phys_addr_t end,
                                    int is_cpu_write_access);
diff --git a/target-i386/exec.h b/target-i386/exec.h
index 3663166..b35f08b 100644
--- a/target-i386/exec.h
+++ b/target-i386/exec.h
@@ -67,10 +67,10 @@ void do_interrupt(int intno, int is_int, int error_code,
                   target_ulong next_eip, int is_hw);
 void do_interrupt_user(int intno, int is_int, int error_code,
                        target_ulong next_eip);
-void raise_interrupt(int intno, int is_int, int error_code,
-                     int next_eip_addend);
-void raise_exception_err(int exception_index, int error_code);
-void raise_exception(int exception_index);
+void noreturn raise_interrupt(int intno, int is_int, int error_code,
+                              int next_eip_addend);
+void noreturn raise_exception_err(int exception_index, int error_code);
+void noreturn raise_exception(int exception_index);
 void do_smm_enter(void);
 
 /* n must be a constant to be efficient */




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 258 bytes --]

Re: [Qemu-devel] Re: [PATCH 2/2] Add __noreturn function attribute

[Thiemo Seufer]

Jan Kiszka wrote:
> malc wrote:
> > On Fri, 28 Nov 2008, Jan Kiszka wrote:
> > 
> >> Introduce __noreturn attribute and attach it to cpu_loop_exit as well as
> >> interrupt/exception helpers for i386. This avoids a bunch of gcc4
> >> warnings.
> > 
> > ISO/IEC 9899:1990
> > 
> > 7.1.3
> >          -- All  identifiers  that  begin  with  an  underscore and
> >             either an uppercase letter or  another  underscore  are
> >             always reserved for any use.
> 
> ...but commonly used in practice (which is no excuse to do so here, just
> an explanation). Given that we have __hidden already, I just followed
> the existing pattern. 

A kernel is free to make its own rules about namespaces, but userland
applications like Qemu should adhere to the userland standards.


Thiemo

Re: [Qemu-devel] Re: [PATCH 2/2] Add __noreturn function attribute

[Avi Kivity]

Jan Kiszka wrote:
>> Breaking the standard is what brings us the joys of recently (re)posted
>> patch for NetBSD and [u]intXX fun.
>>     
>
> I have no problem with calling it 'noreturn' instead.
>   

That will break code that wants to use 'noreturn' as a local variable.  
I think ATTR_NORETURN, while a lot uglier, is safer.



-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [PATCH 2/2] Add __noreturn function attribute

[Stefan Weil]

Avi Kivity schrieb:
> Jan Kiszka wrote:
>>> Breaking the standard is what brings us the joys of recently (re)posted
>>> patch for NetBSD and [u]intXX fun.
>>>     
>>
>> I have no problem with calling it 'noreturn' instead.
>>   
>
> That will break code that wants to use 'noreturn' as a local
> variable.  I think ATTR_NORETURN, while a lot uglier, is safer.
>
>
>

Why do we need a new macro instead of just using __attribute__
((__noreturn__))?

The macro won't save very much writing, needs an include dependency, breaks
code which uses the same symbol for other purposes, makes code reusage
in other
products more difficult, ...

Stefan

Re: [Qemu-devel] Re: [PATCH 2/2] Add __noreturn function attribute

[Laurent Desnogues]

On Sun, Nov 30, 2008 at 12:51 PM, Stefan Weil <berlios@weilnetz.de> wrote:
>
> Why do we need a new macro instead of just using __attribute__
> ((__noreturn__))?

Don't we need it to prevent gcc-ism from being all over the code?
Though I wonder how much tcg converted code still depends on
being compiled by gcc.


Laurent

[Qemu-devel] Re: [PATCH 2/2] Add __noreturn function attribute

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 661 bytes --]

Laurent Desnogues wrote:
> On Sun, Nov 30, 2008 at 12:51 PM, Stefan Weil <berlios@weilnetz.de> wrote:
>> Why do we need a new macro instead of just using __attribute__
>> ((__noreturn__))?
> 
> Don't we need it to prevent gcc-ism from being all over the code?

Yep, and that's also why we need wrapping. You can easily define it away
if your compiler doesn't support it. We just need to add the required
conditions.

> Though I wonder how much tcg converted code still depends on
> being compiled by gcc.

I guess once the hard dependencies are removed, fixing things like the
existing __attributes__ etc. will just be mechanical work.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Qemu-devel] Re: [PATCH 2/2] Add __noreturn function attribute

[Stefan Weil]

Jan Kiszka schrieb:
> Laurent Desnogues wrote:
>> On Sun, Nov 30, 2008 at 12:51 PM, Stefan Weil <berlios@weilnetz.de>
>> wrote:
>>> Why do we need a new macro instead of just using __attribute__
>>> ((__noreturn__))?
>> Don't we need it to prevent gcc-ism from being all over the code?
>
> Yep, and that's also why we need wrapping. You can easily define it away
> if your compiler doesn't support it. We just need to add the required
> conditions.
>
>> Though I wonder how much tcg converted code still depends on
>> being compiled by gcc.
>
> I guess once the hard dependencies are removed, fixing things like the
> existing __attributes__ etc. will just be mechanical work.
>
> Jan
>


For compilers which don't support __attributes__, a simple

#define __attributes__(dummy) /* dummy */

or an equivalent command line option would eliminate all gcc-isms.
I don't think we need wrapping, at least not now.

Stefan

Re: [Qemu-devel] Re: [PATCH 2/2] Add __noreturn function attribute

[Andreas Färber]

Am 30.11.2008 um 13:52 schrieb Stefan Weil:

> Jan Kiszka schrieb:
>> Laurent Desnogues wrote:
>>> On Sun, Nov 30, 2008 at 12:51 PM, Stefan Weil <berlios@weilnetz.de>
>>> wrote:
>>>> Why do we need a new macro instead of just using __attribute__
>>>> ((__noreturn__))?
>>> Don't we need it to prevent gcc-ism from being all over the code?
>>
>> Yep, and that's also why we need wrapping. You can easily define it  
>> away
>> if your compiler doesn't support it. We just need to add the required
>> conditions.
>
> For compilers which don't support __attributes__, a simple
>
> #define __attributes__(dummy) /* dummy */
>
> or an equivalent command line option would eliminate all gcc-isms.
> I don't think we need wrapping, at least not now.

Aren't there GCCs which support __attributes__ but not all of them?
Apple's GCC 4.0.1 comes to mind, which doesn't support visibility  
attributes.

Andreas

Re: [Qemu-devel] Re: [PATCH 2/2] Add __noreturn function attribute

[M. Warner Losh]

In message: <49328C7F.1050705@mail.berlios.de>
            Stefan Weil <weil@mail.berlios.de> writes:
: Jan Kiszka schrieb:
: > Laurent Desnogues wrote:
: >> On Sun, Nov 30, 2008 at 12:51 PM, Stefan Weil <berlios@weilnetz.de>
: >> wrote:
: >>> Why do we need a new macro instead of just using __attribute__
: >>> ((__noreturn__))?
: >> Don't we need it to prevent gcc-ism from being all over the code?
: >
: > Yep, and that's also why we need wrapping. You can easily define it away
: > if your compiler doesn't support it. We just need to add the required
: > conditions.
: >
: >> Though I wonder how much tcg converted code still depends on
: >> being compiled by gcc.
: >
: > I guess once the hard dependencies are removed, fixing things like the
: > existing __attributes__ etc. will just be mechanical work.
: >
: > Jan
: >
: 
: 
: For compilers which don't support __attributes__, a simple
: 
: #define __attributes__(dummy) /* dummy */
: 
: or an equivalent command line option would eliminate all gcc-isms.
: I don't think we need wrapping, at least not now.

This doesn't work so well when 'dummy' is __packed__.

The BSD projects have had similar macros for a long long time, and
they work out well in practice.

Warner

[Qemu-devel] Re: [PATCH 2/2] Add __noreturn function attribute

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 575 bytes --]

Avi Kivity wrote:
> Jan Kiszka wrote:
>>> Breaking the standard is what brings us the joys of recently (re)posted
>>> patch for NetBSD and [u]intXX fun.
>>>     
>>
>> I have no problem with calling it 'noreturn' instead.
>>   
> 
> That will break code that wants to use 'noreturn' as a local variable. 
> I think ATTR_NORETURN, while a lot uglier, is safer.

Do you have such code already? Is it exported beyond qemu scope? Then
why not going for our own convention "'noreturn' is reserved as function
attribute"? (And yes, your macro is ugly :) ).

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Qemu-devel] Re: [PATCH 2/2] Add __noreturn function attribute

[Thiemo Seufer]

Jan Kiszka wrote:
> Avi Kivity wrote:
> > Jan Kiszka wrote:
> >>> Breaking the standard is what brings us the joys of recently (re)posted
> >>> patch for NetBSD and [u]intXX fun.
> >>>     
> >>
> >> I have no problem with calling it 'noreturn' instead.
> >>   
> > 
> > That will break code that wants to use 'noreturn' as a local variable. 
> > I think ATTR_NORETURN, while a lot uglier, is safer.
> 
> Do you have such code already? Is it exported beyond qemu scope? Then
> why not going for our own convention "'noreturn' is reserved as function
> attribute"? (And yes, your macro is ugly :) ).

"Macro names should be in upper case" is also a useful convention.

FWIW, I agree with Stefan, there's currently not much need to isolate
gcc-isms.


Thiemo

Re: [Qemu-devel] Re: [PATCH 2/2] Add __noreturn function attribute

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1210 bytes --]

Thiemo Seufer wrote:
> Jan Kiszka wrote:
>> Avi Kivity wrote:
>>> Jan Kiszka wrote:
>>>>> Breaking the standard is what brings us the joys of recently (re)posted
>>>>> patch for NetBSD and [u]intXX fun.
>>>>>     
>>>> I have no problem with calling it 'noreturn' instead.
>>>>   
>>> That will break code that wants to use 'noreturn' as a local variable. 
>>> I think ATTR_NORETURN, while a lot uglier, is safer.
>> Do you have such code already? Is it exported beyond qemu scope? Then
>> why not going for our own convention "'noreturn' is reserved as function
>> attribute"? (And yes, your macro is ugly :) ).
> 
> "Macro names should be in upper case" is also a useful convention.

Generally yes. But there are exceptions when the macro is used in a
context where upper case disturbs the readability instead of improving
it. I would argue that this is the case here, but it's always a matter
of taste.

> 
> FWIW, I agree with Stefan, there's currently not much need to isolate
> gcc-isms.

If everyone prefers having __attribute__ in the function prototypes
directly -- OK. All I want is to get rid of the warnings without
changing the code into the wrong direction.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Qemu-devel] Re: [PATCH 2/2] Add __noreturn function attribute

[Anthony Liguori]

Jan Kiszka wrote:
> Thiemo Seufer wrote:
>   
>> Jan Kiszka wrote:
>>     
>>> Avi Kivity wrote:
>>>       
>>>> Jan Kiszka wrote:
>>>>         
>>>>>> Breaking the standard is what brings us the joys of recently (re)posted
>>>>>> patch for NetBSD and [u]intXX fun.
>>>>>>     
>>>>>>             
>>>>> I have no problem with calling it 'noreturn' instead.
>>>>>   
>>>>>           
>>>> That will break code that wants to use 'noreturn' as a local variable. 
>>>> I think ATTR_NORETURN, while a lot uglier, is safer.
>>>>         
>>> Do you have such code already? Is it exported beyond qemu scope? Then
>>> why not going for our own convention "'noreturn' is reserved as function
>>> attribute"? (And yes, your macro is ugly :) ).
>>>       
>> "Macro names should be in upper case" is also a useful convention.
>>     
>
> Generally yes. But there are exceptions when the macro is used in a
> context where upper case disturbs the readability instead of improving
> it. I would argue that this is the case here, but it's always a matter
> of taste.
>
>   
>> FWIW, I agree with Stefan, there's currently not much need to isolate
>> gcc-isms.
>>     
>
> If everyone prefers having __attribute__ in the function prototypes
> directly -- OK. All I want is to get rid of the warnings without
> changing the code into the wrong direction.
>   

Please stick with the #define.  It's not about the GCC-ism, it's being 
able to quickly replace it with something else.

This helps for things like sparse.

Regards,

Anthony Liguori

> Jan
>
>   

[Qemu-devel] Re: [PATCH 2/2] Add __noreturn function attribute

[Jan Kiszka]

Anthony Liguori wrote:
> Jan Kiszka wrote:
>> Thiemo Seufer wrote:
>>  
>>> Jan Kiszka wrote:
>>>    
>>>> Avi Kivity wrote:
>>>>      
>>>>> Jan Kiszka wrote:
>>>>>        
>>>>>>> Breaking the standard is what brings us the joys of recently
>>>>>>> (re)posted
>>>>>>> patch for NetBSD and [u]intXX fun.
>>>>>>>                 
>>>>>> I have no problem with calling it 'noreturn' instead.
>>>>>>             
>>>>> That will break code that wants to use 'noreturn' as a local
>>>>> variable. I think ATTR_NORETURN, while a lot uglier, is safer.
>>>>>         
>>>> Do you have such code already? Is it exported beyond qemu scope? Then
>>>> why not going for our own convention "'noreturn' is reserved as
>>>> function
>>>> attribute"? (And yes, your macro is ugly :) ).
>>>>       
>>> "Macro names should be in upper case" is also a useful convention.
>>>     
>>
>> Generally yes. But there are exceptions when the macro is used in a
>> context where upper case disturbs the readability instead of improving
>> it. I would argue that this is the case here, but it's always a matter
>> of taste.
>>
>>  
>>> FWIW, I agree with Stefan, there's currently not much need to isolate
>>> gcc-isms.
>>>     
>>
>> If everyone prefers having __attribute__ in the function prototypes
>> directly -- OK. All I want is to get rid of the warnings without
>> changing the code into the wrong direction.
>>   
> 
> Please stick with the #define.  It's not about the GCC-ism, it's being
> able to quickly replace it with something else.

For sure. Err... but which one now? "noreturn" is already available as
signed patch.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 26
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH 2/2] Add __noreturn function attribute

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2307 bytes --]

Jan Kiszka wrote:
> Anthony Liguori wrote:
>> Jan Kiszka wrote:
>>> Thiemo Seufer wrote:
>>>  
>>>> Jan Kiszka wrote:
>>>>    
>>>>> Avi Kivity wrote:
>>>>>      
>>>>>> Jan Kiszka wrote:
>>>>>>        
>>>>>>>> Breaking the standard is what brings us the joys of recently
>>>>>>>> (re)posted
>>>>>>>> patch for NetBSD and [u]intXX fun.
>>>>>>>>                 
>>>>>>> I have no problem with calling it 'noreturn' instead.
>>>>>>>             
>>>>>> That will break code that wants to use 'noreturn' as a local
>>>>>> variable. I think ATTR_NORETURN, while a lot uglier, is safer.
>>>>>>         
>>>>> Do you have such code already? Is it exported beyond qemu scope? Then
>>>>> why not going for our own convention "'noreturn' is reserved as
>>>>> function
>>>>> attribute"? (And yes, your macro is ugly :) ).
>>>>>       
>>>> "Macro names should be in upper case" is also a useful convention.
>>>>     
>>> Generally yes. But there are exceptions when the macro is used in a
>>> context where upper case disturbs the readability instead of improving
>>> it. I would argue that this is the case here, but it's always a matter
>>> of taste.
>>>
>>>  
>>>> FWIW, I agree with Stefan, there's currently not much need to isolate
>>>> gcc-isms.
>>>>     
>>> If everyone prefers having __attribute__ in the function prototypes
>>> directly -- OK. All I want is to get rid of the warnings without
>>> changing the code into the wrong direction.
>>>   
>> Please stick with the #define.  It's not about the GCC-ism, it's being
>> able to quickly replace it with something else.
> 
> For sure. Err... but which one now? "noreturn" is already available as
> signed patch.

...but that patch in fact has an issue with existing
__attribute__((noreturn)) sites. Converting them all to the common macro
raises the question where to put the definition. qemu-common.h appears
logical on first sight, but due to dyngen-exec.h's redefinition mess we
cannot include that header easily.

I'm currently trying to find a workaround that is not too invasive and
can quickly be removed once dyngen is gone. The deeper I dig for that,
the more weird the dependencies get.

However, please let me know if the preferred macro name is different
from "noreturn".

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]