[PATCH] parisc: prefer _THIS_IP_ and _RET_IP_ statement expressions

[PATCH] parisc: prefer _THIS_IP_ and _RET_IP_ statement expressions

[Nick Desaulniers]

As part of the effort to reduce the code duplication between _THIS_IP_
and current_text_addr(), let's consolidate callers of
current_text_addr() to use _THIS_IP_.

Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
 arch/parisc/kernel/unwind.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/parisc/kernel/unwind.c b/arch/parisc/kernel/unwind.c
index 2ef83d78eec4..a4b430f440a9 100644
--- a/arch/parisc/kernel/unwind.c
+++ b/arch/parisc/kernel/unwind.c
@@ -439,8 +439,8 @@ unsigned long return_address(unsigned int level)
 	/* initialize unwind info */
 	asm volatile ("copy %%r30, %0" : "=r"(sp));
 	memset(&r, 0, sizeof(struct pt_regs));
-	r.iaoq[0] = (unsigned long) current_text_addr();
-	r.gr[2] = (unsigned long) __builtin_return_address(0);
+	r.iaoq[0] = _THIS_IP_;
+	r.gr[2] = _RET_IP_;
 	r.gr[30] = sp;
 	unwind_frame_init(&info, current, &r);
 
-- 
2.18.0.597.ga71716f1ad-goog

Re: [PATCH] parisc: prefer _THIS_IP_ and _RET_IP_ statement expressions

[John David Anglin]

On 2018-08-01 2:22 PM, Nick Desaulniers wrote:
> As part of the effort to reduce the code duplication between _THIS_IP_
> and current_text_addr(), let's consolidate callers of
> current_text_addr() to use _THIS_IP_.
Using the generic _THIS_IP_ results in significantly longer code than 
the parisc implementation
of current_text_addr().  It also results in a local label in the text.  
This breaks the unwind data
for the function with the label in 32-bit kernels.  The implementation 
of current_text_addr()
doesn't add a label.  _THIS_IP_ should be defined using 
current_text_addr() on parisc.
>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
>   arch/parisc/kernel/unwind.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/parisc/kernel/unwind.c b/arch/parisc/kernel/unwind.c
> index 2ef83d78eec4..a4b430f440a9 100644
> --- a/arch/parisc/kernel/unwind.c
> +++ b/arch/parisc/kernel/unwind.c
> @@ -439,8 +439,8 @@ unsigned long return_address(unsigned int level)
>   	/* initialize unwind info */
>   	asm volatile ("copy %%r30, %0" : "=r"(sp));
>   	memset(&r, 0, sizeof(struct pt_regs));
> -	r.iaoq[0] = (unsigned long) current_text_addr();
> -	r.gr[2] = (unsigned long) __builtin_return_address(0);
> +	r.iaoq[0] = _THIS_IP_;
> +	r.gr[2] = _RET_IP_;
>   	r.gr[30] = sp;
>   	unwind_frame_init(&info, current, &r);
>   

Dave

-- 
John David Anglin  dave.anglin@bell.net

Re: [PATCH] parisc: prefer _THIS_IP_ and _RET_IP_ statement expressions

[Nick Desaulniers]

Dave, thanks for the quick review!

On Wed, Aug 1, 2018 at 1:10 PM John David Anglin <dave.anglin@bell.net> wrote:
>
> On 2018-08-01 2:22 PM, Nick Desaulniers wrote:
> > As part of the effort to reduce the code duplication between _THIS_IP_
> > and current_text_addr(), let's consolidate callers of
> > current_text_addr() to use _THIS_IP_.
> Using the generic _THIS_IP_ results in significantly longer code than
> the parisc implementation
> of current_text_addr().

It might be worthwhile to file a bug with your compiler vendor.  It
seems like there may be a better way to generate code for this
construct.

Also, I'm curious how hot this code is? I assume in general that the C
construct may be larger than the inline assembly, but I'm curious if
this introduces a performance regression or just makes the code
larger?  Do you have stats on the size difference and performance
differences?  What's more important to me is whether the patch is
correct...

