2007-05-25 18:34:40

by Robert P. J. Day

[permalink] [raw]
Subject: [PATCH] MIPS: Transform old-style macros to newer "__noreturn" standard.


Convert old/obsolete NORET_TYPE and ATTRIB_NORET macros to use the
newer standard of "__noreturn" as defined in compiler-gcc.h.

Signed-off-by: Robert P. J. Day <[email protected]>

---

since there were a number of these old macros under the MIPS
directory alone, i thought it would be useful to do that subsystem as
a patch by itself, and define the standards for this cleanup:

1) in a function declaration, the "__noreturn" will go at the end of
the declaration.

2) in a definition, "__noreturn" will go between the return type and
the function name

3) in a function typedef, "__noreturn" will go immediately after the
return type, just like with definitions.

4) if a function definition already includes "__noreturn", there's no
point in having any external references to it also say the same thing.
(right?)

does all that sound reasonable? if it does, i can deal with the
rest of them in the same way.

arch/mips/dec/prom/init.c | 2 +-
arch/mips/dec/reset.c | 10 +++++-----
arch/mips/kernel/process.c | 4 ++--
arch/mips/kernel/smp.c | 2 +-
arch/mips/kernel/traps.c | 2 +-
arch/mips/sgi-ip22/ip22-reset.c | 6 +++---
arch/mips/sibyte/cfe/setup.c | 6 +++---
7 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/arch/mips/dec/prom/init.c b/arch/mips/dec/prom/init.c
index a217aaf..4828cac 100644
--- a/arch/mips/dec/prom/init.c
+++ b/arch/mips/dec/prom/init.c
@@ -86,7 +86,7 @@ void __init which_prom(s32 magic, s32 *prom_vec)

