linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* nios2: is the ptrace ABI correct?
@ 2015-02-24  3:04 Ezequiel Garcia
  2015-02-24  8:54 ` Arnd Bergmann
  0 siblings, 1 reply; 18+ messages in thread
From: Ezequiel Garcia @ 2015-02-24  3:04 UTC (permalink / raw)
  To: Tobias Klauser, Chung-Ling Tang, Walter Goossens, Ley Foon Tan,
	Arnd Bergmann
  Cc: linux-arch, nios2-dev, linux-kernel

Hi everyone,

I've been trying to port strace to Nios-II, but came across some oddities
in the ptrace ABI. The pt_regs structure is exposed to userspace through
the ptrace.h UAPI header: arch/nios2/include/uapi/asm/ptrace.h

Such pt_regs has the following layout:

struct pt_regs {
        unsigned long  r8;
        unsigned long  r9;
        unsigned long  r10;
[and so on]
}

But the regset implementation in arch/nios2/kernel/ptrace.c
pushes a different layout to userspace, as it uses the PTR_
macros, also in the UAPI header:

#define PTR_R0          0
#define PTR_R1          1
#define PTR_R2          2
#define PTR_R3          3
#define PTR_R4          4
#define PTR_R5          5
#define PTR_R6          6
#define PTR_R7          7
#define PTR_R8          8
[..]

In other words, the PTRACE_GETREGSET will pass to userspace register
r8 at offset 8*4, but the pt_regs structure says it's at offset 0.

This difference appears because ptrace returns one layout to userspace,
and pushes the registers to the stack with another layout.

I've fixed this and managed to port strace by changing the genregs_get
implementation, so it returns a layout that matches pt_regs, and therefore
matches the stack.

Having this one-to-one correspondence, has a nice benefit. The implementation
is trivial:

static int genregs_get(struct task_struct *target,
                       const struct user_regset *regset,
                       unsigned int pos, unsigned int count,
                       void *kbuf, void __user *ubuf)
{
        const struct pt_regs *regs = task_pt_regs(target);
        const struct switch_stack *sw = (struct switch_stack *)regs - 1;
        int ret;

        ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
                                  regs, 0, PT_REGS_SIZE);
        if (!ret)
                ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
                                          sw, 0, SWITCH_STACK_SIZE);
        return ret;
}

However, there's a problem. The ABI is already different, and current gdb
seems to rely in the current layout. However, it does it by *not* using the
pt_regs structure.

Give the ABI is already used, I'm reluctant to change it as I don't want to
break our beloved users.

So, tried a different approach and removed pt_regs from the UAPI ptrace.h,
replacing it with a new user_regs that describes how registers are passed
to user. This however is also problematic, as pt_regs is already used
by glibc (not really sure what for).

In conclusion, I'm lost and have no clue how a proper fix
would look like. (Or perhaps, I'm really lost and there's no fix needed.)

Any ideas?

-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: nios2: is the ptrace ABI correct?
  2015-02-24  3:04 nios2: is the ptrace ABI correct? Ezequiel Garcia
@ 2015-02-24  8:54 ` Arnd Bergmann
  2015-02-24 15:28   ` Ezequiel Garcia
  0 siblings, 1 reply; 18+ messages in thread
From: Arnd Bergmann @ 2015-02-24  8:54 UTC (permalink / raw)
  To: Ezequiel Garcia
  Cc: Tobias Klauser, Chung-Ling Tang, Walter Goossens, Ley Foon Tan,
	linux-arch, nios2-dev, linux-kernel

On Tuesday 24 February 2015 00:04:21 Ezequiel Garcia wrote:
> So, tried a different approach and removed pt_regs from the UAPI ptrace.h,
> replacing it with a new user_regs that describes how registers are passed
> to user. This however is also problematic, as pt_regs is already used
> by glibc (not really sure what for).
> 

I've looked at glibc and could not find a use for pt_regs there. Where
did you find it? It's quite possible that it's incorrect as well
if the structures don't match.

	Arnd

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: nios2: is the ptrace ABI correct?
  2015-02-24  8:54 ` Arnd Bergmann
@ 2015-02-24 15:28   ` Ezequiel Garcia
  2015-02-24 19:25     ` Arnd Bergmann
  0 siblings, 1 reply; 18+ messages in thread
From: Ezequiel Garcia @ 2015-02-24 15:28 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Tobias Klauser, Chung-Ling Tang, Walter Goossens, Ley Foon Tan,
	linux-arch, nios2-dev, linux-kernel

Hi Arnd,

On 02/24/2015 05:54 AM, Arnd Bergmann wrote:
> On Tuesday 24 February 2015 00:04:21 Ezequiel Garcia wrote:
>> So, tried a different approach and removed pt_regs from the UAPI ptrace.h,
>> replacing it with a new user_regs that describes how registers are passed
>> to user. This however is also problematic, as pt_regs is already used
>> by glibc (not really sure what for).
>>
> 
> I've looked at glibc and could not find a use for pt_regs there. Where
> did you find it? It's quite possible that it's incorrect as well
> if the structures don't match.
> 

Gah, no, you are right. I got confused.

So it would be OK to avoid remove pt_regs from the uapi headers?
How does this affect the signal handling nios2 implementation?

-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: nios2: is the ptrace ABI correct?
  2015-02-24 15:28   ` Ezequiel Garcia
@ 2015-02-24 19:25     ` Arnd Bergmann
  2015-02-25 11:33       ` Ezequiel Garcia
  0 siblings, 1 reply; 18+ messages in thread
From: Arnd Bergmann @ 2015-02-24 19:25 UTC (permalink / raw)
  To: Ezequiel Garcia
  Cc: Tobias Klauser, Chung-Ling Tang, Walter Goossens, Ley Foon Tan,
	linux-arch, nios2-dev, linux-kernel

On Tuesday 24 February 2015 12:28:41 Ezequiel Garcia wrote:
> 
> Gah, no, you are right. I got confused.
> 
> So it would be OK to avoid remove pt_regs from the uapi headers?
> How does this affect the signal handling nios2 implementation?
> 

We have a number of architectures that don't provide this structure:

$ git grep -L pt_regs arch/*/include/uapi/asm/ptrace.h
arch/frv/include/uapi/asm/ptrace.h
arch/metag/include/uapi/asm/ptrace.h
arch/openrisc/include/uapi/asm/ptrace.h
arch/s390/include/uapi/asm/ptrace.h

so I'd assume it's ok in general not to have it. However, on
nios2, struct pt_regs is embedded inside of struct sigcontext.
If I read the code in arch/nios2/kernel/signal.c correctly,
this is actually a bug and you should use a different structure
there too, because pt_regs does not match the layout of the
stack either. This means that the (rare) user programs that
would know about the architecture to modify signal stacks
are currently broken.

	Arnd

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: nios2: is the ptrace ABI correct?
  2015-02-24 19:25     ` Arnd Bergmann
@ 2015-02-25 11:33       ` Ezequiel Garcia
  2015-02-25 14:07         ` Arnd Bergmann
  0 siblings, 1 reply; 18+ messages in thread
From: Ezequiel Garcia @ 2015-02-25 11:33 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Tobias Klauser, Chung-Ling Tang, Walter Goossens, Ley Foon Tan,
	linux-arch, nios2-dev, linux-kernel