> It also results in a local label in the text.
> This breaks the unwind data
> for the function with the label in 32-bit kernels.  The implementation
> of current_text_addr()
> doesn't add a label.

... oh, I guess I'm surprised that the label ends up in the code, vs
there just being a constant generated.  Can you send me the
disassembly?  Also, I'm curious how does having the label present in
the text break the unwinder?  (I'm not familiar with how unwinding
works beyond following frame pointers).

> _THIS_IP_ should be defined using
> current_text_addr() on parisc.

I'm trying to eliminate current_text_addr() in the kernel, as it only
has 4 call sites (3 are arch specific).  What are your thoughts on
making the current parisc current_text_addr() just a static function
(or statement expression that's local to) in
arch/parisc/kernel/unwind.c?

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] parisc: prefer _THIS_IP_ and _RET_IP_ statement expressions

[John David Anglin]

On 2018-08-01 4:52 PM, Nick Desaulniers wrote:
> Dave, thanks for the quick review!
>
> On Wed, Aug 1, 2018 at 1:10 PM John David Anglin <dave.anglin@bell.net> wrote:
>> On 2018-08-01 2:22 PM, Nick Desaulniers wrote:
>>> As part of the effort to reduce the code duplication between _THIS_IP_
>>> and current_text_addr(), let's consolidate callers of
>>> current_text_addr() to use _THIS_IP_.
>> Using the generic _THIS_IP_ results in significantly longer code than
>> the parisc implementation
>> of current_text_addr().
> It might be worthwhile to file a bug with your compiler vendor.  It
> seems like there may be a better way to generate code for this
> construct.
>
> Also, I'm curious how hot this code is? I assume in general that the C
> construct may be larger than the inline assembly, but I'm curious if
> this introduces a performance regression or just makes the code
> larger?  Do you have stats on the size difference and performance
> differences?  What's more important to me is whether the patch is
> correct...
I exaggerated the size difference.  It's two instructions versus one on 
PA 2.0.
>
>> It also results in a local label in the text.
>> This breaks the unwind data
>> for the function with the label in 32-bit kernels.  The implementation
>> of current_text_addr()
>> doesn't add a label.
> ... oh, I guess I'm surprised that the label ends up in the code, vs
> there just being a constant generated.  Can you send me the
> disassembly?  Also, I'm curious how does having the label present in
> the text break the unwinder?  (I'm not familiar with how unwinding
> works beyond following frame pointers).
The generic code results in the following assembly code:

.L2:
         ldil LR'.L2,%r25
         ldo RR'.L2(%r25),%r25

It's the LR and RR relocations that cause the label to end up in the 
final symbol table.
The linker can't throw .L2 away  as its address has been taken.

The comparable PA 2.0 code is:

         mfia %r25

I'm aware of this issue as I just changed gcc to move branch tables to 
read-only data
when generating non-PIC code because of this issue.

Helge knows more about the unwind issues than I do as it's specific to 
the linux kernel.
Userspace uses dwarf unwind info.  I believe what happens is the 
unwinder finds the label
instead of the relevant function and its unwind data.