void __init prom_init(void)
{
- extern void ATTRIB_NORET dec_machine_halt(void);
+ extern void dec_machine_halt(void);
static char cpu_msg[] __initdata =
"Sorry, this kernel is compiled for a wrong CPU type!\n";
s32 argc = fw_arg0;
diff --git a/arch/mips/dec/reset.c b/arch/mips/dec/reset.c
index 5639722..c15a879 100644
--- a/arch/mips/dec/reset.c
+++ b/arch/mips/dec/reset.c
@@ -9,26 +9,26 @@

#include <asm/addrspace.h>

-typedef void ATTRIB_NORET (* noret_func_t)(void);
+typedef void __noreturn (* noret_func_t)(void);

-static inline void ATTRIB_NORET back_to_prom(void)
+static inline void __noreturn back_to_prom(void)
{
noret_func_t func = (void *)CKSEG1ADDR(0x1fc00000);

func();
}

-void ATTRIB_NORET dec_machine_restart(char *command)
+void __noreturn dec_machine_restart(char *command)
{
back_to_prom();
}

-void ATTRIB_NORET dec_machine_halt(void)
+void __noreturn dec_machine_halt(void)
{
back_to_prom();
}

-void ATTRIB_NORET dec_machine_power_off(void)
+void __noreturn dec_machine_power_off(void)
{
/* DECstations don't have a software power switch */
back_to_prom();
diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index 6bdfb5a..8f4cf27 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -46,7 +46,7 @@
* power and have a low exit latency (ie sit in a loop waiting for somebody to
* say that they'd like to reschedule)
*/
-ATTRIB_NORET void cpu_idle(void)
+void __noreturn cpu_idle(void)
{
/* endless idle loop with no priority at all */
while (1) {
@@ -213,7 +213,7 @@ int dump_task_fpu (struct task_struct *t, elf_fpregset_t *fpr)
/*
* Create a kernel thread
*/
-static ATTRIB_NORET void kernel_thread_helper(void *arg, int (*fn)(void *))
+static void __noreturn kernel_thread_helper(void *arg, int (*fn)(void *))
{
do_exit(fn(arg));
}
diff --git a/arch/mips/kernel/smp.c b/arch/mips/kernel/smp.c
index c46e479..fa63727 100644
--- a/arch/mips/kernel/smp.c
+++ b/arch/mips/kernel/smp.c
@@ -62,7 +62,7 @@ static void smp_tune_scheduling (void)
}

extern void __init calibrate_delay(void);
-extern ATTRIB_NORET void cpu_idle(void);
+extern void cpu_idle(void);

/*
* First C code run on the secondary CPUs after being started up by
diff --git a/arch/mips/kernel/traps.c b/arch/mips/kernel/traps.c
index 200de02..0c72b2c 100644
--- a/arch/mips/kernel/traps.c
+++ b/arch/mips/kernel/traps.c
@@ -310,7 +310,7 @@ void show_registers(struct pt_regs *regs)

static DEFINE_SPINLOCK(die_lock);

-NORET_TYPE void ATTRIB_NORET die(const char * str, struct pt_regs * regs)
+void __noreturn die(const char * str, struct pt_regs * regs)
{
static int die_counter;
#ifdef CONFIG_MIPS_MT_SMTC
diff --git a/arch/mips/sgi-ip22/ip22-reset.c b/arch/mips/sgi-ip22/ip22-reset.c
index 66df5ac..63afd7e 100644
--- a/arch/mips/sgi-ip22/ip22-reset.c
+++ b/arch/mips/sgi-ip22/ip22-reset.c
@@ -46,7 +46,7 @@ static struct timer_list power_timer, blink_timer, debounce_timer, volume_timer;

static int machine_state;

-static void ATTRIB_NORET sgi_machine_power_off(void)
+static void __noreturn sgi_machine_power_off(void)
{
unsigned int tmp;

@@ -68,7 +68,7 @@ static void ATTRIB_NORET sgi_machine_power_off(void)
}
}

-static void ATTRIB_NORET sgi_machine_restart(char *command)
+static void __noreturn sgi_machine_restart(char *command)
{
if (machine_state & MACHINE_SHUTTING_DOWN)
sgi_machine_power_off();
@@ -76,7 +76,7 @@ static void ATTRIB_NORET sgi_machine_restart(char *command)
while (1);
}

-static void ATTRIB_NORET sgi_machine_halt(void)
+static void __noreturn sgi_machine_halt(void)
{
if (machine_state & MACHINE_SHUTTING_DOWN)
sgi_machine_power_off();
diff --git a/arch/mips/sibyte/cfe/setup.c b/arch/mips/sibyte/cfe/setup.c
index ae4a92c..51898dd 100644
--- a/arch/mips/sibyte/cfe/setup.c
+++ b/arch/mips/sibyte/cfe/setup.c
@@ -62,7 +62,7 @@ extern unsigned long initrd_start, initrd_end;
extern int kgdb_port;
#endif

-static void ATTRIB_NORET cfe_linux_exit(void *arg)
+static void __noreturn cfe_linux_exit(void *arg)
{
int warm = *(int *)arg;

@@ -83,14 +83,14 @@ static void ATTRIB_NORET cfe_linux_exit(void *arg)
while (1);
}

-static void ATTRIB_NORET cfe_linux_restart(char *command)
+static void __noreturn cfe_linux_restart(char *command)
{
static const int zero;

cfe_linux_exit((void *)&zero);
}

-static void ATTRIB_NORET cfe_linux_halt(void)
+static void __noreturn cfe_linux_halt(void)
{
static const int one = 1;


--
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://fsdev.net/wiki/index.php?title=Main_Page
========================================================================


2007-05-25 18:41:00

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH] MIPS: Transform old-style macros to newer "__noreturn" standard.

Robert P. J. Day wrote:
> Convert old/obsolete NORET_TYPE and ATTRIB_NORET macros to use the
> newer standard of "__noreturn" as defined in compiler-gcc.h.
>
> Signed-off-by: Robert P. J. Day <[email protected]>

> 1) in a function declaration, the "__noreturn" will go at the end of
> the declaration.
>
> 2) in a definition, "__noreturn" will go between the return type and
> the function name
>
> 3) in a function typedef, "__noreturn" will go immediately after the
> return type, just like with definitions.
>
> 4) if a function definition already includes "__noreturn", there's no
> point in having any external references to it also say the same thing.
> (right?)

This is dumb, though.

"void __noreturn" is redundant. It would be much cleaner to have a
macro which amounts to "void __attribute__((noreturn))" and use it
instead of giving a return type.

Even "void" as the return type is bogus -- the function never returns so
it doesn't *have* a return type...

-hpa

2007-05-25 19:05:46

by Robert P. J. Day

[permalink] [raw]
Subject: Re: [PATCH] MIPS: Transform old-style macros to newer "__noreturn" standard.

On Fri, 25 May 2007, H. Peter Anvin wrote:

> Robert P. J. Day wrote:
> > Convert old/obsolete NORET_TYPE and ATTRIB_NORET macros to use the
> > newer standard of "__noreturn" as defined in compiler-gcc.h.
> >
> > Signed-off-by: Robert P. J. Day <[email protected]>
>
> > 1) in a function declaration, the "__noreturn" will go at the end of
> > the declaration.
> >
> > 2) in a definition, "__noreturn" will go between the return type and
> > the function name
> >
> > 3) in a function typedef, "__noreturn" will go immediately after the
> > return type, just like with definitions.
> >
> > 4) if a function definition already includes "__noreturn", there's no
> > point in having any external references to it also say the same thing.
> > (right?)
>
> This is dumb, though.
>
> "void __noreturn" is redundant. It would be much cleaner to have a
> macro which amounts to "void __attribute__((noreturn))" and use it
> instead of giving a return type.
>
> Even "void" as the return type is bogus -- the function never
> returns so it doesn't *have* a return type...

i agree, and i just did a quick test and noticed that, if you try to
declare a function thusly:

f() __attribute__((noreturn)) ;

you get:

warning: data definition has no type or storage class

but gcc doesn't complain if you declare it thusly:

__attribute__((noreturn)) f() ;

that strikes me as a flaw in gcc, no?

rday
--
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://fsdev.net/wiki/index.php?title=Main_Page
========================================================================

2007-05-25 19:40:26

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH] MIPS: Transform old-style macros to newer "__noreturn" standard.

Robert P. J. Day wrote:
>
>
> f() __attribute__((noreturn)) ;
>
> you get:
>
> warning: data definition has no type or storage class
>
> but gcc doesn't complain if you declare it thusly:
>
> __attribute__((noreturn)) f() ;
>
> that strikes me as a flaw in gcc, no?
>

Doesn't matter. gcc accepts "void __attribute__((noreturn)) f();", and
thus, one can define:

#define __noreturn void __attribute__((noreturn))

... and declare functions as:

__noreturn f();

... which is the syntactially sane way of doing it.

-hpa

2007-05-25 21:12:25

by Robert P. J. Day

[permalink] [raw]
Subject: Re: [PATCH] MIPS: Transform old-style macros to newer "__noreturn" standard.

On Fri, 25 May 2007, H. Peter Anvin wrote:

> Robert P. J. Day wrote:
> >
> >
> > f() __attribute__((noreturn)) ;
> >
> > you get:
> >
> > warning: data definition has no type or storage class
> >
> > but gcc doesn't complain if you declare it thusly:
> >
> > __attribute__((noreturn)) f() ;
> >
> > that strikes me as a flaw in gcc, no?
> >
>
> Doesn't matter. gcc accepts "void __attribute__((noreturn)) f();", and
> thus, one can define:
>
> #define __noreturn void __attribute__((noreturn))
>
> ... and declare functions as:
>
> __noreturn f();
>
> ... which is the syntactially sane way of doing it.

that may be, but keep in mind that gcc allows attributes to *follow*
the parameter list as well, and some people might prefer to do the
following:

f() __noreturn;

that would fail badly if you defined __noreturn as you suggest.

is there, in fact, a tradition for attribute usage, along the lines
of what satyam suggested earlier? because once there's an established
standard, that's going to dictate what is and isn't possible in terms
of macros and shortcuts.

rday
--
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://fsdev.net/wiki/index.php?title=Main_Page
========================================================================

2007-05-25 21:44:16

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH] MIPS: Transform old-style macros to newer "__noreturn" standard.

Robert P. J. Day wrote:
>> ... and declare functions as:
>>
>> __noreturn f();
>>
>> ... which is the syntactially sane way of doing it.
>
> that may be, but keep in mind that gcc allows attributes to *follow*
> the parameter list as well, and some people might prefer to do the
> following:
>
> f() __noreturn;
>
> that would fail badly if you defined __noreturn as you suggest.

That's equally moronic that saying that "some people might prefer to
write 'f() void;'", which is what it's *EXACTLY* equivalent to. Yes,
they might "prefer" it, but it's syntactically invalid and the compiler
won't accept it. As it shouldn't.

__noreturn here takes the syntactic place of the return type, because
that's what it IS.

-hpa

2007-05-25 22:16:56

by Satyam Sharma

[permalink] [raw]
Subject: Re: [PATCH] MIPS: Transform old-style macros to newer "__noreturn" standard.

Hi Peter,

On 5/26/07, H. Peter Anvin <[email protected]> wrote:
> Robert P. J. Day wrote:
> >> ... and declare functions as:
> >>
> >> __noreturn f();
> >>
> >> ... which is the syntactially sane way of doing it.
> >
> > that may be, but keep in mind that gcc allows attributes to *follow*
> > the parameter list as well, and some people might prefer to do the
> > following:
> >
> > f() __noreturn;
> >
> > that would fail badly if you defined __noreturn as you suggest.
>
> That's equally moronic that saying that "some people might prefer to
> write 'f() void;'", which is what it's *EXACTLY* equivalent to. Yes,
> they might "prefer" it, but it's syntactically invalid and the compiler
> won't accept it. As it shouldn't.
>
> __noreturn here takes the syntactic place of the return type, because
> that's what it IS.

But __attribute__((noreturn)) is simply a _function attribute_. Of course,
it is legal / valid only for functions with return-type void, so it does make
sense to combine both void and __attribute__((noreturn)) in the same
macro like you say. But that's not syntactically necessary. In fact,
grepping through the sources, a lot of people do prefer to place the
attribute _after_ the function declarator.

Anyway, I'm fine either way.

Thanks,
Satyam

2007-05-25 22:28:04

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH] MIPS: Transform old-style macros to newer "__noreturn" standard.

Satyam Sharma wrote:
>
> But __attribute__((noreturn)) is simply a _function attribute_. Of course,
> it is legal / valid only for functions with return-type void, so it does
> make
> sense to combine both void and __attribute__((noreturn)) in the same
> macro like you say. But that's not syntactically necessary. In fact,
> grepping through the sources, a lot of people do prefer to place the
> attribute _after_ the function declarator.
>
> Anyway, I'm fine either way.
>

Sorry to say, but weren't you the person who didn't recognize !! as the
idiomatic booleanizing operator?

I think you need to learn that everything that the compiler accepts
isn't necessarily idiomatic, readable code. Consider
__attribute__((noreturn)); it's a nonstandard feature implemented using
a generic gcc mechanism -- thus what the compiler will accept is quite
flexible, because it's a generic building block. It doesn't mean it's a
good idea.

The reason it's often written at the end of the expression mostly has to
do with bugs in some very early versions of gcc.

-hpa

2007-05-25 22:35:54

by Satyam Sharma

[permalink] [raw]
Subject: Re: [PATCH] MIPS: Transform old-style macros to newer "__noreturn" standard.

On 5/26/07, H. Peter Anvin <[email protected]> wrote:
> Satyam Sharma wrote:
> >
> > But __attribute__((noreturn)) is simply a _function attribute_. Of course,
> > it is legal / valid only for functions with return-type void, so it does
> > make
> > sense to combine both void and __attribute__((noreturn)) in the same
> > macro like you say. But that's not syntactically necessary. In fact,
> > grepping through the sources, a lot of people do prefer to place the
> > attribute _after_ the function declarator.
> >
> > Anyway, I'm fine either way.
> >
>
> Sorry to say, but weren't you the person who didn't recognize !! as the
> idiomatic booleanizing operator?

Yes, of course, please prove a link / connection between that and this?

> I think you need to learn that everything that the compiler accepts
> isn't necessarily idiomatic, readable code. Consider
> __attribute__((noreturn)); it's a nonstandard feature implemented using
> a generic gcc mechanism -- thus what the compiler will accept is quite
> flexible, because it's a generic building block. It doesn't mean it's a
> good idea.
>
> The reason it's often written at the end of the expression mostly has to
> do with bugs in some very early versions of gcc.

That might be, but I was only saying that there is no syntactical
*compulsion* to combine the attribute with the return type. As for what's
readable, it is subjective. And as for what's common / standard / idiomatic
in the kernel code as of today, nothing beats a grep. Anyway, as I said
previously, I'm fine with either way.

Satyam

2007-05-25 22:46:58

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH] MIPS: Transform old-style macros to newer "__noreturn" standard.

Satyam Sharma wrote:
>>
>> Sorry to say, but weren't you the person who didn't recognize !! as the
>> idiomatic booleanizing operator?
>
> Yes, of course, please prove a link / connection between that and this?
>

Very simple: it shows a lack of understanding of idiomatic use of C.

-hpa

2007-05-25 22:56:54

by Satyam Sharma

[permalink] [raw]
Subject: Re: [PATCH] MIPS: Transform old-style macros to newer "__noreturn" standard.

On 5/26/07, H. Peter Anvin <[email protected]> wrote:
> Satyam Sharma wrote:
> >>
> >> Sorry to say, but weren't you the person who didn't recognize !! as the
> >> idiomatic booleanizing operator?
> >
> > Yes, of course, please prove a link / connection between that and this?
> >
>
> Very simple: it shows a lack of understanding of idiomatic use of C.

Ah, I could lecture / *prove* to you how this argument ad hominem is
*absolutely* fallacious and nonsense, but I'll leave that just now.

2007-05-26 13:35:05

by Robert P. J. Day

[permalink] [raw]
Subject: Re: [PATCH] MIPS: Transform old-style macros to newer "__noreturn" standard.

On Fri, 25 May 2007, H. Peter Anvin wrote:

> Robert P. J. Day wrote:
> >
> >
> > f() __attribute__((noreturn)) ;
> >
> > you get:
> >
> > warning: data definition has no type or storage class
> >
> > but gcc doesn't complain if you declare it thusly:
> >
> > __attribute__((noreturn)) f() ;
> >
> > that strikes me as a flaw in gcc, no?
> >
>
> Doesn't matter. gcc accepts "void __attribute__((noreturn)) f();", and
> thus, one can define:
>
> #define __noreturn void __attribute__((noreturn))
>
> ... and declare functions as:
>
> __noreturn f();
>
> ... which is the syntactially sane way of doing it.

i've thought about this and, while i philosophically agree with you,
i'm not going down that road for a couple reasons.

first, there is the fact that there is already a macro definition for
__noreturn in include/linux/compiler-gcc.h:

#define __noreturn __attribute__((noreturn))

second, and more importantly, defining __noreturn as you suggest is
now mixing two fundamental pieces of information. "void" is a return
type, plain and simple. an attribute, OTOH, as i read it, is more of
a suggestion to the compiler to allow better checking of your code
and, based on future compilers, it may not even be meaningful.

all of the short forms for attributes defined in compiler-gcc.h are
exactly that -- short forms. by adding in a return type, you're
fundamentally changing the way that short cut can be used.

yes, i *know* that __noreturn makes the return type irrelevant. but
even the gcc manual doesn't try to take that extra step:

================================================================
A few standard library functions, such as abort and exit, cannot
return. GCC knows this automatically. Some programs define their own
functions that never return. You can declare them noreturn to tell the
compiler this fact. For example,



void fatal () __attribute__ ((noreturn));

void
fatal (...)
{
... /* Print error message. */ ...
exit (1);
}

The noreturn keyword tells the compiler to assume that fatal cannot
return. It can then optimize without regard to what would happen if
fatal ever did return. This makes slightly better code. More
importantly, it helps avoid spurious warnings of uninitialized
variables.

Do not assume that registers saved by the calling function are
restored before calling the noreturn function.

It does not make sense for a noreturn function to have a return type
other than void.
^^^^^ ^^^^ ^^^^
===============================================================

so I'm just going to stick with the pattern that's been used so far.
i realize it offends your sense of syntactic sensibility, but it's
just not worth treating that one attribute so differently from the
rest of them.

rday
--
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://fsdev.net/wiki/index.php?title=Main_Page
========================================================================

2007-05-26 18:10:50

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH] MIPS: Transform old-style macros to newer "__noreturn" standard.

Robert P. J. Day wrote:
>
> It does not make sense for a noreturn function to have a return type
> other than void.
> ^^^^^ ^^^^ ^^^^
> ===============================================================
>
> so I'm just going to stick with the pattern that's been used so far.
> i realize it offends your sense of syntactic sensibility, but it's
> just not worth treating that one attribute so differently from the
> rest of them.
>

Why are you so hung up over the fact that the *implementation* of this
is an attribute? You're totally confusing interface and implementation.

-hpa

2007-05-26 23:29:17

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [PATCH] MIPS: Transform old-style macros to newer "__noreturn" standard.

H. Peter Anvin wrote:
> Robert P. J. Day wrote:
>> It does not make sense for a noreturn function to have a return type
>> other than void.
>> ^^^^^ ^^^^ ^^^^
>> ===============================================================
>>
>> so I'm just going to stick with the pattern that's been used so far.
>> i realize it offends your sense of syntactic sensibility, but it's
>> just not worth treating that one attribute so differently from the
>> rest of them.
>>
>
> Why are you so hung up over the fact that the *implementation* of this
> is an attribute? You're totally confusing interface and implementation.
>

Perhaps I should clarify this:

The whole reason to abstract this as a macro *at all* is to take it away
from the specific implementation in gcc. Thus, implementing an inferior
interface just because gcc happens to do it that way is actively
counterproductive.

-hpa