On 02/24/2015 04:25 PM, Arnd Bergmann wrote:
> On Tuesday 24 February 2015 12:28:41 Ezequiel Garcia wrote:
>>
>> Gah, no, you are right. I got confused.
>>
>> So it would be OK to avoid remove pt_regs from the uapi headers?
>> How does this affect the signal handling nios2 implementation?
>>
> 
> We have a number of architectures that don't provide this structure:
> 
> $ git grep -L pt_regs arch/*/include/uapi/asm/ptrace.h
> arch/frv/include/uapi/asm/ptrace.h
> arch/metag/include/uapi/asm/ptrace.h
> arch/openrisc/include/uapi/asm/ptrace.h
> arch/s390/include/uapi/asm/ptrace.h
> 
> so I'd assume it's ok in general not to have it. However, on
> nios2, struct pt_regs is embedded inside of struct sigcontext.
> If I read the code in arch/nios2/kernel/signal.c correctly,
> this is actually a bug and you should use a different structure
> there too, because pt_regs does not match the layout of the
> stack either. This means that the (rare) user programs that
> would know about the architecture to modify signal stacks
> are currently broken.
> 

/me is more confused now

In arch/nios2/include/asm/ucontext.h

struct ucontext {
        unsigned long     uc_flags;
        struct ucontext  *uc_link;
        stack_t           uc_stack;
        struct mcontext   uc_mcontext;
        sigset_t          uc_sigmask;
};

And in include/uapi/asm-generic/ucontext.h:

struct ucontext {
        unsigned long     uc_flags;
        struct ucontext  *uc_link;
        stack_t           uc_stack;
        struct sigcontext uc_mcontext;
        sigset_t          uc_sigmask;
};

Which one is the one that userspace sees? And why does the kernel has
two different structures?

Given this oddities, I'm wondering how troublesome would be to just
re-do this and break the ptrace and signal ABI. For instance, just
pushing pt_regs in PTRACE_GETREGSET would make things much clearer.

I guess Linus would burn me for even suggesting to breaking users... but
do we have any users at all? This arch has just been mainlined. Altera's
out-of-tree is already ABI-incompatible with mainline so that's not an
issue.

The only one using this ABI is gdb, which is easily fixed.
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: nios2: is the ptrace ABI correct?
  2015-02-25 11:33       ` Ezequiel Garcia
@ 2015-02-25 14:07         ` Arnd Bergmann
  2015-02-27  8:57           ` Chung-Lin Tang
  0 siblings, 1 reply; 18+ messages in thread
From: Arnd Bergmann @ 2015-02-25 14:07 UTC (permalink / raw)
  To: Ezequiel Garcia
  Cc: Tobias Klauser, Chung-Ling Tang, Walter Goossens, Ley Foon Tan,
	linux-arch, nios2-dev, linux-kernel

On Wednesday 25 February 2015 08:33:16 Ezequiel Garcia wrote:
> 
> /me is more confused now
> 
> In arch/nios2/include/asm/ucontext.h
> 
> struct ucontext {
>         unsigned long     uc_flags;
>         struct ucontext  *uc_link;
>         stack_t           uc_stack;
>         struct mcontext   uc_mcontext;
>         sigset_t          uc_sigmask;
> };
> 
> And in include/uapi/asm-generic/ucontext.h:
> 
> struct ucontext {
>         unsigned long     uc_flags;
>         struct ucontext  *uc_link;
>         stack_t           uc_stack;
>         struct sigcontext uc_mcontext;
>         sigset_t          uc_sigmask;
> };
> 
> Which one is the one that userspace sees? And why does the kernel has
> two different structures?

Userspace sees the asm-generic header, which I assume is a bug
in this case.

> Given this oddities, I'm wondering how troublesome would be to just
> re-do this and break the ptrace and signal ABI. For instance, just
> pushing pt_regs in PTRACE_GETREGSET would make things much clearer.

Could you change pt_regs to match the layout you have for PTRACE_GETREGSET
instead? It seems much more intuitive.

> I guess Linus would burn me for even suggesting to breaking users... but
> do we have any users at all? This arch has just been mainlined. Altera's
> out-of-tree is already ABI-incompatible with mainline so that's not an
> issue.
> 
> The only one using this ABI is gdb, which is easily fixed.

You can change anything you like as long as nobody complains about
regressions.

	Arnd

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: nios2: is the ptrace ABI correct?
  2015-02-25 14:07         ` Arnd Bergmann
@ 2015-02-27  8:57           ` Chung-Lin Tang
  2015-02-27 11:19             ` Ezequiel Garcia
  0 siblings, 1 reply; 18+ messages in thread
From: Chung-Lin Tang @ 2015-02-27  8:57 UTC (permalink / raw)
  To: Arnd Bergmann, Ezequiel Garcia
  Cc: Tobias Klauser, Walter Goossens, Ley Foon Tan, linux-arch,
	nios2-dev, linux-kernel

On 15/2/25 10:07 PM, Arnd Bergmann wrote:
> On Wednesday 25 February 2015 08:33:16 Ezequiel Garcia wrote:
>>
>> /me is more confused now
>>
>> In arch/nios2/include/asm/ucontext.h
>>
>> struct ucontext {
>>         unsigned long     uc_flags;
>>         struct ucontext  *uc_link;
>>         stack_t           uc_stack;
>>         struct mcontext   uc_mcontext;
>>         sigset_t          uc_sigmask;
>> };
>>
>> And in include/uapi/asm-generic/ucontext.h:
>>
>> struct ucontext {
>>         unsigned long     uc_flags;
>>         struct ucontext  *uc_link;
>>         stack_t           uc_stack;
>>         struct sigcontext uc_mcontext;
>>         sigset_t          uc_sigmask;
>> };
>>
>> Which one is the one that userspace sees? And why does the kernel has
>> two different structures?
> 
> Userspace sees the asm-generic header, which I assume is a bug
> in this case.

Yes, I believe nios2 doesn't not need this asm-generic/ucontext.h
header; OTOH it just isn't used; no real harm done, so easily fixed.

>> Given this oddities, I'm wondering how troublesome would be to just
>> re-do this and break the ptrace and signal ABI. For instance, just
>> pushing pt_regs in PTRACE_GETREGSET would make things much clearer.
> 
> Could you change pt_regs to match the layout you have for PTRACE_GETREGSET
> instead? It seems much more intuitive.

There is a reason for this pt_regs arrangement: the nios2 syscall
interface uses r4-r9 for parameters, while the usual C conventions use
only r4-r7, placing r8-r9 at the start of pt_regs creates a natural
stack layout for entering C code after the asm shims in entry.S

>> I guess Linus would burn me for even suggesting to breaking users... but
>> do we have any users at all? This arch has just been mainlined. Altera's
>> out-of-tree is already ABI-incompatible with mainline so that's not an
>> issue.
>>
>> The only one using this ABI is gdb, which is easily fixed.
> 
> You can change anything you like as long as nobody complains about
> regressions.

PTRACE_GET/SETREGSET is a new feature in nios2-linux that we're still
about to support in upstream GDB, so things could be fixed if needed,
but why can't you just use the [0...] ordering in userspace?

BTW, it's even that way in signal stacks as well; nios2 does not
use/export sigcontext inside struct ucontext. We just use a int[32]
array there.

Thanks,
Chung-Lin


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: nios2: is the ptrace ABI correct?
  2015-02-27  8:57           ` Chung-Lin Tang
@ 2015-02-27 11:19             ` Ezequiel Garcia
  2015-03-04 20:56               ` Ezequiel Garcia
  2015-03-09 16:54               ` Chung-Lin Tang
  0 siblings, 2 replies; 18+ messages in thread
From: Ezequiel Garcia @ 2015-02-27 11:19 UTC (permalink / raw)
  To: Chung-Lin Tang, Arnd Bergmann
  Cc: Tobias Klauser, Walter Goossens, Ley Foon Tan, linux-arch,
	nios2-dev, linux-kernel

Hi Chung-Lin Tang,

On 02/27/2015 05:57 AM, Chung-Lin Tang wrote:
> On 15/2/25 10:07 PM, Arnd Bergmann wrote:
>> On Wednesday 25 February 2015 08:33:16 Ezequiel Garcia wrote:
>>>
>>> /me is more confused now
>>>
>>> In arch/nios2/include/asm/ucontext.h
>>>
>>> struct ucontext {
>>>         unsigned long     uc_flags;
>>>         struct ucontext  *uc_link;
>>>         stack_t           uc_stack;
>>>         struct mcontext   uc_mcontext;
>>>         sigset_t          uc_sigmask;
>>> };
>>>
>>> And in include/uapi/asm-generic/ucontext.h:
>>>
>>> struct ucontext {
>>>         unsigned long     uc_flags;
>>>         struct ucontext  *uc_link;
>>>         stack_t           uc_stack;
>>>         struct sigcontext uc_mcontext;
>>>         sigset_t          uc_sigmask;
>>> };
>>>
>>> Which one is the one that userspace sees? And why does the kernel has
>>> two different structures?
>>
>> Userspace sees the asm-generic header, which I assume is a bug
>> in this case.
> 
> Yes, I believe nios2 doesn't not need this asm-generic/ucontext.h
> header; OTOH it just isn't used; no real harm done, so easily fixed.
> 
>>> Given this oddities, I'm wondering how troublesome would be to just
>>> re-do this and break the ptrace and signal ABI. For instance, just
>>> pushing pt_regs in PTRACE_GETREGSET would make things much clearer.
>>
>> Could you change pt_regs to match the layout you have for PTRACE_GETREGSET
>> instead? It seems much more intuitive.
> 
> There is a reason for this pt_regs arrangement: the nios2 syscall
> interface uses r4-r9 for parameters, while the usual C conventions use
> only r4-r7, placing r8-r9 at the start of pt_regs creates a natural
> stack layout for entering C code after the asm shims in entry.S
> 
>>> I guess Linus would burn me for even suggesting to breaking users... but
>>> do we have any users at all? This arch has just been mainlined. Altera's
>>> out-of-tree is already ABI-incompatible with mainline so that's not an
>>> issue.
>>>
>>> The only one using this ABI is gdb, which is easily fixed.
>>
>> You can change anything you like as long as nobody complains about
>> regressions.
> 
> PTRACE_GET/SETREGSET is a new feature in nios2-linux that we're still
> about to support in upstream GDB, so things could be fixed if needed,
> but why can't you just use the [0...] ordering in userspace?
> 

Sure, that's doable. However, I believe the problem is the pt_regs
struct is exported in ptrace.h so we seem to be telling userspace to use it.

> BTW, it's even that way in signal stacks as well; nios2 does not
> use/export sigcontext inside struct ucontext. We just use a int[32]
> array there.
> 

I think we are more or less on the same page.

Right now, my biggest problem is how to remove the pt_regs from being
exported to userspace, without breaking things. The struct is defined
and used in a few UAPI headers:

arch/nios2/include/uapi/asm/ptrace.h
arch/nios2/include/uapi/asm/sigcontext.h
arch/nios2/include/uapi/asm/elf.h

For ucontext, we would need to remove
arch/nios2/include/uapi/asm/sigcontext.h and avoid using
asm-generic/ucontext.h.

For ptrace, we could move pt_regs from the UAPI header to some internal
header. That way userspace is not exposed the wrong struct.

For the elf coredump, I have no idea yet :)
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: nios2: is the ptrace ABI correct?
  2015-02-27 11:19             ` Ezequiel Garcia
@ 2015-03-04 20:56               ` Ezequiel Garcia
  2015-03-09 16:54               ` Chung-Lin Tang
  1 sibling, 0 replies; 18+ messages in thread
From: Ezequiel Garcia @ 2015-03-04 20:56 UTC (permalink / raw)
  To: Chung-Lin Tang, Arnd Bergmann, Ley Foon Tan
  Cc: Tobias Klauser, Walter Goossens, linux-arch, nios2-dev, linux-kernel

Hi all,

I have here a patch to rework most of the UAPI headers, so the info
exported to userspace is sane. The ABI itself is left untouched.

This will allow to support strace (which is ready since a few weeks now)
and will sanitize the UAPIs. The ABI itself is ugly as hell, but I don't
want to break it.

Given the lack of replies on this thread, I guess nobody cares much
about nios2 mainline. Anyway... I'll try to submit a patchset soon and
I'll appreciate if the libc can take a look and make sure everything is OK.
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: nios2: is the ptrace ABI correct?
  2015-02-27 11:19             ` Ezequiel Garcia
  2015-03-04 20:56               ` Ezequiel Garcia
@ 2015-03-09 16:54               ` Chung-Lin Tang
  2015-03-09 17:02                 ` Chung-Lin Tang
  1 sibling, 1 reply; 18+ messages in thread
From: Chung-Lin Tang @ 2015-03-09 16:54 UTC (permalink / raw)
  To: Ezequiel Garcia, Arnd Bergmann, Tobias Klauser, Ley Foon Tan
  Cc: Walter Goossens, linux-arch, nios2-dev, linux-kernel

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

On 2015/2/27 07:19 PM, Ezequiel Garcia wrote:
> Right now, my biggest problem is how to remove the pt_regs from being
> exported to userspace, without breaking things. The struct is defined
> and used in a few UAPI headers:
> 
> arch/nios2/include/uapi/asm/ptrace.h
> arch/nios2/include/uapi/asm/sigcontext.h
> arch/nios2/include/uapi/asm/elf.h
> 
> For ucontext, we would need to remove
> arch/nios2/include/uapi/asm/sigcontext.h and avoid using
> asm-generic/ucontext.h.
> 
> For ptrace, we could move pt_regs from the UAPI header to some internal
> header. That way userspace is not exposed the wrong struct.
> 
> For the elf coredump, I have no idea yet :)

It appears that some of the ways nios2 has organized the
ucontext/pt_regs/etc. are remnants of the pre-generic code, some
basically because the port was based off m68k.

I've re-organized the headers a bit: nios2/include/asm/ucontext.h is
deleted, and re-definition of struct sigcontext now allows use of
uapi/asm-generic/ucontext.h directly.  Note that the reorg, despite
effectively renaming some fields, is still binary compatible. I'll
probably update the corresponding glibc definitions later.

struct pt_regs is now not exported, and all exported register sets are
now supposed to follow the 49 register set defined as in GDB now.

Tobias, Ley Foon, how do you think this looks?

Thanks,
Chung-Lin


[-- Attachment #2: x.diff --]
[-- Type: text/plain, Size: 6248 bytes --]

Index: gcc/config/nios2/nios2-protos.h
===================================================================
--- gcc/config/nios2/nios2-protos.h	(revision 446123)
+++ gcc/config/nios2/nios2-protos.h	(working copy)
@@ -32,7 +32,7 @@ extern void nios2_function_profiler (FILE *, int);
 #ifdef RTX_CODE
 extern bool nios2_emit_move_sequence (rtx *, enum machine_mode);
 extern void nios2_emit_expensive_div (rtx *, enum machine_mode);
-extern void nios2_adjust_call_address (rtx *);
+extern void nios2_adjust_call_address (rtx *, rtx);
 
 extern rtx nios2_get_return_address (int);
 extern void nios2_set_return_address (rtx, rtx);
Index: gcc/config/nios2/nios2.c
===================================================================
--- gcc/config/nios2/nios2.c	(revision 446123)
+++ gcc/config/nios2/nios2.c	(working copy)
@@ -1511,12 +1511,12 @@ nios2_unspec_offset (rtx loc, int unspec)
 
 /* Generate GOT pointer based address with large offset.  */
 static rtx