>
>> _THIS_IP_ should be defined using
>> current_text_addr() on parisc.
> I'm trying to eliminate current_text_addr() in the kernel, as it only
> has 4 call sites (3 are arch specific).  What are your thoughts on
> making the current parisc current_text_addr() just a static function
> (or statement expression that's local to) in
> arch/parisc/kernel/unwind.c?
I understand the desire to eliminate current_text_addr().  However, as 
it stands, using _THIS_IP_
introduces text  labels at many sites in the kernel.  That's why parisc 
needs to be able to provide its
own define for _THIS_IP_.  Currently, we have in asm/processor.h:

/*
  * Default implementation of macro that returns current
  * instruction pointer ("program counter").
  */
#ifdef CONFIG_PA20
#define current_ia(x)   __asm__("mfia %0" : "=r"(x))
#else /* mfia added in pa2.0 */
#define current_ia(x)   __asm__("blr 0,%0\n\tnop" : "=r"(x))
#endif
#define current_text_addr() ({ void *pc; current_ia(pc); pc; })

Dave

-- 
John David Anglin  dave.anglin@bell.net

Re: [PATCH] parisc: prefer _THIS_IP_ and _RET_IP_ statement expressions

[Nick Desaulniers]

On Wed, Aug 1, 2018 at 2:27 PM John David Anglin <dave.anglin@bell.net> wrote:
>
> On 2018-08-01 4:52 PM, Nick Desaulniers wrote:
> > Dave, thanks for the quick review!
> >
> > On Wed, Aug 1, 2018 at 1:10 PM John David Anglin <dave.anglin@bell.net> wrote:
> >> On 2018-08-01 2:22 PM, Nick Desaulniers wrote:
> >>> As part of the effort to reduce the code duplication between _THIS_IP_
> >>> and current_text_addr(), let's consolidate callers of
> >>> current_text_addr() to use _THIS_IP_.
> >> Using the generic _THIS_IP_ results in significantly longer code than
> >> the parisc implementation
> >> of current_text_addr().
> > It might be worthwhile to file a bug with your compiler vendor.  It
> > seems like there may be a better way to generate code for this
> > construct.
> >
> > Also, I'm curious how hot this code is? I assume in general that the C
> > construct may be larger than the inline assembly, but I'm curious if
> > this introduces a performance regression or just makes the code
> > larger?  Do you have stats on the size difference and performance
> > differences?  What's more important to me is whether the patch is
> > correct...
> I exaggerated the size difference.  It's two instructions versus one on
> PA 2.0.

Thanks for measuring.

> >
> >> It also results in a local label in the text.
> >> This breaks the unwind data
> >> for the function with the label in 32-bit kernels.  The implementation
> >> of current_text_addr()
> >> doesn't add a label.
> > ... oh, I guess I'm surprised that the label ends up in the code, vs
> > there just being a constant generated.  Can you send me the
> > disassembly?  Also, I'm curious how does having the label present in
> > the text break the unwinder?  (I'm not familiar with how unwinding
> > works beyond following frame pointers).
> The generic code results in the following assembly code:
>
> .L2:
>          ldil LR'.L2,%r25
>          ldo RR'.L2(%r25),%r25
>
> It's the LR and RR relocations that cause the label to end up in the
> final symbol table.
> The linker can't throw .L2 away  as its address has been taken.
>
> The comparable PA 2.0 code is:
>
>          mfia %r25

Thanks for the disassembly and explanation.

>
> I'm aware of this issue as I just changed gcc to move branch tables to
> read-only data
> when generating non-PIC code because of this issue.
>
> Helge knows more about the unwind issues than I do as it's specific to
> the linux kernel.
> Userspace uses dwarf unwind info.  I believe what happens is the
> unwinder finds the label
> instead of the relevant function and its unwind data.

Seems like return_address() is being used in
arch/parisc/include/asm/ftrace.h. Does v1 break tracing?  When you
found out the hard way about this requirement for labels in unwind
data, how did you reproduce (boot test that failed, ftrace failed)?
Does the same thing happen with this patch?


> >> _THIS_IP_ should be defined using
> >> current_text_addr() on parisc.
> > I'm trying to eliminate current_text_addr() in the kernel, as it only
> > has 4 call sites (3 are arch specific).  What are your thoughts on
> > making the current parisc current_text_addr() just a static function
> > (or statement expression that's local to) in
> > arch/parisc/kernel/unwind.c?
> I understand the desire to eliminate current_text_addr().  However, as
> it stands, using _THIS_IP_
> introduces text  labels at many sites in the kernel.  That's why parisc
> needs to be able to provide its
> own define for _THIS_IP_.  Currently, we have in asm/processor.h:
>
> /*
>   * Default implementation of macro that returns current
>   * instruction pointer ("program counter").
>   */
> #ifdef CONFIG_PA20
> #define current_ia(x)   __asm__("mfia %0" : "=r"(x))
> #else /* mfia added in pa2.0 */
> #define current_ia(x)   __asm__("blr 0,%0\n\tnop" : "=r"(x))
> #endif
> #define current_text_addr() ({ void *pc; current_ia(pc); pc; })

I was thinking something like:

diff --git a/arch/parisc/include/asm/processor.h
b/arch/parisc/include/asm/processor.h
index 2dbe5580a1a4..0d7f64ef9c7d 100644
--- a/arch/parisc/include/asm/processor.h
+++ b/arch/parisc/include/asm/processor.h
@@ -20,17 +20,6 @@
 #include <asm/percpu.h>
 #endif /* __ASSEMBLY__ */

-/*
- * Default implementation of macro that returns current
- * instruction pointer ("program counter").
- */
-#ifdef CONFIG_PA20
-#define current_ia(x)  __asm__("mfia %0" : "=r"(x))

-#else /* mfia added in pa2.0 */
-#define current_ia(x)  __asm__("blr 0,%0\n\tnop" : "=r"(x))
-#endif
-#define current_text_addr() ({ void *pc; current_ia(pc); pc; })
-
 #define HAVE_ARCH_PICK_MMAP_LAYOUT

 #define TASK_SIZE_OF(tsk)       ((tsk)->thread.task_size)
diff --git a/arch/parisc/kernel/unwind.c b/arch/parisc/kernel/unwind.c
index 2ef83d78eec4..818ac8a60a4a 100644
--- a/arch/parisc/kernel/unwind.c
+++ b/arch/parisc/kernel/unwind.c
@@ -430,6 +430,18 @@ int unwind_to_user(struct unwind_frame_info *info)
        return ret;
 }

+/*
+ * Arch-specific marcro that returns the current instruction pointer ("program
+ * counter").  Prefer this to _THIS_IP_ for the sole purpose of not emitting a
+ * label, which breaks unwinding.
+ */
+#ifdef CONFIG_PA20
+#define current_ia(x)   __asm__("mfia %0" : "=r"(x))
+#else /* mfia added in pa2.0 */
+#define current_ia(x)   __asm__("blr 0,%0\n\tnop" : "=r"(x))
+#endif
+#define current_text_addr ({ void *pc; current_ia(pc); (unsigned long) pc; })
+
 unsigned long return_address(unsigned int level)
 {
        struct unwind_frame_info info;
@@ -439,8 +451,8 @@ unsigned long return_address(unsigned int level)
        /* initialize unwind info */
        asm volatile ("copy %%r30, %0" : "=r"(sp));
        memset(&r, 0, sizeof(struct pt_regs));
-       r.iaoq[0] = (unsigned long) current_text_addr();
-       r.gr[2] = (unsigned long) __builtin_return_address(0);
+       r.iaoq[0] = current_text_addr;
+       r.gr[2] = _RET_IP_;
        r.gr[30] = sp;
        unwind_frame_init(&info, current, &r);

Thoughts?  Idea being there's only one call site in your tree that has
this requirement (and the other one in
include/net/inet_connection_sock.h I don't think is correct, and will
send a patch out imminently).

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] parisc: prefer _THIS_IP_ and _RET_IP_ statement expressions

[John David Anglin]

On 2018-08-01 5:49 PM, Nick Desaulniers wrote:
> Thoughts?  Idea being there's only one call site in your tree that has
> this requirement (and the other one in
> include/net/inet_connection_sock.h I don't think is correct, and will
> send a patch out imminently).
What about the uses in the fs support, etc?

Dave

-- 
John David Anglin  dave.anglin@bell.net

Re: [PATCH] parisc: prefer _THIS_IP_ and _RET_IP_ statement expressions

[Nick Desaulniers]

On Wed, Aug 1, 2018 at 3:12 PM John David Anglin <dave.anglin@bell.net> wrote:
>
> On 2018-08-01 5:49 PM, Nick Desaulniers wrote:
> > Thoughts?  Idea being there's only one call site in your tree that has
> > this requirement (and the other one in
> > include/net/inet_connection_sock.h I don't think is correct, and will
> > send a patch out imminently).

Turns out it is correct (I assumed by caller, they meant _RET_IP_, but
they're in an inline function). https://lkml.org/lkml/2018/8/1/1687 is
the patch.

> What about the uses in the fs support, etc?

Sorry, I don't see it?

$ ag current_text_addr
and
$ grep -R current_text_addr

turn up nothing in fs/.  Is this torvals/linux with no out of tree patches?
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] parisc: prefer _THIS_IP_ and _RET_IP_ statement expressions

[John David Anglin]

On 2018-08-01 6:18 PM, Nick Desaulniers wrote:
>> What about the uses in the fs support, etc?
> Sorry, I don't see it?
I mean _THIS_IP_.

Dave

-- 
John David Anglin  dave.anglin@bell.net

Re: [PATCH] parisc: prefer _THIS_IP_ and _RET_IP_ statement expressions

[Nick Desaulniers]

On Wed, Aug 1, 2018 at 5:49 PM John David Anglin <dave.anglin@bell.net> wrote:
>
> On 2018-08-01 6:18 PM, Nick Desaulniers wrote:
> >> What about the uses in the fs support, etc?
> > Sorry, I don't see it?
> I mean _THIS_IP_.

I don't understand, I'm referring to current_text_addr().  Maybe this
explains more what I'm trying to do:
https://lkml.org/lkml/2018/8/1/1689

If I understand your point correctly, is it that you're saying that
_THIS_IP_ should be implemented in terms of inline assembly (as in
what current_text_addr() is currently)?  If that's what you mean and
I'm understanding correctly, my point is that we should be preferring
the generic C implementation as that's what's being used in most
places currently, so if it was broken you'd likely already know about
it.  Unless unwinding is truly broken by the additional label, I don't
think we need an inline assembly implementation of current_text_addr()
for parisc (or any arch for that matter).  If we do, then it can be
localized to the parisc unwinding code, that way it can be
consolidated everywhere else for every other arch.

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] parisc: prefer _THIS_IP_ and _RET_IP_ statement expressions

