[PATCH] MIPS: Changed current_thread_info() to an equivalent supported by both clang and GCC

[PATCH] MIPS: Changed current_thread_info() to an equivalent supported by both clang and GCC

[Daniel Sanders]

Without this, a 'break' instruction is executed very early in the boot and
the boot hangs.

The problem is that clang doesn't honour named registers on local variables
and silently treats them as normal uninitialized variables. However, it
does honour them on global variables.

Signed-off-by: Daniel Sanders <daniel.sanders@imgtec.com>
---
 arch/mips/include/asm/thread_info.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

For reference, a similar patch for ARM's stack pointer has already been merged:
  0abc08b ARM: 8170/1: Add global named register current_stack_pointer for ARM

This is part of a patch series to get Linux for Mips working when compiled with
clang. I've chosen to submit this patch individually since it's my first kernel
patch and I'd like to be sure I'm following your processes correctly before I
submit all of them.

Please CC me on replies since I'm not subscribed to the mailing list.

diff --git a/arch/mips/include/asm/thread_info.h b/arch/mips/include/asm/thread_info.h
index 99eea59..2a2f3c4 100644
--- a/arch/mips/include/asm/thread_info.h
+++ b/arch/mips/include/asm/thread_info.h
@@ -58,11 +58,11 @@ struct thread_info {
 #define init_stack		(init_thread_union.stack)
 
 /* How to get the thread information struct from C.  */
+register struct thread_info *current_gp_register asm("$28");
+
 static inline struct thread_info *current_thread_info(void)
 {
-	register struct thread_info *__current_thread_info __asm__("$28");
-
-	return __current_thread_info;
+	return current_gp_register;
 }
 
 #endif /* !__ASSEMBLY__ */
-- 
2.1.3

[PATCH] MIPS: Changed current_thread_info() to an equivalent supported by both clang and GCC

[Daniel Sanders]

Without this, a 'break' instruction is executed very early in the boot and
the boot hangs.

The problem is that clang doesn't honour named registers on local variables
and silently treats them as normal uninitialized variables. However, it
does honour them on global variables.

Signed-off-by: Daniel Sanders <daniel.sanders@imgtec.com>
---
 arch/mips/include/asm/thread_info.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

For reference, a similar patch for ARM's stack pointer has already been merged:
  0abc08b ARM: 8170/1: Add global named register current_stack_pointer for ARM

This is part of a patch series to get Linux for Mips working when compiled with
clang. I've chosen to submit this patch individually since it's my first kernel
patch and I'd like to be sure I'm following your processes correctly before I
submit all of them.

Please CC me on replies since I'm not subscribed to the mailing list.

diff --git a/arch/mips/include/asm/thread_info.h b/arch/mips/include/asm/thread_info.h
index 99eea59..2a2f3c4 100644
--- a/arch/mips/include/asm/thread_info.h
+++ b/arch/mips/include/asm/thread_info.h
@@ -58,11 +58,11 @@ struct thread_info {
 #define init_stack		(init_thread_union.stack)
 
 /* How to get the thread information struct from C.  */
+register struct thread_info *current_gp_register asm("$28");
+
 static inline struct thread_info *current_thread_info(void)
 {
-	register struct thread_info *__current_thread_info __asm__("$28");
-
-	return __current_thread_info;
+	return current_gp_register;
 }
 
 #endif /* !__ASSEMBLY__ */
-- 
2.1.3

Re: [PATCH] MIPS: Changed current_thread_info() to an equivalent supported by both clang and GCC

[Sergei Shtylyov]

Hello.

On 1/9/2015 3:06 PM, Daniel Sanders wrote:

> Without this, a 'break' instruction is executed very early in the boot and
> the boot hangs.

> The problem is that clang doesn't honour named registers on local variables
> and silently treats them as normal uninitialized variables. However, it
> does honour them on global variables.

> Signed-off-by: Daniel Sanders <daniel.sanders@imgtec.com>

[...]

> diff --git a/arch/mips/include/asm/thread_info.h b/arch/mips/include/asm/thread_info.h
> index 99eea59..2a2f3c4 100644
> --- a/arch/mips/include/asm/thread_info.h
> +++ b/arch/mips/include/asm/thread_info.h
> @@ -58,11 +58,11 @@ struct thread_info {
>   #define init_stack		(init_thread_union.stack)
>
>   /* How to get the thread information struct from C.  */
> +register struct thread_info *current_gp_register asm("$28");

    *static* missing?

WBR, Sergei

RE: [PATCH] MIPS: Changed current_thread_info() to an equivalent supported by both clang and GCC

[Daniel Sanders]

Hi,

Thanks for the quick reply.

> -----Original Message-----
> From: Sergei Shtylyov [mailto:sergei.shtylyov@cogentembedded.com]
> Sent: 09 January 2015 12:18
> To: Daniel Sanders; linux-mips@linux-mips.org; Ralf Baechle
> Cc: Paul Burton; Markos Chandras; James Hogan; Behan Webster
> Subject: Re: [PATCH] MIPS: Changed current_thread_info() to an equivalent
> supported by both clang and GCC
> 
> Hello.
> 
> On 1/9/2015 3:06 PM, Daniel Sanders wrote:
> 
> > Without this, a 'break' instruction is executed very early in the boot and
> > the boot hangs.
> 
> > The problem is that clang doesn't honour named registers on local variables
> > and silently treats them as normal uninitialized variables. However, it
> > does honour them on global variables.
> 
> > Signed-off-by: Daniel Sanders <daniel.sanders@imgtec.com>
> 
> [...]
> 
> > diff --git a/arch/mips/include/asm/thread_info.h
> b/arch/mips/include/asm/thread_info.h
> > index 99eea59..2a2f3c4 100644
> > --- a/arch/mips/include/asm/thread_info.h
> > +++ b/arch/mips/include/asm/thread_info.h
> > @@ -58,11 +58,11 @@ struct thread_info {
> >   #define init_stack		(init_thread_union.stack)
> >
> >   /* How to get the thread information struct from C.  */
> > +register struct thread_info *current_gp_register asm("$28");
> 
>     *static* missing?
> 
> WBR, Sergei

Combining 'register' and 'static' is invalid.

gcc gives:
arch/mips/include/asm/thread_info.h:61:1: error: multiple storage classes in declaration specifiers
 static register struct thread_info *current_gp_register asm("$28");
 ^

and clang gives:
arch/mips/include/asm/thread_info.h:61:8: error: cannot combine with previous 'static' declaration specifier
static register struct thread_info *current_gp_register asm("$28");
       ^

Re: [PATCH] MIPS: Changed current_thread_info() to an equivalent supported by both clang and GCC

[David Daney]

On 01/09/2015 05:23 AM, Daniel Sanders wrote:
> Hi,
>
> Thanks for the quick reply.
>
>> -----Original Message-----
>> From: Sergei Shtylyov [mailto:sergei.shtylyov@cogentembedded.com]
>> Sent: 09 January 2015 12:18
>> To: Daniel Sanders; linux-mips@linux-mips.org; Ralf Baechle
>> Cc: Paul Burton; Markos Chandras; James Hogan; Behan Webster
>> Subject: Re: [PATCH] MIPS: Changed current_thread_info() to an equivalent
>> supported by both clang and GCC
>>
>> Hello.
>>
>> On 1/9/2015 3:06 PM, Daniel Sanders wrote:
>>
>>> Without this, a 'break' instruction is executed very early in the boot and
>>> the boot hangs.
>>
>>> The problem is that clang doesn't honour named registers on local variables
>>> and silently treats them as normal uninitialized variables. However, it
>>> does honour them on global variables.

Why not fix clang instead?

>>
>>> Signed-off-by: Daniel Sanders <daniel.sanders@imgtec.com>
>>
>> [...]
>>
>>> diff --git a/arch/mips/include/asm/thread_info.h
>> b/arch/mips/include/asm/thread_info.h
>>> index 99eea59..2a2f3c4 100644
>>> --- a/arch/mips/include/asm/thread_info.h
>>> +++ b/arch/mips/include/asm/thread_info.h
>>> @@ -58,11 +58,11 @@ struct thread_info {
>>>    #define init_stack		(init_thread_union.stack)
>>>
>>>    /* How to get the thread information struct from C.  */
>>> +register struct thread_info *current_gp_register asm("$28");
>>
>>      *static* missing?
>>
>> WBR, Sergei
>
> Combining 'register' and 'static' is invalid.

Defining global variables in header files is also invalid.

>
> gcc gives:
> arch/mips/include/asm/thread_info.h:61:1: error: multiple storage classes in declaration specifiers
>   static register struct thread_info *current_gp_register asm("$28");
>   ^
>
> and clang gives:
> arch/mips/include/asm/thread_info.h:61:8: error: cannot combine with previous 'static' declaration specifier
> static register struct thread_info *current_gp_register asm("$28");
>         ^
>
>

RE: [PATCH] MIPS: Changed current_thread_info() to an equivalent supported by both clang and GCC

[Daniel Sanders]

> -----Original Message-----
> From: David Daney [mailto:ddaney.cavm@gmail.com]
> Sent: 09 January 2015 17:26
> To: Daniel Sanders
> Cc: Sergei Shtylyov; linux-mips@linux-mips.org; Ralf Baechle; Paul Burton;
> Markos Chandras; James Hogan; Behan Webster
> Subject: Re: [PATCH] MIPS: Changed current_thread_info() to an equivalent
> supported by both clang and GCC
> 
> On 01/09/2015 05:23 AM, Daniel Sanders wrote:
> > Hi,
> >
> > Thanks for the quick reply.
> >
> >> -----Original Message-----
> >> From: Sergei Shtylyov [mailto:sergei.shtylyov@cogentembedded.com]
> >> Sent: 09 January 2015 12:18
> >> To: Daniel Sanders; linux-mips@linux-mips.org; Ralf Baechle
> >> Cc: Paul Burton; Markos Chandras; James Hogan; Behan Webster
> >> Subject: Re: [PATCH] MIPS: Changed current_thread_info() to an equivalent
> >> supported by both clang and GCC
> >>
> >> Hello.
> >>
> >> On 1/9/2015 3:06 PM, Daniel Sanders wrote:
> >>
> >>> Without this, a 'break' instruction is executed very early in the boot and
> >>> the boot hangs.
> >>
> >>> The problem is that clang doesn't honour named registers on local
> variables
> >>> and silently treats them as normal uninitialized variables. However, it
> >>> does honour them on global variables.
> 
> Why not fix clang instead?

There's some significant implementation difficulties in LLVM that appear to stem from it not being designed to accommodate this extension. There were also some objections based on the future direction of LLVM. The thread can be found at http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-March/071555.html. I've linked to the bit where the issues started to be discussed rather than the start of the thread.

Difficulty and objections aside, it's also a very large amount of work to support a single (as far as I know) user of named register locals, especially when Linux has already accepted patches to switch named register locals to named register globals elsewhere. On balance, it seems best to change Linux.

> >>> Signed-off-by: Daniel Sanders <daniel.sanders@imgtec.com>
> >>
> >> [...]
> >>
> >>> diff --git a/arch/mips/include/asm/thread_info.h
> >> b/arch/mips/include/asm/thread_info.h
> >>> index 99eea59..2a2f3c4 100644
> >>> --- a/arch/mips/include/asm/thread_info.h
> >>> +++ b/arch/mips/include/asm/thread_info.h
> >>> @@ -58,11 +58,11 @@ struct thread_info {
> >>>    #define init_stack		(init_thread_union.stack)
> >>>
> >>>    /* How to get the thread information struct from C.  */
> >>> +register struct thread_info *current_gp_register asm("$28");
> >>
> >>      *static* missing?
> >>
> >> WBR, Sergei
> >
> > Combining 'register' and 'static' is invalid.
> 
> Defining global variables in header files is also invalid.

I agree with that statement but named register globals are not the same as normal global variables. In particular, they do not take up space in the data section and they do not have an entry in the symbol table. They can therefore be included in multiple objects without causing link errors.

Re: [PATCH] MIPS: Changed current_thread_info() to an equivalent supported by both clang and GCC

[Behan Webster]

On 01/09/15 12:06, Daniel Sanders wrote:
>> -----Original Message-----
>> From: David Daney [mailto:ddaney.cavm@gmail.com]
>> Sent: 09 January 2015 17:26
>> To: Daniel Sanders
>> Cc: Sergei Shtylyov; linux-mips@linux-mips.org; Ralf Baechle; Paul Burton;
>> Markos Chandras; James Hogan; Behan Webster
>> Subject: Re: [PATCH] MIPS: Changed current_thread_info() to an equivalent
>> supported by both clang and GCC
>>
>> On 01/09/2015 05:23 AM, Daniel Sanders wrote:
>>> Hi,
>>>
>>> Thanks for the quick reply.
>>>
>>>> -----Original Message-----
>>>> From: Sergei Shtylyov [mailto:sergei.shtylyov@cogentembedded.com]
>>>> Sent: 09 January 2015 12:18
>>>> To: Daniel Sanders; linux-mips@linux-mips.org; Ralf Baechle
>>>> Cc: Paul Burton; Markos Chandras; James Hogan; Behan Webster
>>>> Subject: Re: [PATCH] MIPS: Changed current_thread_info() to an equivalent
>>>> supported by both clang and GCC
>>>>
>>>> Hello.
>>>>
>>>> On 1/9/2015 3:06 PM, Daniel Sanders wrote:
>>>>
>>>>> Without this, a 'break' instruction is executed very early in the boot and
>>>>> the boot hangs.
>>>>> The problem is that clang doesn't honour named registers on local
>> variables
>>>>> and silently treats them as normal uninitialized variables. However, it
>>>>> does honour them on global variables.
>> Why not fix clang instead?
> There's some significant implementation difficulties in LLVM that appear to stem from it not being designed to accommodate this extension. There were also some objections based on the future direction of LLVM. The thread can be found at http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-March/071555.html. I've linked to the bit where the issues started to be discussed rather than the start of the thread.
>
> Difficulty and objections aside, it's also a very large amount of work to support a single (as far as I know) user of named register locals, especially when Linux has already accepted patches to switch named register locals to named register globals elsewhere. On balance, it seems best to change Linux.
>
>>>>> Signed-off-by: Daniel Sanders <daniel.sanders@imgtec.com>
>>>> [...]
>>>>
>>>>> diff --git a/arch/mips/include/asm/thread_info.h
>>>> b/arch/mips/include/asm/thread_info.h
>>>>> index 99eea59..2a2f3c4 100644
>>>>> --- a/arch/mips/include/asm/thread_info.h
>>>>> +++ b/arch/mips/include/asm/thread_info.h
>>>>> @@ -58,11 +58,11 @@ struct thread_info {
>>>>>    #define init_stack		(init_thread_union.stack)
>>>>>
>>>>>    /* How to get the thread information struct from C.  */
>>>>> +register struct thread_info *current_gp_register asm("$28");
>>>>      *static* missing?
>>>>
>>>> WBR, Sergei
>>> Combining 'register' and 'static' is invalid.
>> Defining global variables in header files is also invalid.
> I agree with that statement but named register globals are not the same as normal global variables. In particular, they do not take up space in the data section and they do not have an entry in the symbol table. They can therefore be included in multiple objects without causing link errors.
Daniel's code is in line with the solution already used in both the arm
and arm64/aarch64 arches which works with both gcc and clang.

Just in case that helps.

Behan

-- 
Behan Webster
behanw@converseincode.com

Re: [PATCH] MIPS: Changed current_thread_info() to an equivalent supported by both clang and GCC

[David Daney]

On 01/09/2015 12:06 PM, Daniel Sanders wrote:
>> -----Original Message-----
>> From: David Daney [mailto:ddaney.cavm@gmail.com]
[...]

>
>>>>> Signed-off-by: Daniel Sanders <daniel.sanders@imgtec.com>
>>>>
>>>> [...]
>>>>
>>>>> diff --git a/arch/mips/include/asm/thread_info.h
>>>> b/arch/mips/include/asm/thread_info.h
>>>>> index 99eea59..2a2f3c4 100644
>>>>> --- a/arch/mips/include/asm/thread_info.h
>>>>> +++ b/arch/mips/include/asm/thread_info.h
>>>>> @@ -58,11 +58,11 @@ struct thread_info {
>>>>>     #define init_stack		(init_thread_union.stack)
>>>>>
>>>>>     /* How to get the thread information struct from C.  */
>>>>> +register struct thread_info *current_gp_register asm("$28");
>>>>
>>>>       *static* missing?
>>>>
>>>> WBR, Sergei
>>>
>>> Combining 'register' and 'static' is invalid.
>>
>> Defining global variables in header files is also invalid.
>
> I agree with that statement but named register globals are not the same as normal global variables. In particular, they do not take up space in the data section and they do not have an entry in the symbol table. They can therefore be included in multiple objects without causing link errors.
>

Well, the GCC manual seems to bless your usage, so I withdraw my 
objection on making this global.  But, changing the name to 
"current_gp_register" removes information about what it is used for.

Can you resend that patch so that it still has the name 
"__current_thread_info", and only moves it to the global scope?

Thanks,
David Daney


>

RE: [PATCH] MIPS: Changed current_thread_info() to an equivalent supported by both clang and GCC

[Daniel Sanders]

> -----Original Message-----
> From: David Daney [mailto:ddaney.cavm@gmail.com]
> Sent: 09 January 2015 23:53
> To: Daniel Sanders
> Cc: Sergei Shtylyov; linux-mips@linux-mips.org; Ralf Baechle; Paul Burton;
> Markos Chandras; James Hogan; Behan Webster
> Subject: Re: [PATCH] MIPS: Changed current_thread_info() to an equivalent
> supported by both clang and GCC
> 
> On 01/09/2015 12:06 PM, Daniel Sanders wrote:
> >> -----Original Message-----
> >> From: David Daney [mailto:ddaney.cavm@gmail.com]
> [...]
> 
> >
> >>>>> Signed-off-by: Daniel Sanders <daniel.sanders@imgtec.com>
> >>>>
> >>>> [...]
> >>>>
> >>>>> diff --git a/arch/mips/include/asm/thread_info.h
> >>>> b/arch/mips/include/asm/thread_info.h
> >>>>> index 99eea59..2a2f3c4 100644
> >>>>> --- a/arch/mips/include/asm/thread_info.h
> >>>>> +++ b/arch/mips/include/asm/thread_info.h
> >>>>> @@ -58,11 +58,11 @@ struct thread_info {
> >>>>>     #define init_stack		(init_thread_union.stack)
> >>>>>
> >>>>>     /* How to get the thread information struct from C.  */
> >>>>> +register struct thread_info *current_gp_register asm("$28");
> >>>>
> >>>>       *static* missing?
> >>>>
> >>>> WBR, Sergei
> >>>
> >>> Combining 'register' and 'static' is invalid.
> >>
> >> Defining global variables in header files is also invalid.
> >
> > I agree with that statement but named register globals are not the same as
> normal global variables. In particular, they do not take up space in the data
> section and they do not have an entry in the symbol table. They can therefore
> be included in multiple objects without causing link errors.
> >
> 
> Well, the GCC manual seems to bless your usage, so I withdraw my
> objection on making this global.  But, changing the name to
> "current_gp_register" removes information about what it is used for.
> 
> Can you resend that patch so that it still has the name
> "__current_thread_info", and only moves it to the global scope?
> 
> Thanks,
> David Daney

Sure. I'll Cc you directly on the re-send.

The main reason I renamed it is that identifiers starting with '__' are reserved. It's pretty unlikely but it's possible that the name will conflict with a C implementation in the future.

Re: [PATCH] MIPS: Changed current_thread_info() to an equivalent supported by both clang and GCC

[Ralf Baechle]

On Sat, Jan 10, 2015 at 12:53:22PM +0000, Daniel Sanders wrote:

> The main reason I renamed it is that identifiers starting with '__' are reserved. It's pretty unlikely but it's possible that the name will conflict with a C implementation in the future.

The whole kernel is using identifiers starting with a double underscore
left and right.  The risk should be acceptable though - also because the
kernel isn't linked against external libraries.

The sole reason why _current_thread_info was a local variable is so nobody
else can use it - the proper interface to use is current_thread_info().

Other than that, both the current and the proposed variant aren't really
correct for a variable that really is per thread.  So I'm going to just
queue this for 3.20.

Thanks!

  Ralf

Re: [PATCH] MIPS: Changed current_thread_info() to an equivalent supported by both clang and GCC

[Måns Rullgård]

Ralf Baechle <ralf@linux-mips.org> writes:

> On Sat, Jan 10, 2015 at 12:53:22PM +0000, Daniel Sanders wrote:
>
>> The main reason I renamed it is that identifiers starting with '__'
>> are reserved. It's pretty unlikely but it's possible that the name
>> will conflict with a C implementation in the future.
>
> The whole kernel is using identifiers starting with a double underscore
> left and right.  The risk should be acceptable though - also because the
> kernel isn't linked against external libraries.

The reserved namespace applies to applications built against a standard
library so as to avoid name clashes with library internals.  The kernel
doesn't use the standard library, so it can use any identifiers.  This
should be clear from the fact that the reserved identifiers are listed
in the "Library" chapter of the C standard.

-- 
Måns Rullgård
mans@mansr.com

Re: [PATCH] MIPS: Changed current_thread_info() to an equivalent supported by both clang and GCC

[Måns Rullgård]

Ralf Baechle <ralf@linux-mips.org> writes:

> On Sat, Jan 10, 2015 at 12:53:22PM +0000, Daniel Sanders wrote:
>
>> The main reason I renamed it is that identifiers starting with '__'
>> are reserved. It's pretty unlikely but it's possible that the name
>> will conflict with a C implementation in the future.
>
> The whole kernel is using identifiers starting with a double underscore
> left and right.  The risk should be acceptable though - also because the
> kernel isn't linked against external libraries.

The reserved namespace applies to applications built against a standard
library so as to avoid name clashes with library internals.  The kernel
doesn't use the standard library, so it can use any identifiers.  This
should be clear from the fact that the reserved identifiers are listed
in the "Library" chapter of the C standard.

-- 
Måns Rullgård
mans@mansr.com

RE: [PATCH] MIPS: Changed current_thread_info() to an equivalent supported by both clang and GCC

[Daniel Sanders]

> -----Original Message-----
> From: Ralf Baechle [mailto:ralf@linux-mips.org]
> Sent: 16 January 2015 14:37
> To: Daniel Sanders
> Cc: David Daney; Sergei Shtylyov; linux-mips@linux-mips.org; Paul Burton;
> Markos Chandras; James Hogan; Behan Webster
> Subject: Re: [PATCH] MIPS: Changed current_thread_info() to an equivalent
> supported by both clang and GCC
> 
> On Sat, Jan 10, 2015 at 12:53:22PM +0000, Daniel Sanders wrote:
> 
> > The main reason I renamed it is that identifiers starting with '__' are
> reserved. It's pretty unlikely but it's possible that the name will conflict with
> a C implementation in the future.
> 
> The whole kernel is using identifiers starting with a double underscore
> left and right.  The risk should be acceptable though - also because the
> kernel isn't linked against external libraries.

Makes sense, I won't worry too much about then. Thanks for explaining.
 
> The sole reason why _current_thread_info was a local variable is so nobody
> else can use it - the proper interface to use is current_thread_info().
>
> Other than that, both the current and the proposed variant aren't really
> correct for a variable that really is per thread.  So I'm going to just
> queue this for 3.20.
> 
> Thanks!
> 
>   Ralf

Thanks. I'll finish preparing the other clang-related patches for MIPS and will submit them soon.