-nios2_large_got_address (rtx offset)
+nios2_large_got_address (rtx offset, rtx tmp)
 {
-  rtx addr = gen_reg_rtx (Pmode);
-  emit_insn (gen_add3_insn (addr, pic_offset_table_rtx,
-			    force_reg (Pmode, offset)));
-  return addr;
+  if (!tmp)
+    tmp = gen_reg_rtx (Pmode);
+  emit_move_insn (tmp, offset);
+  return gen_rtx_PLUS (Pmode, tmp, pic_offset_table_rtx);
 }
 
 /* Generate a GOT pointer based address.  */
@@ -1527,7 +1527,7 @@ nios2_got_address (rtx loc, int unspec)
   crtl->uses_pic_offset_table = 1;
 
   if (nios2_large_offset_p (unspec))
-    return nios2_large_got_address (offset);
+    return force_reg (Pmode, nios2_large_got_address (offset, NULL_RTX));
 
   return gen_rtx_PLUS (Pmode, pic_offset_table_rtx, offset);
 }
@@ -2080,13 +2080,17 @@ nios2_load_pic_register (void)
 
 /* Generate a PIC address as a MEM rtx.  */
 static rtx
-nios2_load_pic_address (rtx sym, int unspec)
+nios2_load_pic_address (rtx sym, int unspec, rtx tmp)
 {
   if (flag_pic == 2
       && GET_CODE (sym) == SYMBOL_REF
       && nios2_symbol_binds_local_p (sym))
     /* Under -fPIC, generate a GOTOFF address for local symbols.  */
-    return nios2_got_address (sym, UNSPEC_PIC_GOTOFF_SYM);
+    {
+      rtx offset = nios2_unspec_offset (sym, UNSPEC_PIC_GOTOFF_SYM);
+      crtl->uses_pic_offset_table = 1;
+      return nios2_large_got_address (offset, tmp);
+    }
 
   return gen_const_mem (Pmode, nios2_got_address (sym, unspec));
 }