[John David Anglin]

On 2018-08-02 4:31 PM, Nick Desaulniers wrote:
> If I understand your point correctly, is it that you're saying that
> _THIS_IP_  should be implemented in terms of inline assembly (as in
> what current_text_addr() is currently)?  If that's what you mean and
> I'm understanding correctly, my point is that we should be preferring
> the generic C implementation as that's what's being used in most
> places currently, so if it was broken you'd likely already know about
> it.  Unless unwinding is truly broken by the additional label, I don't
> think we need an inline assembly implementation of current_text_addr()
> for parisc (or any arch for that matter).  If we do, then it can be
> localized to the parisc unwinding code, that way it can be
> consolidated everywhere else for every other arch.
The label breaks the unwind data, not the unwind code.  So, localizing 
the use of
current_text_addr() to the parisc unwind code doesn't help.

Personally, I prefer the implementation of current_text_addr() because:

1) The generated code is smaller, and
2) it doesn't introduce any unnecessary labels into the text.

As noted, these labels can cause issues with unwinding.

Dave

-- 
John David Anglin  dave.anglin@bell.net

Re: [PATCH] parisc: prefer _THIS_IP_ and _RET_IP_ statement expressions

[Nick Desaulniers]

On Fri, Aug 3, 2018 at 10:57 AM John David Anglin <dave.anglin@bell.net> wrote:
>
> On 2018-08-02 4:31 PM, Nick Desaulniers wrote:
> > If I understand your point correctly, is it that you're saying that
> > _THIS_IP_  should be implemented in terms of inline assembly (as in
> > what current_text_addr() is currently)?  If that's what you mean and
> > I'm understanding correctly, my point is that we should be preferring
> > the generic C implementation as that's what's being used in most
> > places currently, so if it was broken you'd likely already know about
> > it.  Unless unwinding is truly broken by the additional label, I don't
> > think we need an inline assembly implementation of current_text_addr()
> > for parisc (or any arch for that matter).  If we do, then it can be
> > localized to the parisc unwinding code, that way it can be
> > consolidated everywhere else for every other arch.
> The label breaks the unwind data, not the unwind code.  So, localizing
> the use of
> current_text_addr() to the parisc unwind code doesn't help.

