given that:
$ grep -r "define.*NORET_TYPE" *
include/linux/ext4_fs.h:# define NORET_TYPE /**/
include/linux/linkage.h:#define NORET_TYPE /**/
include/linux/ext3_fs.h:# define NORET_TYPE /**/
$
is there any obvious value to the 30 or so uses of that macro
sprinkled throughout the tree?
rday
--
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA
http://fsdev.net/wiki/index.php?title=Main_Page
========================================================================
> given that:
>
> $ grep -r "define.*NORET_TYPE" *
> include/linux/ext4_fs.h:# define NORET_TYPE /**/
> include/linux/linkage.h:#define NORET_TYPE /**/
> include/linux/ext3_fs.h:# define NORET_TYPE /**/
> $
>
> is there any obvious value to the 30 or so uses of that macro
> sprinkled throughout the tree?
Since it evaluates to absolutely empty code during pre-processing, there
is no obvious value. The question is whether there is some odd hackish
non-obvious value, I'd expect. (I'd also expect that to be another "no".)
If something that evaluates to nothingness ("There was nothing left...not
even a hole!") actually does anything, then somebody in the
standards-compilers-users pipeline needs to be violently beaten for
stupidity.
On Tue, May 22, 2007 at 09:09:22AM -0400, Robert P. J. Day wrote:
>
> given that:
>
> $ grep -r "define.*NORET_TYPE" *
> include/linux/ext4_fs.h:# define NORET_TYPE /**/
> include/linux/linkage.h:#define NORET_TYPE /**/
> include/linux/ext3_fs.h:# define NORET_TYPE /**/
> $
>
> is there any obvious value to the 30 or so uses of that macro
> sprinkled throughout the tree?
I can be mistaken, but it seems the uses should be replaced with your
__noreturn
> rday
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
On Tue, 22 May 2007, John Anthony Kazos Jr. wrote:
> > given that:
> >
> > $ grep -r "define.*NORET_TYPE" *
> > include/linux/ext4_fs.h:# define NORET_TYPE /**/
> > include/linux/linkage.h:#define NORET_TYPE /**/
> > include/linux/ext3_fs.h:# define NORET_TYPE /**/
> > $
> >
> > is there any obvious value to the 30 or so uses of that macro
> > sprinkled throughout the tree?
>
> Since it evaluates to absolutely empty code during pre-processing,
> there is no obvious value. The question is whether there is some odd
> hackish non-obvious value, I'd expect. (I'd also expect that to be
> another "no".)
>
> If something that evaluates to nothingness ("There was nothing
> left...not even a hole!") actually does anything, then somebody in
> the standards-compilers-users pipeline needs to be violently beaten
> for stupidity.
actually, one of the folks on the KJ list found this:
http://www.ussg.iu.edu/hypermail/linux/kernel/9605/1957.html
which speaks thusly:
...
-#if __GNUC__ < 2 || (__GNUC__ == 2 && __GNUC_MINOR__ < 5)
-# define NORET_TYPE __volatile__
-# define ATTRIB_NORET /**/
-# define NORET_AND /**/
-#else
# define NORET_TYPE /**/
# define ATTRIB_NORET __attribute__((noreturn))
# define NORET_AND noreturn,
-#endif
...
so it looks like a thoroughly obsolete macro which can be tossed.
i'll make the patch and test it.
rday
--
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA
http://fsdev.net/wiki/index.php?title=Main_Page
========================================================================
On Tue, 22 May 2007, Adrian Bunk wrote:
> On Tue, May 22, 2007 at 09:09:22AM -0400, Robert P. J. Day wrote:
> >
> > given that:
> >
> > $ grep -r "define.*NORET_TYPE" *
> > include/linux/ext4_fs.h:# define NORET_TYPE /**/
> > include/linux/linkage.h:#define NORET_TYPE /**/
> > include/linux/ext3_fs.h:# define NORET_TYPE /**/
> > $
> >
> > is there any obvious value to the 30 or so uses of that macro
> > sprinkled throughout the tree?
>
> I can be mistaken, but it seems the uses should be replaced with your
> __noreturn
no, i think you're thinking of the alternative ATTRIB_NORET macro.
as you can read in my previous post, NORET_TYPE used to resolve to
"__volatile__" for very old gcc. so i think it's legitimately dead
and can be ripped out.
rday
--
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA
http://fsdev.net/wiki/index.php?title=Main_Page
========================================================================
On 5/22/07, Robert P. J. Day <[email protected]> wrote:
> On Tue, 22 May 2007, John Anthony Kazos Jr. wrote:
>
> > > given that:
> > >
> > > $ grep -r "define.*NORET_TYPE" *
> > > include/linux/ext4_fs.h:# define NORET_TYPE /**/
> > > include/linux/linkage.h:#define NORET_TYPE /**/
> > > include/linux/ext3_fs.h:# define NORET_TYPE /**/
> > > $
> > >
> > > is there any obvious value to the 30 or so uses of that macro
> > > sprinkled throughout the tree?
> >
> > Since it evaluates to absolutely empty code during pre-processing,
> > there is no obvious value. The question is whether there is some odd
> > hackish non-obvious value, I'd expect. (I'd also expect that to be
> > another "no".)
> >
> > If something that evaluates to nothingness ("There was nothing
> > left...not even a hole!") actually does anything, then somebody in
> > the standards-compilers-users pipeline needs to be violently beaten
> > for stupidity.
>
>
> actually, one of the folks on the KJ list found this:
>
> http://www.ussg.iu.edu/hypermail/linux/kernel/9605/1957.html
>
> which speaks thusly:
>
> ...
> -#if __GNUC__ < 2 || (__GNUC__ == 2 && __GNUC_MINOR__ < 5)
> -# define NORET_TYPE __volatile__
> -# define ATTRIB_NORET /**/
> -# define NORET_AND /**/
> -#else
> # define NORET_TYPE /**/
> # define ATTRIB_NORET __attribute__((noreturn))
> # define NORET_AND noreturn,
> -#endif
> ...
>
> so it looks like a thoroughly obsolete macro which can be tossed.
> i'll make the patch and test it.
AFAICT, NORET_TYPE must've been introduced to silence gcc _warnings_,
and not do actually do anything useful that affects functionality in any way.
So the way to "test" your patch would be to see if there is any increase /
decrease in the number of *warnings* blurted out by gcc during kernel build
(best would be to build with various gcc versions on various platforms :-)
Satyam
On 5/22/07, Satyam Sharma <[email protected]> wrote:
> On 5/22/07, Robert P. J. Day <[email protected]> wrote:
> > On Tue, 22 May 2007, John Anthony Kazos Jr. wrote:
> >
> > > > given that:
> > > >
> > > > $ grep -r "define.*NORET_TYPE" *
> > > > include/linux/ext4_fs.h:# define NORET_TYPE /**/
> > > > include/linux/linkage.h:#define NORET_TYPE /**/
> > > > include/linux/ext3_fs.h:# define NORET_TYPE /**/
> > > > $
> > > >
> > > > is there any obvious value to the 30 or so uses of that macro
> > > > sprinkled throughout the tree?
> > >
> > > Since it evaluates to absolutely empty code during pre-processing,
> > > there is no obvious value. The question is whether there is some odd
> > > hackish non-obvious value, I'd expect. (I'd also expect that to be
> > > another "no".)
> > >
> > > If something that evaluates to nothingness ("There was nothing
> > > left...not even a hole!") actually does anything, then somebody in
> > > the standards-compilers-users pipeline needs to be violently beaten
> > > for stupidity.
> >
> >
> > actually, one of the folks on the KJ list found this:
> >
> > http://www.ussg.iu.edu/hypermail/linux/kernel/9605/1957.html
> >
> > which speaks thusly:
> >
> > ...
> > -#if __GNUC__ < 2 || (__GNUC__ == 2 && __GNUC_MINOR__ < 5)
> > -# define NORET_TYPE __volatile__
> > -# define ATTRIB_NORET /**/
> > -# define NORET_AND /**/
> > -#else
> > # define NORET_TYPE /**/
> > # define ATTRIB_NORET __attribute__((noreturn))
> > # define NORET_AND noreturn,
> > -#endif
> > ...
> >
> > so it looks like a thoroughly obsolete macro which can be tossed.
> > i'll make the patch and test it.
>
> AFAICT, NORET_TYPE must've been introduced to silence gcc _warnings_,
> and not do actually do anything useful that affects functionality in any way.
> So the way to "test" your patch would be to see if there is any increase /
> decrease in the number of *warnings* blurted out by gcc during kernel build
> (best would be to build with various gcc versions on various platforms :-)
But of course, John is *bang right* that if /**/ makes gcc do (or not do)
_anything at all_ (be it crying out a warning), then someone surely
deserves to get his head bashed in ... :-)
"Satyam Sharma" <[email protected]> writes:
> But of course, John is *bang right* that if /**/ makes gcc do (or not do)
> _anything at all_ (be it crying out a warning), then someone surely
> deserves to get his head bashed in ... :-)
I think it's impossible, comments are removed by preprocessor.
--
Krzysztof Halasa
On Tue, 22 May 2007, Satyam Sharma wrote:
> On 5/22/07, Robert P. J. Day <[email protected]> wrote:
> > actually, one of the folks on the KJ list found this:
> >
> > http://www.ussg.iu.edu/hypermail/linux/kernel/9605/1957.html
> >
> > which speaks thusly:
> >
> > ...
> > -#if __GNUC__ < 2 || (__GNUC__ == 2 && __GNUC_MINOR__ < 5)
> > -# define NORET_TYPE __volatile__
> > -# define ATTRIB_NORET /**/
> > -# define NORET_AND /**/
> > -#else
> > # define NORET_TYPE /**/
> > # define ATTRIB_NORET __attribute__((noreturn))
> > # define NORET_AND noreturn,
> > -#endif
> > ...
> >
> > so it looks like a thoroughly obsolete macro which can be tossed.
> > i'll make the patch and test it.
>
> AFAICT, NORET_TYPE must've been introduced to silence gcc
> _warnings_, and not do actually do anything useful that affects
> functionality in any way. So the way to "test" your patch would be
> to see if there is any increase / decrease in the number of
> *warnings* blurted out by gcc during kernel build (best would be to
> build with various gcc versions on various platforms :-)
i can understand that logic but, since that macro wouldn't have any
effect since gcc-2.2.5 and the current version of the kernel won't
even *build* with gcc < 3.2, i can't imagine how there could be any
difference these days.
of course, i've been unpleasantly surprised before.
rday
--
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA
http://fsdev.net/wiki/index.php?title=Main_Page
========================================================================
On 5/22/07, Krzysztof Halasa <[email protected]> wrote:
> "Satyam Sharma" <[email protected]> writes:
>
> > But of course, John is *bang right* that if /**/ makes gcc do (or not do)
> > _anything at all_ (be it crying out a warning), then someone surely
> > deserves to get his head bashed in ... :-)
>
> I think it's impossible, comments are removed by preprocessor.
Right -- was just trying to imagine "hard" why that macro existed.
Rday, kill it off!
On Tue, May 22, 2007 at 10:04:16AM -0400, Robert P. J. Day wrote:
> On Tue, 22 May 2007, Adrian Bunk wrote:
>
> > On Tue, May 22, 2007 at 09:09:22AM -0400, Robert P. J. Day wrote:
> > >
> > > given that:
> > >
> > > $ grep -r "define.*NORET_TYPE" *
> > > include/linux/ext4_fs.h:# define NORET_TYPE /**/
> > > include/linux/linkage.h:#define NORET_TYPE /**/
> > > include/linux/ext3_fs.h:# define NORET_TYPE /**/
> > > $
> > >
> > > is there any obvious value to the 30 or so uses of that macro
> > > sprinkled throughout the tree?
> >
> > I can be mistaken, but it seems the uses should be replaced with your
> > __noreturn
>
> no, i think you're thinking of the alternative ATTRIB_NORET macro.
> as you can read in my previous post, NORET_TYPE used to resolve to
> "__volatile__" for very old gcc. so i think it's legitimately dead
> and can be ripped out.
No doubt that it could be removed because it doesn't have any effect.
But locking at the usages, it seems to have been used when people
thought it was what __noreturn now is, so replacing NORET_TYPE with
__noreturn might be a small optimization (but every NORET_TYPE should
be checked that it's actually correct).
> rday
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
On 5/22/07, Adrian Bunk <[email protected]> wrote:
> On Tue, May 22, 2007 at 10:04:16AM -0400, Robert P. J. Day wrote:
> > On Tue, 22 May 2007, Adrian Bunk wrote:
> >
> > > On Tue, May 22, 2007 at 09:09:22AM -0400, Robert P. J. Day wrote:
> > > >
> > > > given that:
> > > >
> > > > $ grep -r "define.*NORET_TYPE" *
> > > > include/linux/ext4_fs.h:# define NORET_TYPE /**/
> > > > include/linux/linkage.h:#define NORET_TYPE /**/
> > > > include/linux/ext3_fs.h:# define NORET_TYPE /**/
> > > > $
> > > >
> > > > is there any obvious value to the 30 or so uses of that macro
> > > > sprinkled throughout the tree?
> > >
> > > I can be mistaken, but it seems the uses should be replaced with your
> > > __noreturn
> >
> > no, i think you're thinking of the alternative ATTRIB_NORET macro.
> > as you can read in my previous post, NORET_TYPE used to resolve to
> > "__volatile__" for very old gcc. so i think it's legitimately dead
> > and can be ripped out.
>
> No doubt that it could be removed because it doesn't have any effect.
>
> But locking at the usages, it seems to have been used when people
> thought it was what __noreturn now is, so replacing NORET_TYPE with
> __noreturn might be a small optimization (but every NORET_TYPE should
> be checked that it's actually correct).
Adrian's right, and in fact from ...
http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html
... we know why that macro existed and evaluated to __volatile__
for pre-2.5 gcc. Perhaps most of today's users of NORET_TYPE were
probably looking for ATTRIB_NORET (which is quite common) actually.
[ __noreturn is defined in compiler-gcc.h, so the easier way would be
to kill NORET_TYPE and replace its usages with ATTRIB_NORET instead.
Of course, after verifying that the function _really_ never returns. ]
On Tue, 22 May 2007, Satyam Sharma wrote:
> On 5/22/07, Adrian Bunk <[email protected]> wrote:
> > On Tue, May 22, 2007 at 10:04:16AM -0400, Robert P. J. Day wrote:
...
> > > no, i think you're thinking of the alternative ATTRIB_NORET
> > > macro. as you can read in my previous post, NORET_TYPE used to
> > > resolve to "__volatile__" for very old gcc. so i think it's
> > > legitimately dead and can be ripped out.
> >
> > No doubt that it could be removed because it doesn't have any
> > effect.
> >
> > But locking at the usages, it seems to have been used when people
> > thought it was what __noreturn now is, so replacing NORET_TYPE
> > with __noreturn might be a small optimization (but every
> > NORET_TYPE should be checked that it's actually correct).
>
> Adrian's right, and in fact from ...
>
> http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html
>
> ... we know why that macro existed and evaluated to __volatile__ for
> pre-2.5 gcc. Perhaps most of today's users of NORET_TYPE were
> probably looking for ATTRIB_NORET (which is quite common) actually.
>
> [ __noreturn is defined in compiler-gcc.h, so the easier way would
> be to kill NORET_TYPE and replace its usages with ATTRIB_NORET
> instead. Of course, after verifying that the function _really_ never
> returns. ]
oh, barf. why does this always happen to me? :-) i just verified
that a patch i threw together to rid the tree of NORET_TYPE does in
fact (and not surprisingly) produce *exactly* the same 7000+ lines of
output and, based on what we've established so far, that's exactly
what it should have done.
at this point, i'd prefer to submit that patch as is for a couple
reasons. first, it's a no-brainer if all it does is remove every
reference to NORET_TYPE and leave everything else untouched. if i go
beyond that, then i'm starting to make judgment calls and possibly
changing the logic, which makes this a totally different cleanup job.
also, in a number of cases, there are some routines that are qualified
with *both* NORET_TYPE and ATTRIB_NORET, suggesting that some
developers knew very well what the difference was, and i was going to
deal with all that ATTRIB_NORET nonsense in another pass.
in short, i'd rather keep this as an aesthetic, no-op kind of cleanup,
otherwise, i just *know* things are going to get ugly. the final
patch will follow shortly.
rday
--
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA
http://fsdev.net/wiki/index.php?title=Main_Page
========================================================================
Adrian Bunk <[email protected]> writes:
>> no, i think you're thinking of the alternative ATTRIB_NORET macro.
>> as you can read in my previous post, NORET_TYPE used to resolve to
>> "__volatile__" for very old gcc. so i think it's legitimately dead
>> and can be ripped out.
>
> No doubt that it could be removed because it doesn't have any effect.
>
> But locking at the usages, it seems to have been used when people
> thought it was what __noreturn now is, so replacing NORET_TYPE with
> __noreturn might be a small optimization (but every NORET_TYPE should
> be checked that it's actually correct).
Actually it seems all the NORET_TYPEs are in fact (should be)
__attribute__((noreturn)) (2.6.21):
Almost certainly:
arch/mips/kernel/traps.c:NORET_TYPE void ATTRIB_NORET die()
arch/ia64/kernel/machine_kexec.c:typedef NORET_TYPE void ()(
arch/x86_64/kernel/machine_kexec.c:NORET_TYPE void machine_kexec()
arch/arm26/kernel/traps.c:NORET_TYPE void die()
arch/ppc/kernel/machine_kexec.c:typedef NORET_TYPE void ()(
arch/ppc/kernel/machine_kexec.c:NORET_TYPE void machine_kexec()
arch/ppc/amiga/config.c:static NORET_TYPE void amiga_reset()
arch/powerpc/kernel/machine_kexec.c:NORET_TYPE void machine_kexec()
arch/powerpc/kernel/machine_kexec_64.c:extern NORET_TYPE void kexec_sequence()
arch/powerpc/kernel/machine_kexec_32.c:typedef NORET_TYPE void ()(
arch/sh/kernel/machine_kexec.c:typedef NORET_TYPE void ()(
arch/sh/kernel/machine_kexec.c:NORET_TYPE void machine_kexec()
arch/arm/kernel/traps.c:NORET_TYPE void die()
arch/i386/kernel/machine_kexec.c:NORET_TYPE void machine_kexec()
arch/m68k/amiga/config.c:static NORET_TYPE void amiga_reset()
drivers/s390/s390mach.c:static NORET_TYPE void s390_handle_damage()
include/asm-i386/kexec.h:asmlinkage NORET_TYPE void
include/asm-mips/ptrace.h:extern NORET_TYPE void die();
include/asm-x86_64/kexec.h:NORET_TYPE void
include/linux/kexec.h:extern NORET_TYPE void machine_kexec() ATTRIB_NORET;
include/linux/linkage.h:#define NORET_TYPE /**/
include/linux/kernel.h:NORET_TYPE void panic()
include/linux/kernel.h:fastcall NORET_TYPE void do_exit()
include/linux/kernel.h:NORET_TYPE void complete_and_exit()
include/linux/sched.h:extern NORET_TYPE void do_group_exit();
kernel/exit.c:fastcall NORET_TYPE void do_exit()
kernel/exit.c:NORET_TYPE void complete_and_exit()
kernel/exit.c:NORET_TYPE void do_group_exit()
kernel/panic.c:NORET_TYPE void panic()
The remaining 2 ones should most probably be removed, e2fsprogs
don't use it either (it was probably used by ext[23] in-kernel
code in the past):
include/linux/ext4_fs.h:# define NORET_TYPE /**/
include/linux/ext3_fs.h:# define NORET_TYPE /**/
--
Krzysztof Halasa
On Tue, 22 May 2007, Krzysztof Halasa wrote:
> "Satyam Sharma" <[email protected]> writes:
>
> > But of course, John is *bang right* that if /**/ makes gcc do (or not do)
> > _anything at all_ (be it crying out a warning), then someone surely
> > deserves to get his head bashed in ... :-)
>
> I think it's impossible, comments are removed by preprocessor.
Maybe it's used by some lint-alike tools?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Tue, 22 May 2007, Krzysztof Halasa wrote:
> Adrian Bunk <[email protected]> writes:
>
> >> no, i think you're thinking of the alternative ATTRIB_NORET macro.
> >> as you can read in my previous post, NORET_TYPE used to resolve to
> >> "__volatile__" for very old gcc. so i think it's legitimately dead
> >> and can be ripped out.
> >
> > No doubt that it could be removed because it doesn't have any effect.
> >
> > But locking at the usages, it seems to have been used when people
> > thought it was what __noreturn now is, so replacing NORET_TYPE with
> > __noreturn might be a small optimization (but every NORET_TYPE should
> > be checked that it's actually correct).
>
> Actually it seems all the NORET_TYPEs are in fact (should be)
> __attribute__((noreturn)) (2.6.21):
that may be but, as i suggested earlier, that would get into guessing
what those developers were thinking, and i just didn't want to go
there.
the simple version of the patch is now in andrew's tree, and i'll
worry about the harder stuff next time.
rday
--
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA
http://fsdev.net/wiki/index.php?title=Main_Page
========================================================================
"Robert P. J. Day" <[email protected]> writes:
> that may be but, as i suggested earlier, that would get into guessing
> what those developers were thinking, and i just didn't want to go
> there.
No guessing, I just checked it (though a second check wouldn't do
any harm).
> the simple version of the patch is now in andrew's tree, and i'll
> worry about the harder stuff next time.
The "next time" would be much harder as there would be no key for
searching for these functions.
--
Krzysztof Halasa
"Robert P. J. Day" <[email protected]> writes:
> that may be but, as i suggested earlier, that would get into guessing
> what those developers were thinking, and i just didn't want to go
> there.
>
> the simple version of the patch is now in andrew's tree, and i'll
> worry about the harder stuff next time.
Ok, we will see.
--
Krzysztof Halasa
On 5/23/07, Krzysztof Halasa <[email protected]> wrote:
> "Robert P. J. Day" <[email protected]> writes:
>
> > that may be but, as i suggested earlier, that would get into guessing
> > what those developers were thinking, and i just didn't want to go
> > there.
>
> No guessing, I just checked it (though a second check wouldn't do
> any harm).
>
> > the simple version of the patch is now in andrew's tree, and i'll
> > worry about the harder stuff next time.
>
> The "next time" would be much harder as there would be no key for
> searching for these functions.
Krzysztof's absolutely right ... we don't want to lose the NORET_TYPE
annotations on all these functions before we switch them to
ATTRIB_NORET. And yes, _all_ of these NORET_TYPE's do want
to be ATTRIB_NORET (except for those that are double-annotated,
for those we can just get rid of the NORET_TYPE macro).
On Wed, 23 May 2007, Satyam Sharma wrote:
> On 5/23/07, Krzysztof Halasa <[email protected]> wrote:
> > "Robert P. J. Day" <[email protected]> writes:
> >
> > > that may be but, as i suggested earlier, that would get into
> > > guessing what those developers were thinking, and i just didn't
> > > want to go there.
> >
> > No guessing, I just checked it (though a second check wouldn't do
> > any harm).
> >
> > > the simple version of the patch is now in andrew's tree, and
> > > i'll worry about the harder stuff next time.
> >
> > The "next time" would be much harder as there would be no key for
> > searching for these functions.
>
> Krzysztof's absolutely right ... we don't want to lose the
> NORET_TYPE annotations on all these functions before we switch them
> to ATTRIB_NORET. And yes, _all_ of these NORET_TYPE's do want to be
> ATTRIB_NORET (except for those that are double-annotated, for those
> we can just get rid of the NORET_TYPE macro).
ok, i'll go back and take another look at this.
rday
--
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA
http://fsdev.net/wiki/index.php?title=Main_Page
========================================================================
Hi Robert,
On 5/23/07, Robert P. J. Day <[email protected]> wrote:
> On Wed, 23 May 2007, Satyam Sharma wrote:
>
> > On 5/23/07, Krzysztof Halasa <[email protected]> wrote:
> > > "Robert P. J. Day" <[email protected]> writes:
> > >
> > > > that may be but, as i suggested earlier, that would get into
> > > > guessing what those developers were thinking, and i just didn't
> > > > want to go there.
> > >
> > > No guessing, I just checked it (though a second check wouldn't do
> > > any harm).
> > >
> > > > the simple version of the patch is now in andrew's tree, and
> > > > i'll worry about the harder stuff next time.
> > >
> > > The "next time" would be much harder as there would be no key for
> > > searching for these functions.
> >
> > Krzysztof's absolutely right ... we don't want to lose the
> > NORET_TYPE annotations on all these functions before we switch them
> > to ATTRIB_NORET. And yes, _all_ of these NORET_TYPE's do want to be
> > ATTRIB_NORET (except for those that are double-annotated, for those
> > we can just get rid of the NORET_TYPE macro).
>
> ok, i'll go back and take another look at this.
Actually there's another thing :-) The __attribute__((xxx)) must go with the
function _declarations_ (and not the implementations/definitions). I noticed
after my previous mail that most of the double annotations are actually in
the case of the _declarations_ of these non-returning functions, whereas
most of the single-occurrences of NORET_TYPE were in the function
definitions, which means your patch that simply got rid of NORET_TYPE
actually ended up doing exactly the right thing that we wanted :-)
The unfortunate / ugly part, however, is that there are few cases where
the above doesn't hold true ([1] NORET_TYPE without ATTRIB_NORET in
function declaration e.g. in include/asm-mips/ptrace.h -- this must
become ATTRIB_NORET, and [2] ATTRIB_NORET either alone or with
NORET_TYPE in function definitions e.g. arch/mips/kernel/traps.c -- these
can be eliminated entirely, and [3] NORET_AND occurrences where we
don't really want ATTRIB_NORET anymore, so just removing
NORET_TYPE would be right).
So this might not really be a simple patch anymore, and because the
benefit of ATTRIB_NORET is only miniscule, so you can ignore my
previous mail if you want :-)
Thanks,
Satyam
"Satyam Sharma" <[email protected]> writes:
> Actually there's another thing :-) The __attribute__((xxx)) must go with the
> function _declarations_ (and not the implementations/definitions). I noticed
> after my previous mail that most of the double annotations are actually in
> the case of the _declarations_ of these non-returning functions, whereas
> most of the single-occurrences of NORET_TYPE were in the function
> definitions, which means your patch that simply got rid of NORET_TYPE
> actually ended up doing exactly the right thing that we wanted :-)
Only half of it. Half of right thing is (here) a bad thing.
NORET_TYPE does not do any harm.
Removing it removes pointers to attrib((noreturn)) candidates.
Simple.
--
Krzysztof Halasa
On Thu, 24 May 2007, Krzysztof Halasa wrote:
> "Satyam Sharma" <[email protected]> writes:
>
> > Actually there's another thing :-) The __attribute__((xxx)) must go with the
> > function _declarations_ (and not the implementations/definitions). I noticed
> > after my previous mail that most of the double annotations are actually in
> > the case of the _declarations_ of these non-returning functions, whereas
> > most of the single-occurrences of NORET_TYPE were in the function
> > definitions, which means your patch that simply got rid of NORET_TYPE
> > actually ended up doing exactly the right thing that we wanted :-)
>
> Only half of it. Half of right thing is (here) a bad thing.
>
> NORET_TYPE does not do any harm.
> Removing it removes pointers to attrib((noreturn)) candidates.
> Simple.
everybody take a valium -- i asked andrew to drop that patch from the
mm-tree until i get the time to go back and take a closer look.
rday
--
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA
http://fsdev.net/wiki/index.php?title=Main_Page
========================================================================
On Wed, 23 May 2007, Satyam Sharma wrote:
> Actually there's another thing :-) The __attribute__((xxx)) must go
> with the function _declarations_ (and not the
> implementations/definitions).
that's not true, AFAICT. the pattern seems to be that, in the case of
declarations, attributes go at the end, as in:
arch/sparc/boot/btfixupprep.c:void fatal(void) __attribute__((noreturn));
while in actual definitions, they typically go after the return type,
as in arch/arm/kernel/traps.c:
void __attribute__((noreturn)) __bug(const char *file, int line)
{
printk(KERN_CRIT"kernel BUG at %s:%d!\n", file, line);
*(int *)0 = 0;
/* Avoid "noreturn function does return" */
for (;;);
}
is that the standard that most people use? if it is, i can use that
standard for the cleanup.
rday
--
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA
http://fsdev.net/wiki/index.php?title=Main_Page
========================================================================
On 5/24/07, Krzysztof Halasa <[email protected]> wrote:
> "Satyam Sharma" <[email protected]> writes:
>
> > Actually there's another thing :-) The __attribute__((xxx)) must go with the
> > function _declarations_ (and not the implementations/definitions). I noticed
> > after my previous mail that most of the double annotations are actually in
> > the case of the _declarations_ of these non-returning functions, whereas
> > most of the single-occurrences of NORET_TYPE were in the function
> > definitions, which means your patch that simply got rid of NORET_TYPE
> > actually ended up doing exactly the right thing that we wanted :-)
>
> Only half of it. Half of right thing is (here) a bad thing.
>
> NORET_TYPE does not do any harm.
> Removing it removes pointers to attrib((noreturn)) candidates.
> Simple.
NORET_TYPE didn't do any harm because it expanded to /**/. But yes,
removing it does remove the annotations / markers for ATTRIB_NORET
candidates, as I first mentioned to Rday.
But later on going through the code, I noticed that:
1. Most of the combined occurrences of NORET_TYPE along with
ATTRIB_NORET were in function _declarations_. So here, what
we want is to simply remove the NORET_TYPE, and *not* replace
it with ATTRIB_NORET, because it's already there!
2. Most of the single-occurrences (NORET_TYPE alone, without any
ATTRIB_NORET) were in function _implementations_. So here, what
we want is to simply remove the NORET_TYPE, and *not* introduce
any ATTRIB_NORET, because it's not required!
In such a situation, merging a patch that simply gets rid of
NORET_TYPE (i.e. s/NORET_TYPE//) actually ends up doing the
right thing!
... *however* ... as I mentioned next, unfortunately the above 2 points
did not hold for _all_ occurrences of NORET_TYPE. There were
some exceptions (which also I mentioned) where we want to do
somethings slightly more complicated, so I put the ball in Rday's
court -- he had originally expressed a wish to avoid doing complicated
things in this patch.
On 5/24/07, Robert P. J. Day <[email protected]> wrote:
> On Wed, 23 May 2007, Satyam Sharma wrote:
>
> > Actually there's another thing :-) The __attribute__((xxx)) must go
> > with the function _declarations_ (and not the
> > implementations/definitions).
>
> that's not true, AFAICT. the pattern seems to be that, in the case of
> declarations, attributes go at the end, as in:
>
> arch/sparc/boot/btfixupprep.c:void fatal(void) __attribute__((noreturn));
Yes, please, this is the normal way.
> while in actual definitions, they typically go after the return type,
> as in arch/arm/kernel/traps.c:
>
> void __attribute__((noreturn)) __bug(const char *file, int line)
> {
> printk(KERN_CRIT"kernel BUG at %s:%d!\n", file, line);
> *(int *)0 = 0;
>
> /* Avoid "noreturn function does return" */
> for (;;);
> }
Function attributes don't need to appear in function definitions at all.
(I've not come across such cases often, so this is certainly not the
standard). Anyway, the above is a case that the function lacks a
separate declaration which is why we put it there at the definition.
And yes, if we really do have to put an attribute in the function
definition, then we do need to place it _before_ the function name.
> is that the standard that most people use? if it is, i can use that
> standard for the cleanup.
Ok, so if you're really planning to do this cleanup, you can do the
following (for all occurrences of NORET_TYPE):
1. If this is a function _declaration_ (i.e. a prototype in some
header or some .c file), then remove the NORET_TYPE macro. Also,
if an ATTRIB_NORET or NORET_AND already exists then you're done.
Else, introduce an ATTRIB_NORET after the arglist but before ;
2. If this is a function _definition_, then remove the NORET_TYPE
macro.
(a) If a _declaration_ also exists somewhere (say, some
header or in the same .c file) for this function, then go
and check it.
(i) If it already contains an ATTRIB_NORET or
NORET_AND, then you're done.
(ii) Else, introduce ATTRIB_NORET.
(b) If no separate declaration exists for this function,
then just introduce the ATTRIB_NORET _before_ the
function name if it isn't there already.
I suspect the above would cover all cases :-)
On Fri, 25 May 2007, Satyam Sharma wrote:
> On 5/24/07, Robert P. J. Day <[email protected]> wrote:
> > while in actual definitions, they typically go after the return type,
> > as in arch/arm/kernel/traps.c:
> >
> > void __attribute__((noreturn)) __bug(const char *file, int line)
> > {
> > printk(KERN_CRIT"kernel BUG at %s:%d!\n", file, line);
> > *(int *)0 = 0;
> >
> > /* Avoid "noreturn function does return" */
> > for (;;);
> > }
>
> Function attributes don't need to appear in function definitions at all.
> (I've not come across such cases often, so this is certainly not the
> standard). Anyway, the above is a case that the function lacks a
> separate declaration which is why we put it there at the definition.
> And yes, if we really do have to put an attribute in the function
> definition, then we do need to place it _before_ the function name.
right -- the above was referring only to those instances (and there
are some of them) where there is no declaration, only a definition.
i'll take another shot at a patch shortly.
rday
--
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA
http://fsdev.net/wiki/index.php?title=Main_Page
========================================================================
On Fri, 25 May 2007, Satyam Sharma wrote:
...
> 1. If this is a function _declaration_ (i.e. a prototype in some
> header or some .c file), then remove the NORET_TYPE macro. Also,
> if an ATTRIB_NORET or NORET_AND already exists then you're done.
> Else, introduce an ATTRIB_NORET after the arglist but before ;
actually, what i would be introducing in all cases is "__noreturn",
the short form currently defined in compiler-gcc.h. and i would be
removing every instance of ATTRIB_NORET and its buddies.
rday
--
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA
http://fsdev.net/wiki/index.php?title=Main_Page
========================================================================
Hi Robert,
On 5/25/07, Robert P. J. Day <[email protected]> wrote:
> On Fri, 25 May 2007, Satyam Sharma wrote:
> ...
> > 1. If this is a function _declaration_ (i.e. a prototype in some
> > header or some .c file), then remove the NORET_TYPE macro. Also,
> > if an ATTRIB_NORET or NORET_AND already exists then you're done.
> > Else, introduce an ATTRIB_NORET after the arglist but before ;
>
> actually, what i would be introducing in all cases is "__noreturn",
> the short form currently defined in compiler-gcc.h. and i would be
> removing every instance of ATTRIB_NORET and its buddies.
Ummm ... you mean we're replacing all occurrences of ATTRIB_NORET
as well? Note that NORET_TYPE and ATTRIB_NORET are both defined
in the generic include/linux/linkage.h whereas __noreturn is in
compiler-gcc.h which is included only for gcc builds -- hence, my
preference for ATTRIB_NORET. Also, there is not even a single user of
__noreturn anywhere in the kernel code whereas ATTRIB_NORET is used
in all these places, which means it looks like to be the standard thing ...
Anyway, I'm fine either way.
Thanks,
Satyam
On Sat, 26 May 2007, Satyam Sharma wrote:
> Hi Robert,
>
> On 5/25/07, Robert P. J. Day <[email protected]> wrote:
> > On Fri, 25 May 2007, Satyam Sharma wrote:
> > ...
> > > 1. If this is a function _declaration_ (i.e. a prototype in some
> > > header or some .c file), then remove the NORET_TYPE macro. Also,
> > > if an ATTRIB_NORET or NORET_AND already exists then you're done.
> > > Else, introduce an ATTRIB_NORET after the arglist but before ;
> >
> > actually, what i would be introducing in all cases is "__noreturn",
> > the short form currently defined in compiler-gcc.h. and i would be
> > removing every instance of ATTRIB_NORET and its buddies.
>
> Ummm ... you mean we're replacing all occurrences of ATTRIB_NORET
> as well? Note that NORET_TYPE and ATTRIB_NORET are both defined
> in the generic include/linux/linkage.h whereas __noreturn is in
> compiler-gcc.h which is included only for gcc builds -- hence, my
> preference for ATTRIB_NORET. Also, there is not even a single user of
> __noreturn anywhere in the kernel code whereas ATTRIB_NORET is used
> in all these places, which means it looks like to be the standard thing ...
> Anyway, I'm fine either way.
ah, i hadn't noticed that. i must think on this more. man, i thought
this was going to be so simple. argh.
rday
--
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA
http://fsdev.net/wiki/index.php?title=Main_Page
========================================================================
On Fri, May 25, 2007 at 03:40:18PM -0400, Robert P. J. Day wrote:
> On Sat, 26 May 2007, Satyam Sharma wrote:
>
> > Hi Robert,
> >
> > On 5/25/07, Robert P. J. Day <[email protected]> wrote:
> > > On Fri, 25 May 2007, Satyam Sharma wrote:
> > > ...
> > > > 1. If this is a function _declaration_ (i.e. a prototype in some
> > > > header or some .c file), then remove the NORET_TYPE macro. Also,
> > > > if an ATTRIB_NORET or NORET_AND already exists then you're done.
> > > > Else, introduce an ATTRIB_NORET after the arglist but before ;
> > >
> > > actually, what i would be introducing in all cases is "__noreturn",
> > > the short form currently defined in compiler-gcc.h. and i would be
> > > removing every instance of ATTRIB_NORET and its buddies.
> >
> > Ummm ... you mean we're replacing all occurrences of ATTRIB_NORET
> > as well? Note that NORET_TYPE and ATTRIB_NORET are both defined
> > in the generic include/linux/linkage.h whereas __noreturn is in
> > compiler-gcc.h which is included only for gcc builds -- hence, my
> > preference for ATTRIB_NORET. Also, there is not even a single user of
> > __noreturn anywhere in the kernel code whereas ATTRIB_NORET is used
> > in all these places, which means it looks like to be the standard thing ...
> > Anyway, I'm fine either way.
>
> ah, i hadn't noticed that. i must think on this more. man, i thought
> this was going to be so simple. argh.
It's only an optimization, so defining __noreturn to nothing for other
compilers should work fine.
> rday
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
"Robert P. J. Day" <[email protected]> writes:
> that's not true, AFAICT. the pattern seems to be that, in the case of
> declarations, attributes go at the end, as in:
Anyway, that attribute has to be in the declarations as without it
there the compiler can't optimize the callers.
--
Krzysztof Halasa