@@ -2122,7 +2126,7 @@ nios2_legitimize_constant_address (rtx addr)
   if (nios2_tls_symbol_p (base))
     base = nios2_legitimize_tls_address (base);
   else if (flag_pic)
-    base = nios2_load_pic_address (base, UNSPEC_PIC_SYM);
+    base = nios2_load_pic_address (base, UNSPEC_PIC_SYM, NULL_RTX);
   else
     return addr;
 
@@ -2266,18 +2270,24 @@ nios2_emit_move_sequence (rtx *operands, enum mach
 
 /* The function with address *ADDR is being called.  If the address
    needs to be loaded from the GOT, emit the instruction to do so and
-   update *ADDR to point to the rtx for the loaded value.  */
+   update *ADDR to point to the rtx for the loaded value.
+   If REG != NULL_RTX, it is used as the target/scratch register in the
+   GOT address calculation.  */
 void
-nios2_adjust_call_address (rtx *call_op)
+nios2_adjust_call_address (rtx *call_op, rtx reg)
 {
-  rtx addr;
-  gcc_assert (MEM_P (*call_op));
-  addr = XEXP (*call_op, 0);
+  if (MEM_P (*call_op))
+    call_op = &XEXP (*call_op, 0);
+
+  rtx addr = *call_op;
   if (flag_pic && CONSTANT_P (addr))
     {
-      rtx reg = gen_reg_rtx (Pmode);
-      emit_move_insn (reg, nios2_load_pic_address (addr, UNSPEC_PIC_CALL_SYM));
-      XEXP (*call_op, 0) = reg;
+      rtx tmp = reg ? reg : NULL_RTX;
+      if (!reg)
+	reg = gen_reg_rtx (Pmode);
+      addr = nios2_load_pic_address (addr, UNSPEC_PIC_CALL_SYM, tmp);
+      emit_insn (gen_rtx_SET (VOIDmode, reg, addr));
+      *call_op = reg;
     }
 }
 
@@ -4980,8 +4990,10 @@ nios2_asm_output_mi_thunk (FILE *file, tree thunk_
       TREE_USED (function) = 1;
     }
   funexp = XEXP (DECL_RTL (function), 0);
-  funexp = gen_rtx_MEM (FUNCTION_MODE, funexp);
-  insn = emit_call_insn (gen_sibcall (funexp, const0_rtx));
+  /* Function address needs to be constructed under PIC,
+     provide r2 to use here.  */
+  nios2_adjust_call_address (&funexp, gen_rtx_REG (Pmode, 2));
+  insn = emit_call_insn (gen_sibcall_internal (funexp, const0_rtx));
   SIBLING_CALL_P (insn) = 1;
 
   /* Run just enough of rest_of_compilation to get the insns emitted.
Index: gcc/config/nios2/nios2.md
===================================================================
--- gcc/config/nios2/nios2.md	(revision 446123)
+++ gcc/config/nios2/nios2.md	(working copy)
@@ -818,7 +818,7 @@
                     (match_operand 1 "" ""))
               (clobber (reg:SI RA_REGNO))])]
   ""
-  "nios2_adjust_call_address (&operands[0]);")
+  "nios2_adjust_call_address (&operands[0], NULL_RTX);")
 
 (define_expand "call_value"
   [(parallel [(set (match_operand 0 "" "")
@@ -826,7 +826,7 @@
                          (match_operand 2 "" "")))
               (clobber (reg:SI RA_REGNO))])]
   ""
-  "nios2_adjust_call_address (&operands[1]);")
+  "nios2_adjust_call_address (&operands[1], NULL_RTX);")
 
 (define_insn "*call"
   [(call (mem:QI (match_operand:SI 0 "call_operand" "i,r"))
@@ -854,7 +854,7 @@
                     (match_operand 1 "" ""))
               (return)])]
   ""
-  "nios2_adjust_call_address (&operands[0]);")
+  "nios2_adjust_call_address (&operands[0], NULL_RTX);")
 
 (define_expand "sibcall_value"
   [(parallel [(set (match_operand 0 "" "")
@@ -862,9 +862,9 @@
                          (match_operand 2 "" "")))
               (return)])]
   ""
-  "nios2_adjust_call_address (&operands[1]);")
+  "nios2_adjust_call_address (&operands[1], NULL_RTX);")
 
-(define_insn "*sibcall"
+(define_insn "sibcall_internal"
  [(call (mem:QI (match_operand:SI 0 "call_operand" "i,j"))
         (match_operand 1 "" ""))
   (return)]
@@ -874,7 +874,7 @@
    jmp.r\\t%0"
   [(set_attr "type" "control")])
 
-(define_insn "*sibcall_value"
+(define_insn "sibcall_value_internal"
  [(set (match_operand 0 "register_operand" "")
        (call (mem:QI (match_operand:SI 1 "call_operand" "i,j"))
              (match_operand 2 "" "")))

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: nios2: is the ptrace ABI correct?
  2015-03-09 16:54               ` Chung-Lin Tang
@ 2015-03-09 17:02                 ` Chung-Lin Tang
  2015-03-09 17:05                   ` Ezequiel Garcia
  2015-03-12 11:07                   ` Tobias Klauser
  0 siblings, 2 replies; 18+ messages in thread
From: Chung-Lin Tang @ 2015-03-09 17:02 UTC (permalink / raw)
  To: Chung-Lin Tang, Ezequiel Garcia, Arnd Bergmann, Tobias Klauser,
	Ley Foon Tan
  Cc: Walter Goossens, linux-arch, nios2-dev, linux-kernel

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

On 2015/3/10 12:54 AM, Chung-Lin Tang wrote:
> It appears that some of the ways nios2 has organized the
> ucontext/pt_regs/etc. are remnants of the pre-generic code, some
> basically because the port was based off m68k.
> 
> I've re-organized the headers a bit: nios2/include/asm/ucontext.h is
> deleted, and re-definition of struct sigcontext now allows use of
> uapi/asm-generic/ucontext.h directly.  Note that the reorg, despite
> effectively renaming some fields, is still binary compatible. I'll
> probably update the corresponding glibc definitions later.
> 
> struct pt_regs is now not exported, and all exported register sets are
> now supposed to follow the 49 register set defined as in GDB now.
> 
> Tobias, Ley Foon, how do you think this looks?

Sorry, accidentally attached unrelated GCC patch instead, this one's the
correct one.

Chung-Lin


[-- Attachment #2: x.diff --]
[-- Type: text/plain, Size: 6719 bytes --]

diff --git a/arch/nios2/include/asm/ptrace.h b/arch/nios2/include/asm/ptrace.h
index 20fb1cf..6424621 100644
--- a/arch/nios2/include/asm/ptrace.h
+++ b/arch/nios2/include/asm/ptrace.h
@@ -15,7 +15,54 @@
 
 #include <uapi/asm/ptrace.h>
 
+/* This struct defines the way the registers are stored on the
+   stack during a system call.  */
+
 #ifndef __ASSEMBLY__
+struct pt_regs {
+	unsigned long  r8;	/* r8-r15 Caller-saved GP registers */
+	unsigned long  r9;
+	unsigned long  r10;
+	unsigned long  r11;
+	unsigned long  r12;
+	unsigned long  r13;
+	unsigned long  r14;
+	unsigned long  r15;
+	unsigned long  r1;	/* Assembler temporary */
+	unsigned long  r2;	/* Retval LS 32bits */
+	unsigned long  r3;	/* Retval MS 32bits */
+	unsigned long  r4;	/* r4-r7 Register arguments */
+	unsigned long  r5;
+	unsigned long  r6;
+	unsigned long  r7;
+	unsigned long  orig_r2;	/* Copy of r2 ?? */
+	unsigned long  ra;	/* Return address */
+	unsigned long  fp;	/* Frame pointer */
+	unsigned long  sp;	/* Stack pointer */
+	unsigned long  gp;	/* Global pointer */
+	unsigned long  estatus;
+	unsigned long  ea;	/* Exception return address (pc) */
+	unsigned long  orig_r7;
+};
+
+/*
+ * This is the extended stack used by signal handlers and the context
+ * switcher: it's pushed after the normal "struct pt_regs".
+ */
+struct switch_stack {
+	unsigned long  r16;	/* r16-r23 Callee-saved GP registers */
+	unsigned long  r17;
+	unsigned long  r18;
+	unsigned long  r19;
+	unsigned long  r20;
+	unsigned long  r21;
+	unsigned long  r22;
+	unsigned long  r23;
+	unsigned long  fp;
+	unsigned long  gp;
+	unsigned long  ra;
+};
+
 #define user_mode(regs)	(((regs)->estatus & ESTATUS_EU))
 
 #define instruction_pointer(regs)	((regs)->ra)
diff --git a/arch/nios2/include/asm/ucontext.h b/arch/nios2/include/asm/ucontext.h
deleted file mode 100644
index 2c87614..0000000
--- a/arch/nios2/include/asm/ucontext.h
+++ /dev/null
@@ -1,32 +0,0 @@
-/*
- * Copyright (C) 2010 Tobias Klauser <tklauser@distanz.ch>
- * Copyright (C) 2004 Microtronix Datacom Ltd
- *
- * This file is subject to the terms and conditions of the GNU General Public
- * License.  See the file "COPYING" in the main directory of this archive
- * for more details.
- */
-
-#ifndef _ASM_NIOS2_UCONTEXT_H
-#define _ASM_NIOS2_UCONTEXT_H
-
-typedef int greg_t;
-#define NGREG 32
-typedef greg_t gregset_t[NGREG];
-
-struct mcontext {
-	int version;
-	gregset_t gregs;
-};
-
-#define MCONTEXT_VERSION 2
-
-struct ucontext {
-	unsigned long	  uc_flags;
-	struct ucontext  *uc_link;
-	stack_t		  uc_stack;
-	struct mcontext	  uc_mcontext;
-	sigset_t	  uc_sigmask;	/* mask last for extensibility */
-};
-
-#endif
diff --git a/arch/nios2/include/uapi/asm/Kbuild b/arch/nios2/include/uapi/asm/Kbuild
index 4f07ca3f8..37613119 100644
--- a/arch/nios2/include/uapi/asm/Kbuild
+++ b/arch/nios2/include/uapi/asm/Kbuild
@@ -2,3 +2,5 @@ include include/uapi/asm-generic/Kbuild.asm
 
 header-y += elf.h
 header-y += ucontext.h
+
+generic-y += ucontext.h
diff --git a/arch/nios2/include/uapi/asm/elf.h b/arch/nios2/include/uapi/asm/elf.h
index a5b91ae..6f06d3b 100644
--- a/arch/nios2/include/uapi/asm/elf.h
+++ b/arch/nios2/include/uapi/asm/elf.h
@@ -50,9 +50,7 @@
 
 typedef unsigned long elf_greg_t;
 
-#define ELF_NGREG	\
-	((sizeof(struct pt_regs) + sizeof(struct switch_stack)) /	\
-		sizeof(elf_greg_t))
+#define ELF_NGREG		49
 typedef elf_greg_t elf_gregset_t[ELF_NGREG];
 
 typedef unsigned long elf_fpregset_t;
diff --git a/arch/nios2/include/uapi/asm/ptrace.h b/arch/nios2/include/uapi/asm/ptrace.h
index e83a7c9..71a3305 100644
--- a/arch/nios2/include/uapi/asm/ptrace.h
+++ b/arch/nios2/include/uapi/asm/ptrace.h
@@ -67,53 +67,9 @@
 
 #define NUM_PTRACE_REG (PTR_TLBMISC + 1)
 
-/* this struct defines the way the registers are stored on the
-   stack during a system call.
-
-   There is a fake_regs in setup.c that has to match pt_regs.*/
-
-struct pt_regs {
-	unsigned long  r8;		/* r8-r15 Caller-saved GP registers */
-	unsigned long  r9;
-	unsigned long  r10;
-	unsigned long  r11;
-	unsigned long  r12;
-	unsigned long  r13;
-	unsigned long  r14;
-	unsigned long  r15;
-	unsigned long  r1;		/* Assembler temporary */
-	unsigned long  r2;		/* Retval LS 32bits */
-	unsigned long  r3;		/* Retval MS 32bits */
-	unsigned long  r4;		/* r4-r7 Register arguments */
-	unsigned long  r5;
-	unsigned long  r6;
-	unsigned long  r7;
-	unsigned long  orig_r2;		/* Copy of r2 ?? */
-	unsigned long  ra;		/* Return address */
-	unsigned long  fp;		/* Frame pointer */
-	unsigned long  sp;		/* Stack pointer */
-	unsigned long  gp;		/* Global pointer */
-	unsigned long  estatus;
-	unsigned long  ea;		/* Exception return address (pc) */
-	unsigned long  orig_r7;
-};
-
-/*
- * This is the extended stack used by signal handlers and the context
- * switcher: it's pushed after the normal "struct pt_regs".
- */
-struct switch_stack {
-	unsigned long  r16;		/* r16-r23 Callee-saved GP registers */
-	unsigned long  r17;
-	unsigned long  r18;
-	unsigned long  r19;
-	unsigned long  r20;
-	unsigned long  r21;
-	unsigned long  r22;
-	unsigned long  r23;
-	unsigned long  fp;
-	unsigned long  gp;
-	unsigned long  ra;
+/* User structures for general purpose registers.  */
+struct user_pt_regs {
+	__u32		regs[49];
 };
 
 #endif /* __ASSEMBLY__ */
diff --git a/arch/nios2/include/uapi/asm/sigcontext.h b/arch/nios2/include/uapi/asm/sigcontext.h
index 7b8bb41..b67944a 100644
--- a/arch/nios2/include/uapi/asm/sigcontext.h
+++ b/arch/nios2/include/uapi/asm/sigcontext.h
@@ -15,14 +15,16 @@
  * details.
  */
 
-#ifndef _ASM_NIOS2_SIGCONTEXT_H
-#define _ASM_NIOS2_SIGCONTEXT_H
+#ifndef _UAPI__ASM_SIGCONTEXT_H
+#define _UAPI__ASM_SIGCONTEXT_H
 
-#include <asm/ptrace.h>
+#include <linux/types.h>
+
+#define MCONTEXT_VERSION 2
 
 struct sigcontext {
-	struct pt_regs regs;
-	unsigned long  sc_mask;	/* old sigmask */
+	int version;
+	unsigned long gregs[32];
 };
 
 #endif
diff --git a/arch/nios2/kernel/signal.c b/arch/nios2/kernel/signal.c
index 2d0ea25..dda41e4 100644
--- a/arch/nios2/kernel/signal.c
+++ b/arch/nios2/kernel/signal.c
@@ -39,7 +39,7 @@ static inline int rt_restore_ucontext(struct pt_regs *regs,
 					struct ucontext *uc, int *pr2)
 {
 	int temp;
-	greg_t *gregs = uc->uc_mcontext.gregs;
+	unsigned long *gregs = uc->uc_mcontext.gregs;
 	int err;
 
 	/* Always make any pending restarted system calls return -EINTR */
@@ -127,7 +127,7 @@ badframe:
 static inline int rt_setup_ucontext(struct ucontext *uc, struct pt_regs *regs)
 {
 	struct switch_stack *sw = (struct switch_stack *)regs - 1;
-	greg_t *gregs = uc->uc_mcontext.gregs;
+	unsigned long *gregs = uc->uc_mcontext.gregs;
 	int err = 0;
 
 	err |= __put_user(MCONTEXT_VERSION, &uc->uc_mcontext.version);

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: nios2: is the ptrace ABI correct?
  2015-03-09 17:02                 ` Chung-Lin Tang
@ 2015-03-09 17:05                   ` Ezequiel Garcia
  2015-03-10  2:54                     ` Ley Foon Tan
  2015-03-12 11:07                   ` Tobias Klauser
  1 sibling, 1 reply; 18+ messages in thread
From: Ezequiel Garcia @ 2015-03-09 17:05 UTC (permalink / raw)
  To: Chung-Lin Tang, Chung-Lin Tang, Arnd Bergmann, Tobias Klauser,
	Ley Foon Tan
  Cc: Walter Goossens, linux-arch, nios2-dev, linux-kernel



On 03/09/2015 02:02 PM, Chung-Lin Tang wrote:
> On 2015/3/10 12:54 AM, Chung-Lin Tang wrote:
>> It appears that some of the ways nios2 has organized the
>> ucontext/pt_regs/etc. are remnants of the pre-generic code, some
>> basically because the port was based off m68k.
>>
>> I've re-organized the headers a bit: nios2/include/asm/ucontext.h is
>> deleted, and re-definition of struct sigcontext now allows use of
>> uapi/asm-generic/ucontext.h directly.  Note that the reorg, despite
>> effectively renaming some fields, is still binary compatible. I'll
>> probably update the corresponding glibc definitions later.
>>
>> struct pt_regs is now not exported, and all exported register sets are
>> now supposed to follow the 49 register set defined as in GDB now.
>>
>> Tobias, Ley Foon, how do you think this looks?
> 
> Sorry, accidentally attached unrelated GCC patch instead, this one's the
> correct one.
> 

Looks good. I'm wondering if...

+/* User structures for general purpose registers.  */
+struct user_pt_regs {
+	__u32		regs[49];
 };

Can we expose the registers explicitly here? Like this:

struct user_pt_regs {
	__u32 r0;
	__u32 r1;
	...
	__u32 sp;
	__u32 gp;
	__u32 estatus;
};

It looks self-documenting and thus easier to use.
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: nios2: is the ptrace ABI correct?
  2015-03-09 17:05                   ` Ezequiel Garcia
@ 2015-03-10  2:54                     ` Ley Foon Tan
  2015-03-10  6:17                       ` Chung-Lin Tang
  0 siblings, 1 reply; 18+ messages in thread
From: Ley Foon Tan @ 2015-03-10  2:54 UTC (permalink / raw)
  To: Ezequiel Garcia
  Cc: Chung-Lin Tang, Chung-Lin Tang, Arnd Bergmann, Tobias Klauser,
	Walter Goossens, Linux-Arch, nios2-dev, linux-kernel

On Tue, Mar 10, 2015 at 1:05 AM, Ezequiel Garcia
<ezequiel@vanguardiasur.com.ar> wrote:
>
>
> On 03/09/2015 02:02 PM, Chung-Lin Tang wrote:
>> On 2015/3/10 12:54 AM, Chung-Lin Tang wrote:
>>> It appears that some of the ways nios2 has organized the
>>> ucontext/pt_regs/etc. are remnants of the pre-generic code, some
>>> basically because the port was based off m68k.
>>>
>>> I've re-organized the headers a bit: nios2/include/asm/ucontext.h is
>>> deleted, and re-definition of struct sigcontext now allows use of
>>> uapi/asm-generic/ucontext.h directly.  Note that the reorg, despite
>>> effectively renaming some fields, is still binary compatible. I'll
>>> probably update the corresponding glibc definitions later.
>>>
>>> struct pt_regs is now not exported, and all exported register sets are
>>> now supposed to follow the 49 register set defined as in GDB now.
>>>
>>> Tobias, Ley Foon, how do you think this looks?
>>
>> Sorry, accidentally attached unrelated GCC patch instead, this one's the
>> correct one.
>>
>
> Looks good. I'm wondering if...
>
> +/* User structures for general purpose registers.  */
> +struct user_pt_regs {
> +       __u32           regs[49];
>  };
>
> Can we expose the registers explicitly here? Like this:
>
> struct user_pt_regs {
>         __u32 r0;
>         __u32 r1;
>         ...
>         __u32 sp;
>         __u32 gp;
>         __u32 estatus;
> };
>
> It looks self-documenting and thus easier to use.

Hi Chung-Lin,

Your patch look good to me.
Do you have any problem to change the struct user_pt_regs based on
Ezequiel's suggestion?
If not, can you please resend the new patch.
Thanks.


Regards
Ley Foon

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: nios2: is the ptrace ABI correct?
  2015-03-10  2:54                     ` Ley Foon Tan
@ 2015-03-10  6:17                       ` Chung-Lin Tang
  2015-03-11  7:48                         ` Ley Foon Tan
  0 siblings, 1 reply; 18+ messages in thread
From: Chung-Lin Tang @ 2015-03-10  6:17 UTC (permalink / raw)
  To: Ley Foon Tan, Ezequiel Garcia
  Cc: Chung-Lin Tang, Arnd Bergmann, Tobias Klauser, Walter Goossens,
	Linux-Arch, nios2-dev, linux-kernel

On 2015/3/10 10:54 AM, Ley Foon Tan wrote:
> On Tue, Mar 10, 2015 at 1:05 AM, Ezequiel Garcia
> <ezequiel@vanguardiasur.com.ar> wrote:
>>
>>
>> On 03/09/2015 02:02 PM, Chung-Lin Tang wrote:
>>> On 2015/3/10 12:54 AM, Chung-Lin Tang wrote:
>>>> It appears that some of the ways nios2 has organized the
>>>> ucontext/pt_regs/etc. are remnants of the pre-generic code, some
>>>> basically because the port was based off m68k.
>>>>
>>>> I've re-organized the headers a bit: nios2/include/asm/ucontext.h is
>>>> deleted, and re-definition of struct sigcontext now allows use of
>>>> uapi/asm-generic/ucontext.h directly.  Note that the reorg, despite
>>>> effectively renaming some fields, is still binary compatible. I'll
>>>> probably update the corresponding glibc definitions later.
>>>>
>>>> struct pt_regs is now not exported, and all exported register sets are
>>>> now supposed to follow the 49 register set defined as in GDB now.
>>>>
>>>> Tobias, Ley Foon, how do you think this looks?
>>>
>>> Sorry, accidentally attached unrelated GCC patch instead, this one's the
>>> correct one.
>>>
>>
>> Looks good. I'm wondering if...
>>
>> +/* User structures for general purpose registers.  */
>> +struct user_pt_regs {
>> +       __u32           regs[49];
>>  };
>>
>> Can we expose the registers explicitly here? Like this:
>>
>> struct user_pt_regs {
>>         __u32 r0;
>>         __u32 r1;
>>         ...
>>         __u32 sp;
>>         __u32 gp;
>>         __u32 estatus;
>> };
>>
>> It looks self-documenting and thus easier to use.
> 
> Hi Chung-Lin,
> 
> Your patch look good to me.
> Do you have any problem to change the struct user_pt_regs based on
> Ezequiel's suggestion?

Well, exposing the register names like that sort of defeats the purpose of
the PTR_* defines.

Judging from the overall trend of style in arch/*/include/uapi/asm/ptrace.h
across ports, I would prefer to stay with the array field.

Thanks,
Chung-Lin


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: nios2: is the ptrace ABI correct?
  2015-03-10  6:17                       ` Chung-Lin Tang
@ 2015-03-11  7:48                         ` Ley Foon Tan
  2015-03-11 14:31                           ` Ezequiel Garcia
  0 siblings, 1 reply; 18+ messages in thread
From: Ley Foon Tan @ 2015-03-11  7:48 UTC (permalink / raw)
  To: Chung-Lin Tang
  Cc: Ezequiel Garcia, Chung-Lin Tang, Arnd Bergmann, Tobias Klauser,
	Walter Goossens, Linux-Arch, nios2-dev, linux-kernel

On Tue, Mar 10, 2015 at 2:17 PM, Chung-Lin Tang <cltang@codesourcery.com> wrote:
> On 2015/3/10 10:54 AM, Ley Foon Tan wrote:
>> On Tue, Mar 10, 2015 at 1:05 AM, Ezequiel Garcia
>> <ezequiel@vanguardiasur.com.ar> wrote:
>>>
>>>
>>> On 03/09/2015 02:02 PM, Chung-Lin Tang wrote:
>>>> On 2015/3/10 12:54 AM, Chung-Lin Tang wrote:
>>>>> It appears that some of the ways nios2 has organized the
>>>>> ucontext/pt_regs/etc. are remnants of the pre-generic code, some
>>>>> basically because the port was based off m68k.
>>>>>
>>>>> I've re-organized the headers a bit: nios2/include/asm/ucontext.h is
>>>>> deleted, and re-definition of struct sigcontext now allows use of
>>>>> uapi/asm-generic/ucontext.h directly.  Note that the reorg, despite
>>>>> effectively renaming some fields, is still binary compatible. I'll
>>>>> probably update the corresponding glibc definitions later.
>>>>>
>>>>> struct pt_regs is now not exported, and all exported register sets are
>>>>> now supposed to follow the 49 register set defined as in GDB now.
>>>>>
>>>>> Tobias, Ley Foon, how do you think this looks?
>>>>
>>>> Sorry, accidentally attached unrelated GCC patch instead, this one's the
>>>> correct one.
>>>>
>>>
>>> Looks good. I'm wondering if...
>>>
>>> +/* User structures for general purpose registers.  */
>>> +struct user_pt_regs {
>>> +       __u32           regs[49];
>>>  };
>>>
>>> Can we expose the registers explicitly here? Like this:
>>>
>>> struct user_pt_regs {
>>>         __u32 r0;
>>>         __u32 r1;
>>>         ...
>>>         __u32 sp;
>>>         __u32 gp;
>>>         __u32 estatus;
>>> };
>>>
>>> It looks self-documenting and thus easier to use.
>>
>> Hi Chung-Lin,
>>
>> Your patch look good to me.
>> Do you have any problem to change the struct user_pt_regs based on
>> Ezequiel's suggestion?
>
> Well, exposing the register names like that sort of defeats the purpose of
> the PTR_* defines.
>
> Judging from the overall trend of style in arch/*/include/uapi/asm/ptrace.h
> across ports, I would prefer to stay with the array field.
>
Okay, I will include your patch.

Regards
Ley Foon

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: nios2: is the ptrace ABI correct?
  2015-03-11  7:48                         ` Ley Foon Tan
@ 2015-03-11 14:31                           ` Ezequiel Garcia
  0 siblings, 0 replies; 18+ messages in thread
From: Ezequiel Garcia @ 2015-03-11 14:31 UTC (permalink / raw)
  To: Ley Foon Tan, Chung-Lin Tang
  Cc: Chung-Lin Tang, Arnd Bergmann, Tobias Klauser, Walter Goossens,
	Linux-Arch, nios2-dev, linux-kernel



On 03/11/2015 04:48 AM, Ley Foon Tan wrote:
> On Tue, Mar 10, 2015 at 2:17 PM, Chung-Lin Tang <cltang@codesourcery.com> wrote:
>> On 2015/3/10 10:54 AM, Ley Foon Tan wrote:
>>> On Tue, Mar 10, 2015 at 1:05 AM, Ezequiel Garcia
>>> <ezequiel@vanguardiasur.com.ar> wrote:
>>>>
>>>>
>>>> On 03/09/2015 02:02 PM, Chung-Lin Tang wrote:
>>>>> On 2015/3/10 12:54 AM, Chung-Lin Tang wrote:
>>>>>> It appears that some of the ways nios2 has organized the
>>>>>> ucontext/pt_regs/etc. are remnants of the pre-generic code, some
>>>>>> basically because the port was based off m68k.
>>>>>>
>>>>>> I've re-organized the headers a bit: nios2/include/asm/ucontext.h is
>>>>>> deleted, and re-definition of struct sigcontext now allows use of
>>>>>> uapi/asm-generic/ucontext.h directly.  Note that the reorg, despite
>>>>>> effectively renaming some fields, is still binary compatible. I'll
>>>>>> probably update the corresponding glibc definitions later.
>>>>>>
>>>>>> struct pt_regs is now not exported, and all exported register sets are
>>>>>> now supposed to follow the 49 register set defined as in GDB now.
>>>>>>
>>>>>> Tobias, Ley Foon, how do you think this looks?
>>>>>
>>>>> Sorry, accidentally attached unrelated GCC patch instead, this one's the
>>>>> correct one.
>>>>>
>>>>
>>>> Looks good. I'm wondering if...
>>>>
>>>> +/* User structures for general purpose registers.  */
>>>> +struct user_pt_regs {
>>>> +       __u32           regs[49];
>>>>  };
>>>>
>>>> Can we expose the registers explicitly here? Like this:
>>>>
>>>> struct user_pt_regs {
>>>>         __u32 r0;
>>>>         __u32 r1;
>>>>         ...
>>>>         __u32 sp;
>>>>         __u32 gp;
>>>>         __u32 estatus;
>>>> };
>>>>
>>>> It looks self-documenting and thus easier to use.
>>>
>>> Hi Chung-Lin,
>>>
>>> Your patch look good to me.
>>> Do you have any problem to change the struct user_pt_regs based on
>>> Ezequiel's suggestion?
>>
>> Well, exposing the register names like that sort of defeats the purpose of
>> the PTR_* defines.
>>
>> Judging from the overall trend of style in arch/*/include/uapi/asm/ptrace.h
>> across ports, I would prefer to stay with the array field.
>>
> Okay, I will include your patch.
> 

That'd be great.

I'll wait until Linus takes the change, and then will submit the strace
support to strace mailing list.

Thanks for the help!
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: nios2: is the ptrace ABI correct?
  2015-03-09 17:02                 ` Chung-Lin Tang
  2015-03-09 17:05                   ` Ezequiel Garcia
@ 2015-03-12 11:07                   ` Tobias Klauser
  1 sibling, 0 replies; 18+ messages in thread
From: Tobias Klauser @ 2015-03-12 11:07 UTC (permalink / raw)
  To: Chung-Lin Tang
  Cc: Chung-Lin Tang, Ezequiel Garcia, Arnd Bergmann, Ley Foon Tan,
	Walter Goossens, linux-arch, nios2-dev, linux-kernel

On 2015-03-09 at 18:02:17 +0100, Chung-Lin Tang <chunglin.tang@gmail.com> wrote:
> On 2015/3/10 12:54 AM, Chung-Lin Tang wrote:
> > It appears that some of the ways nios2 has organized the
> > ucontext/pt_regs/etc. are remnants of the pre-generic code, some
> > basically because the port was based off m68k.
> > 
> > I've re-organized the headers a bit: nios2/include/asm/ucontext.h is
> > deleted, and re-definition of struct sigcontext now allows use of
> > uapi/asm-generic/ucontext.h directly.  Note that the reorg, despite
> > effectively renaming some fields, is still binary compatible. I'll
> > probably update the corresponding glibc definitions later.
> > 
> > struct pt_regs is now not exported, and all exported register sets are
> > now supposed to follow the 49 register set defined as in GDB now.
> > 
> > Tobias, Ley Foon, how do you think this looks?
> 
> Sorry, accidentally attached unrelated GCC patch instead, this one's the
> correct one.

Sorry for jumping into the discussion so late. The patch looks fine to
me as well. Only one minor nit below. Since Ley Foon already applied it
as is, I'll just send a followup patch.

Thanks Ezequiel, Chung-Lin and Ley Foon for pushing this! Looking
forward to have strace support on nios2.

[...]
> diff --git a/arch/nios2/include/uapi/asm/Kbuild b/arch/nios2/include/uapi/asm/Kbuild
> index 4f07ca3f8..37613119 100644
> --- a/arch/nios2/include/uapi/asm/Kbuild
> +++ b/arch/nios2/include/uapi/asm/Kbuild
> @@ -2,3 +2,5 @@ include include/uapi/asm-generic/Kbuild.asm
>  
>  header-y += elf.h
>  header-y += ucontext.h

This should no longer be needed now that the non-generic ucontext.h is
gone.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* nios2: is the ptrace ABI correct?
@ 2015-02-24  2:30 Ezequiel Garcia
  0 siblings, 0 replies; 18+ messages in thread
From: Ezequiel Garcia @ 2015-02-24  2:30 UTC (permalink / raw)
  To: Tobias Klauser, Chung-Ling Tang, Walter Goossens, Ley Foon Tan,
	Linux Kernel Mailing List, Arnd Bergmann, nios2-dev
  Cc: linux-arch

Hi everyone,

I've been trying to port strace to Nios-II, but came across some oddities
in the ptrace ABI. The pt_regs structure is exposed to userspace through
the ptrace.h UAPI header: arch/nios2/include/uapi/asm/ptrace.h

Such pt_regs has the following layout:

struct pt_regs {
        unsigned long  r8;
        unsigned long  r9;
        unsigned long  r10;
	[and so on]
}

But the regset implementation in arch/nios2/kernel/ptrace.c
pushes a different layout to userspace, as it uses the PTR_
macros, also in the UAPI header:

#define PTR_R0          0
#define PTR_R1          1
#define PTR_R2          2
#define PTR_R3          3
#define PTR_R4          4
#define PTR_R5          5
#define PTR_R6          6
#define PTR_R7          7
#define PTR_R8          8
[..]

In other words, the PTRACE_GETREGSET will pass to userspace register
r8 at offset 8*4, but the pt_regs structure says it's at offset 0.

This difference appears because ptrace returns one layout to userspace,
and pushes the registers to the stack with another layout.

I've fixed this and managed to port strace by changing the genregs_get
implementation, so it returns a layout that matches pt_regs, and therefore
matches the stack.

Having this one-to-one correspondence, has a nice benefit. The implementation
is trivial:

static int genregs_get(struct task_struct *target,
                       const struct user_regset *regset,
                       unsigned int pos, unsigned int count,
                       void *kbuf, void __user *ubuf)
{
        const struct pt_regs *regs = task_pt_regs(target);
        const struct switch_stack *sw = (struct switch_stack *)regs - 1;
        int ret;

        ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
                                  regs, 0, PT_REGS_SIZE);
        if (!ret)
                ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
                                          sw, 0, SWITCH_STACK_SIZE);
        return ret;
}

However, there's a problem. The ABI is already different, and current gdb
seems to rely in the current layout. However, it does it by *not* using the
pt_regs structure.

Give the ABI is already used, I'm reluctant to change it as I don't want to
break our beloved users.

So, tried a different approach and removed pt_regs from the UAPI ptrace.h,
replacing it with a new user_regs that describes how registers are passed
to user. This however is also problematic, as pt_regs is already used
by glibc (not really sure what for).

In conclusion, I'm lost and have no clue how a proper fix
would look like. (Or perhaps, I'm really lost and there's no fix needed.)

Any ideas?

-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2015-03-12 11:12 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-02-24  3:04 nios2: is the ptrace ABI correct? Ezequiel Garcia
2015-02-24  8:54 ` Arnd Bergmann
2015-02-24 15:28   ` Ezequiel Garcia
2015-02-24 19:25     ` Arnd Bergmann
2015-02-25 11:33       ` Ezequiel Garcia
2015-02-25 14:07         ` Arnd Bergmann
2015-02-27  8:57           ` Chung-Lin Tang
2015-02-27 11:19             ` Ezequiel Garcia
2015-03-04 20:56               ` Ezequiel Garcia
2015-03-09 16:54               ` Chung-Lin Tang
2015-03-09 17:02                 ` Chung-Lin Tang
2015-03-09 17:05                   ` Ezequiel Garcia
2015-03-10  2:54                     ` Ley Foon Tan
2015-03-10  6:17                       ` Chung-Lin Tang
2015-03-11  7:48                         ` Ley Foon Tan
2015-03-11 14:31                           ` Ezequiel Garcia
2015-03-12 11:07                   ` Tobias Klauser
  -- strict thread matches above, loose matches on Subject: below --
2015-02-24  2:30 Ezequiel Garcia

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).