But the kernel uses the generic _THIS_IP_ *everywhere*, not parisc's
custom current_text_addr().  So if this did actually break unwinding,
you should have noticed by now.  There's only one call site that uses
the custom current_text_addr(), which is what my patch is addressing.

Localizing the custom implementation would literally produce the same
binary as is produced today, but allow us to start removing all the
other custom implementations of current_text_addr() from
arch/*/include/asm/processor.h, which is why I proposed that as an
alternative to this patch.

> Personally, I prefer the implementation of current_text_addr() because:
>
> 1) The generated code is smaller, and

One instruction vs two, for a single call site that I bet is cold and
not inlined in multiple places.

> 2) it doesn't introduce any unnecessary labels into the text.
>
> As noted, these labels can cause issues with unwinding.

Can you confirm that applying this patch actually breaks unwinding?

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] parisc: prefer _THIS_IP_ and _RET_IP_ statement expressions

[John David Anglin]

On 2018-08-03 2:11 PM, Nick Desaulniers wrote:
> But the kernel uses the generic_THIS_IP_  *everywhere*, not parisc's
> custom current_text_addr().  So if this did actually break unwinding,
> you should have noticed by now.
The unwind problem was noticed.

Patches were recently applied to gcc and binutils to try and fix it.  
The gcc patch moved
branch tables to rodata so that the label at the head of the table 
wasn't in text.

https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01804.html
https://sourceware.org/ml/binutils/2018-07/msg00474.html

When I saw your suggested change, I realized there was another source of 
text labels
that need linker relocations.

Dave

-- 
John David Anglin  dave.anglin@bell.net

Re: [PATCH] parisc: prefer _THIS_IP_ and _RET_IP_ statement expressions

[Nick Desaulniers]

On Fri, Aug 3, 2018 at 12:09 PM John David Anglin <dave.anglin@bell.net> wrote:
>
> On 2018-08-03 2:11 PM, Nick Desaulniers wrote:
> > But the kernel uses the generic_THIS_IP_  *everywhere*, not parisc's
> > custom current_text_addr().  So if this did actually break unwinding,
> > you should have noticed by now.
> The unwind problem was noticed.

So parisc is currently broken (doesn't unwind) due to the pervasive
use of _THIS_IP_ (generic C) throughout the kernel?

If no, that implies this patch (generic C) causes no unwinding problems.
If yes, that implies that the diff I posted later in this thread
(inline assembly) is preferable, and that parisc has bigger problems
(and probably needs to do rewrite the unwinding code to handle these
extra labels everywhere).

> Patches were recently applied to gcc and binutils to try and fix it.
> The gcc patch moved
> branch tables to rodata so that the label at the head of the table
> wasn't in text.
>
> https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01804.html
> https://sourceware.org/ml/binutils/2018-07/msg00474.html
>
> When I saw your suggested change, I realized there was another source of
> text labels
> that need linker relocations.

Thank you for the links.

On Fri, Aug 3, 2018 at 10:57 AM John David Anglin <dave.anglin@bell.net> wrote:
> The label breaks the unwind data, not the unwind code.  So, localizing
> the use of
> current_text_addr() to the parisc unwind code doesn't help.

Have you confirmed that applying my patch breaks *the ability to
unwind correctly*? It looks like return_address() is used in
ftrace_return_address(), so I assume you can boot a kernel with my
patch applied, and CONFIG_FTRACE=y, then run:

$ sudo trace-cmd record -p function date
$ trace-cmd report | grep date- | less

and see if the stacks aren't unwound or look messed up.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] parisc: prefer _THIS_IP_ and _RET_IP_ statement expressions

[Helge Deller]

On 03.08.2018 22:33, Nick Desaulniers wrote:
> On Fri, Aug 3, 2018 at 12:09 PM John David Anglin <dave.anglin@bell.net> wrote:
>>
>> On 2018-08-03 2:11 PM, Nick Desaulniers wrote:
>>> But the kernel uses the generic_THIS_IP_  *everywhere*, not parisc's
>>> custom current_text_addr().  So if this did actually break unwinding,
>>> you should have noticed by now.
>> The unwind problem was noticed.
> 
> So parisc is currently broken (doesn't unwind) due to the pervasive
> use of _THIS_IP_ (generic C) throughout the kernel?

I tested it on the 32bit kernel.
The answer is: No. Unwinding works (with and without your patch).
 
> If no, that implies this patch (generic C) causes no unwinding problems.

correct.

> If yes, that implies that the diff I posted later in this thread
> (inline assembly) is preferable, and that parisc has bigger problems
> (and probably needs to do rewrite the unwinding code to handle these
> extra labels everywhere).
> 
>> Patches were recently applied to gcc and binutils to try and fix it.
>> The gcc patch moved
>> branch tables to rodata so that the label at the head of the table
>> wasn't in text.
>>
>> https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01804.html
>> https://sourceware.org/ml/binutils/2018-07/msg00474.html
>>
>> When I saw your suggested change, I realized there was another source of
>> text labels
>> that need linker relocations.
> 
> Thank you for the links.
> 
> On Fri, Aug 3, 2018 at 10:57 AM John David Anglin <dave.anglin@bell.net> wrote:
>> The label breaks the unwind data, not the unwind code.  So, localizing
>> the use of
>> current_text_addr() to the parisc unwind code doesn't help.
> 
> Have you confirmed that applying my patch breaks *the ability to
> unwind correctly*?

I tested your patch (on 32bit).
Your patch does not break anything.

> It looks like return_address() is used in
> ftrace_return_address(), so I assume you can boot a kernel with my
> patch applied, and CONFIG_FTRACE=y, then run:
> 
> $ sudo trace-cmd record -p function date
> $ trace-cmd report | grep date- | less
> 
> and see if the stacks aren't unwound or look messed up.

I faced issues with trace-cmd, but calling ftracing functions manually worked.

So, your patch is basically OK and doesn't break anything.
But I agree with Dave that Andrew, that THIS_IP is ugly.

Helge

Re: [PATCH] parisc: prefer _THIS_IP_ and _RET_IP_ statement expressions

[Nick Desaulniers]

On Fri, Aug 3, 2018 at 3:34 PM Helge Deller <deller@gmx.de> wrote:
>
> On 03.08.2018 22:33, Nick Desaulniers wrote:
> > On Fri, Aug 3, 2018 at 12:09 PM John David Anglin <dave.anglin@bell.net> wrote:
> >>
> >> On 2018-08-03 2:11 PM, Nick Desaulniers wrote:
> >>> But the kernel uses the generic_THIS_IP_  *everywhere*, not parisc's
> >>> custom current_text_addr().  So if this did actually break unwinding,
> >>> you should have noticed by now.
> >> The unwind problem was noticed.
> >
> > So parisc is currently broken (doesn't unwind) due to the pervasive
> > use of _THIS_IP_ (generic C) throughout the kernel?
>
> I tested it on the 32bit kernel.
> The answer is: No. Unwinding works (with and without your patch).
>
> > If no, that implies this patch (generic C) causes no unwinding problems.
>
> correct.
>
> > If yes, that implies that the diff I posted later in this thread
> > (inline assembly) is preferable, and that parisc has bigger problems
> > (and probably needs to do rewrite the unwinding code to handle these
> > extra labels everywhere).
> >
> >> Patches were recently applied to gcc and binutils to try and fix it.
> >> The gcc patch moved
> >> branch tables to rodata so that the label at the head of the table
> >> wasn't in text.
> >>
> >> https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01804.html
> >> https://sourceware.org/ml/binutils/2018-07/msg00474.html
> >>
> >> When I saw your suggested change, I realized there was another source of
> >> text labels
> >> that need linker relocations.
> >
> > Thank you for the links.
> >
> > On Fri, Aug 3, 2018 at 10:57 AM John David Anglin <dave.anglin@bell.net> wrote:
> >> The label breaks the unwind data, not the unwind code.  So, localizing
> >> the use of
> >> current_text_addr() to the parisc unwind code doesn't help.
> >
> > Have you confirmed that applying my patch breaks *the ability to
> > unwind correctly*?
>
> I tested your patch (on 32bit).
> Your patch does not break anything.
>
> > It looks like return_address() is used in
> > ftrace_return_address(), so I assume you can boot a kernel with my
> > patch applied, and CONFIG_FTRACE=y, then run:
> >
> > $ sudo trace-cmd record -p function date
> > $ trace-cmd report | grep date- | less
> >
> > and see if the stacks aren't unwound or look messed up.
>
> I faced issues with trace-cmd, but calling ftracing functions manually worked.
>
> So, your patch is basically OK and doesn't break anything.
> But I agree with Dave that Andrew, that THIS_IP is ugly.

I don't disagree, and other maintainers have remarked on _THIS_IP_
being ugly, but renaming it en masse is a tree wide change, which I'm
trying to avoid at the moment.

It sounds like we have a working patch? Are there 64b parisc machines
to test on, or can this get merged?
--
Thanks,
~Nick Desaulniers

Re: [PATCH] parisc: prefer _THIS_IP_ and _RET_IP_ statement expressions

[Helge Deller]

On 07.08.2018 20:11, Nick Desaulniers wrote:
> On Fri, Aug 3, 2018 at 3:34 PM Helge Deller <deller@gmx.de> wrote:
>> So, your patch is basically OK and doesn't break anything.
>> But I agree with Dave and Andrew, that THIS_IP is ugly.
> 
> I don't disagree, and other maintainers have remarked on _THIS_IP_
> being ugly, but renaming it en masse is a tree wide change, which I'm
> trying to avoid at the moment.

Understandable.

> It sounds like we have a working patch? Are there 64b parisc machines
> to test on, or can this get merged?

Go ahead and merge it.

In addition, somehow I'd prefer if there would be a way to add to include/linux/kernel.h:
+#if !defined(_THIS_IP_)
#define _THIS_IP_  ({ __label__ __here; __here: (unsigned long)&&__here; })
+#endif

That way it would somehow be possible to replace that label calulation by the
preferable inline assembly of current_text_address()...

Helge

Re: [PATCH] parisc: prefer _THIS_IP_ and _RET_IP_ statement expressions

[Nick Desaulniers]

On Tue, Aug 7, 2018 at 1:30 PM Helge Deller <deller@gmx.de> wrote:
>
> On 07.08.2018 20:11, Nick Desaulniers wrote:
> > On Fri, Aug 3, 2018 at 3:34 PM Helge Deller <deller@gmx.de> wrote:
> >> So, your patch is basically OK and doesn't break anything.
> >> But I agree with Dave and Andrew, that THIS_IP is ugly.
> >
> > I don't disagree, and other maintainers have remarked on _THIS_IP_
> > being ugly, but renaming it en masse is a tree wide change, which I'm
> > trying to avoid at the moment.
>
> Understandable.
>
> > It sounds like we have a working patch? Are there 64b parisc machines
> > to test on, or can this get merged?
>
> Go ahead and merge it.

Thank you, but I was under the impression this would go up through the
parisc tree?
https://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux.git/
right? Or is there a different tree/maintainer I should ask?

> In addition, somehow I'd prefer if there would be a way to add to include/linux/kernel.h:
> +#if !defined(_THIS_IP_)
> #define _THIS_IP_  ({ __label__ __here; __here: (unsigned long)&&__here; })
> +#endif
>
> That way it would somehow be possible to replace that label calulation by the
> preferable inline assembly of current_text_address()...

I'm asserting that there is no need within the entire kernel at the
moment to have inline assembly to get the instruction pointer.  If
there are no call sites of the inline assembly version, there is no
need to define per arch inline assembly versions when C (with GNU
extensions) will suffice.

And if in the future there are, either those call sites can have a
local implementation (as in the second diff I sent in this thread), or
some other change to _THIS_IP_ (as you propose) can be made.  But
until then...

YAGNI: "You Ain't Gonna Need It"

Once this patch and the other 3 outstanding ones are merged, we'll be
sending patches to delete all arch specific assembly implementations
as they will be dead code (no callers, kernel-wide).
-- 
Thanks,
~Nick Desaulniers