Re: [PATCH] bug.h: Work around GCC PR82365 in BUG()
On Tue, Dec 19, 2017 at 11:38 PM, Vineet Gupta
wrote:
> On 12/19/2017 12:13 PM, Arnd Bergmann wrote:
>>
>>
>>> I suppose BUG() implies "dead end" like semantics - which ARC was lacking
>>> before ?
>>
>> Correct. Using __builtin_trap() here avoids the 'control reaches end of
>> non-void
>> function' warnings, but then makes us run into the stack size problem that
>> I work around with the barrier_before_unreachable().
>>
>> It would be good if you could give this a quick test to see if you get
>> sensible
>> output from the __builtin_trap();
>
>
> It does, added a BUG() arbit, hits an abort()
>
> ...
> ISA Extn: atomic ll64 unalign (not used)
> : mpy[opt 9] div_rem norm barrel-shift swap minmax swape
> BPU: partial match, cache:2048, Predict Table:16384
> BUG: failure at ../arch/arc/mm/tlb.c:827/arc_mmu_init()!
>
>
> Tested-by: Vineet Gupta
I meant whether it prints the right registers and stack trace, but I
assume you tested that and just did not list it above.
> FWIW newer ARC gcc actually implements the builtin so we get a trap 5
> instruction now, vs., abort() calls before.
>
> BTW I missed reading the hunk of your changelog where this addresses the
> long standing mystery with ARC builds and numerous -Wreturn-type warnings. I
> always wondered why they were not fixed upstream already, being too lazy to
> investigate myself, and turns out this was due to this BUG() thingy. phew !
Hmm, so with the new definition of abort(),
+__weak void abort(void)
+{
+ BUG();
+
+ /* if that doesn't kill us, halt */
+ panic("Oops failed to kill thread");
+}
won't that run into an endless recursion? Or do you then override abort()
for ARC?
Arnd
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 00/13] replace print_symbol() with printk()-s
On (12/11/17 21:50), Sergey Senozhatsky wrote: > > A rather automatic replacement of print_symbol() > with direct printk() calls. print_symbol() uses extra stack > buffer (KSYM_SYMBOL_LEN 128 bytes) and, basically, should > be identical to printk(%pS). > > I can't test all of the patches, because I don't > own any of those exotic arch-s. Sorry for the inconvenience. > > Sergey Senozhatsky (13): > arm: do not use print_symbol() > arm64: do not use print_symbol() > c6x: do not use print_symbol() > ia64: do not use print_symbol() > mn10300: do not use print_symbol() > sh: do not use print_symbol() > unicore32: do not use print_symbol() > x86: do not use print_symbol() > drivers: do not use print_symbol() > sysfs: do not use print_symbol() > irq debug: do not use print_symbol() > lib: do not use print_symbol() > arc: do not use __print_symbol() Hello, can we please have more reviews/acks/etc? -ss ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH] bug.h: Work around GCC PR82365 in BUG()
On 12/20/2017 01:01 AM, Arnd Bergmann wrote:
On Tue, Dec 19, 2017 at 11:38 PM, Vineet Gupta
wrote:
On 12/19/2017 12:13 PM, Arnd Bergmann wrote:
I suppose BUG() implies "dead end" like semantics - which ARC was lacking
before ?
Correct. Using __builtin_trap() here avoids the 'control reaches end of
non-void
function' warnings, but then makes us run into the stack size problem that
I work around with the barrier_before_unreachable().
It would be good if you could give this a quick test to see if you get
sensible
output from the __builtin_trap();
It does, added a BUG() arbit, hits an abort()
...
ISA Extn: atomic ll64 unalign (not used)
: mpy[opt 9] div_rem norm barrel-shift swap minmax swape
BPU: partial match, cache:2048, Predict Table:16384
BUG: failure at ../arch/arc/mm/tlb.c:827/arc_mmu_init()!
Tested-by: Vineet Gupta
I meant whether it prints the right registers and stack trace, but I
assume you tested that and just did not list it above.
Sorry, I didn't realize we are missing the stack trace now which you removed from
the patch - why ? Did u intend to reduce inline generated code for the stack dump
calls - which sounds like a great idea. But it would only work for the synchronous
abort() but not when builtin translates to actual trap inducing instruction.
Hmm, so with the new definition of abort(),
+__weak void abort(void)
+{
+ BUG();
+
+ /* if that doesn't kill us, halt */
+ panic("Oops failed to kill thread");
+}
won't that run into an endless recursion? Or do you then override abort()
for ARC?
Indeed so. I didn't run into this in my testing as my for-curr has an ARC specific
version (predating Sudip's generic version- because of build failures in our
internal regression jobs etc). That version only calls panic.
abort() is only likely to be called due to __builtin_trap() for arches where gcc
doesn't have a target specific defn of it. And thus adding the call from BUG()
will cause the recursion as you found out with Sudip's generic version and thus
needs a fixup.
Thx,
-Vineet
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH] bug.h: Work around GCC PR82365 in BUG()
On Wed, Dec 20, 2017 at 7:52 PM, Vineet Gupta
wrote:
> On 12/20/2017 01:01 AM, Arnd Bergmann wrote:
>>
>> On Tue, Dec 19, 2017 at 11:38 PM, Vineet Gupta
>> wrote:
>>>
>>> On 12/19/2017 12:13 PM, Arnd Bergmann wrote:
> I suppose BUG() implies "dead end" like semantics - which ARC was
> lacking
> before ?
Correct. Using __builtin_trap() here avoids the 'control reaches end of
non-void
function' warnings, but then makes us run into the stack size problem
that
I work around with the barrier_before_unreachable().
It would be good if you could give this a quick test to see if you get
sensible
output from the __builtin_trap();
>>>
>>>
>>>
>>> It does, added a BUG() arbit, hits an abort()
>>>
>>> ...
>>> ISA Extn: atomic ll64 unalign (not used)
>>> : mpy[opt 9] div_rem norm barrel-shift swap minmax swape
>>> BPU: partial match, cache:2048, Predict Table:16384
>>> BUG: failure at ../arch/arc/mm/tlb.c:827/arc_mmu_init()!
>>>
>>>
>>> Tested-by: Vineet Gupta
>>
>>
>> I meant whether it prints the right registers and stack trace, but I
>> assume you tested that and just did not list it above.
>
>
> Sorry, I didn't realize we are missing the stack trace now which you removed
> from the patch - why ? Did u intend to reduce inline generated code for the
> stack dump calls - which sounds like a great idea. But it would only work
> for the synchronous abort() but not when builtin translates to actual trap
> inducing instruction.
I assumed that the trap instruction would trigger the register and
stack dump, as it does on all other architectures. The most common
way this is handled is to have one instruction that is known to trap,
and use that to trigger a BUG(), and have __builtin_trap() issue
that instruction as well.
You might also want to implement CONFIG_DEBUG_BUGVERBOSE
support to attach further data to it.
>> Hmm, so with the new definition of abort(),
>>
>> +__weak void abort(void)
>> +{
>> + BUG();
>> +
>> + /* if that doesn't kill us, halt */
>> + panic("Oops failed to kill thread");
>> +}
>>
>> won't that run into an endless recursion? Or do you then override abort()
>> for ARC?
>
>
> Indeed so. I didn't run into this in my testing as my for-curr has an ARC
> specific version (predating Sudip's generic version- because of build
> failures in our internal regression jobs etc). That version only calls
> panic.
>
> abort() is only likely to be called due to __builtin_trap() for arches where
> gcc doesn't have a target specific defn of it. And thus adding the call from
> BUG() will cause the recursion as you found out with Sudip's generic version
> and thus needs a fixup.
How about overriding abort() with the same instruction that
__builtin_trap() inserts on newer compilers then? That should
make the behavior consistent.
Arnd
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH] bug.h: Work around GCC PR82365 in BUG()
On 12/20/2017 12:12 PM, Arnd Bergmann wrote: Sorry, I didn't realize we are missing the stack trace now which you removed from the patch - why ? Did u intend to reduce inline generated code for the stack dump calls - which sounds like a great idea. But it would only work for the synchronous abort() but not when builtin translates to actual trap inducing instruction. I assumed that the trap instruction would trigger the register and stack dump, as it does on all other architectures. Only if __builtin_trap() translated to that instruction. Otherwise we need to do this inside abort() The most common way this is handled is to have one instruction that is known to trap, and use that to trigger a BUG(), and have __builtin_trap() issue that instruction as well. Good point. So we'll need ARC specific abort anyways. You might also want to implement CONFIG_DEBUG_BUGVERBOSE support to attach further data to it. OK I'll take a look ! How about overriding abort() with the same instruction that __builtin_trap() inserts on newer compilers then? That should make the behavior consistent. Yeah this is a great point ! Will do thx, -Vineet ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
[PATCH 1/2] ARC: handle gcc generated __builtin_trap()
gcc toggle -fisolate-erroneous-paths-dereference (default at -O2
onwards) isolates faulty code paths such as null pointer access, divide
by zero etc by emitting __builtin_trap()
Newer ARC gcc generates TRAP_S 5 instruction correspondingly which this
patch handles. If user mode, the task is terminated with a SEGV, for
kernel mode die() called after register and stack dump.
Signed-off-by: Vineet Gupta
---
arch/arc/kernel/traps.c| 6 ++
arch/arc/kernel/troubleshoot.c | 3 +++
2 files changed, 9 insertions(+)
diff --git a/arch/arc/kernel/traps.c b/arch/arc/kernel/traps.c
index bcd7c9fc5d0f..004f4e4a4c10 100644
--- a/arch/arc/kernel/traps.c
+++ b/arch/arc/kernel/traps.c
@@ -83,6 +83,7 @@ DO_ERROR_INFO(SIGILL, "Illegal Insn (or Seq)",
insterror_is_error, ILL_ILLOPC)
DO_ERROR_INFO(SIGBUS, "Invalid Mem Access", __weak do_memory_error, BUS_ADRERR)
DO_ERROR_INFO(SIGTRAP, "Breakpoint Set", trap_is_brkpt, TRAP_BRKPT)
DO_ERROR_INFO(SIGBUS, "Misaligned Access", do_misaligned_error, BUS_ADRALN)
+DO_ERROR_INFO(SIGSEGV, "gcc generated __builtin_trap", do_trap5_error, 0)
/*
* Entry Point for Misaligned Data access Exception, for emulating in software
@@ -115,6 +116,8 @@ void do_machine_check_fault(unsigned long address, struct
pt_regs *regs)
* Thus TRAP_S can be used for specific purpose
* -1 used for software breakpointing (gdb)
* -2 used by kprobes
+ * -5 __builtin_trap() generated by gcc (2018.03 onwards) for toggle such as
+ * -fno-isolate-erroneous-paths-dereference
*/
void do_non_swi_trap(unsigned long address, struct pt_regs *regs)
{
@@ -134,6 +137,9 @@ void do_non_swi_trap(unsigned long address, struct pt_regs
*regs)
kgdb_trap(regs);
break;
+ case 5:
+ do_trap5_error(address, regs);
+ break;
default:
break;
}
diff --git a/arch/arc/kernel/troubleshoot.c b/arch/arc/kernel/troubleshoot.c
index 7d8c1d6c2f60..6e9a0a9a6a04 100644
--- a/arch/arc/kernel/troubleshoot.c
+++ b/arch/arc/kernel/troubleshoot.c
@@ -163,6 +163,9 @@ static void show_ecr_verbose(struct pt_regs *regs)
else
pr_cont("Bus Error, check PRM\n");
#endif
+ } else if (vec == ECR_V_TRAP) {
+ if (regs->ecr_param == 5)
+ pr_cont("gcc generated __builtin_trap\n");
} else {
pr_cont("Check Programmer's Manual\n");
}
--
2.7.4
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
[PATCH 0/2] ARC __builtin_trap handling
Vineet Gupta (2): ARC: handle gcc generated __builtin_trap() ARC: handle gcc generated __builtin_trap for older compiler arch/arc/kernel/traps.c| 14 ++ arch/arc/kernel/troubleshoot.c | 3 +++ 2 files changed, 17 insertions(+) -- 2.7.4 ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
[PATCH 2/2] ARC: handle gcc generated __builtin_trap for older compiler
ARC gcc prior to GNU 2018.03 release didn't have a target specific
__builtin_trap() implementation, generating default abort() call.
Implement the abort() call - emulating what newer gcc does for the same,
as suggested by Arnd.
Signed-off-by: Vineet Gupta
---
arch/arc/kernel/traps.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/arch/arc/kernel/traps.c b/arch/arc/kernel/traps.c
index 004f4e4a4c10..51a55b06cb2a 100644
--- a/arch/arc/kernel/traps.c
+++ b/arch/arc/kernel/traps.c
@@ -161,3 +161,12 @@ void do_insterror_or_kprobe(unsigned long address, struct
pt_regs *regs)
insterror_is_error(address, regs);
}
+
+/*
+ * abort() call generated by older gcc for __builtin_trap()
+ */
+void abort(void)
+{
+ __asm__ __volatile__("trap_s 5\n");
+}
+EXPORT_SYMBOL(abort);
--
2.7.4
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH 00/13] replace print_symbol() with printk()-s
On (12/11/17 19:10), Joe Perches wrote:
[..]
> As far as I'm concerned, as soon as there is
> no longer a single user in the kernel tree,
> better to delete it instead.
sounds good to me. can drop it, once the series upstreamed.
8< ---
From: Sergey Senozhatsky
Subject: [PATCH] kallsyms: remove print_symbol() function
No more print_symbol()/__print_symbol() users left, remove these
symbols.
Signed-off-by: Sergey Senozhatsky
Suggested-by: Joe Perches
---
Documentation/filesystems/sysfs.txt| 4 ++--
Documentation/translations/zh_CN/filesystems/sysfs.txt | 4 ++--
include/linux/kallsyms.h | 18 --
kernel/kallsyms.c | 11 ---
4 files changed, 4 insertions(+), 33 deletions(-)
diff --git a/Documentation/filesystems/sysfs.txt
b/Documentation/filesystems/sysfs.txt
index 9a3658cc399e..a1426cabcef1 100644
--- a/Documentation/filesystems/sysfs.txt
+++ b/Documentation/filesystems/sysfs.txt
@@ -154,8 +154,8 @@ static ssize_t dev_attr_show(struct kobject *kobj, struct
attribute *attr,
if (dev_attr->show)
ret = dev_attr->show(dev, dev_attr, buf);
if (ret >= (ssize_t)PAGE_SIZE) {
-print_symbol("dev_attr_show: %s returned bad count\n",
-(unsigned long)dev_attr->show);
+printk("dev_attr_show: %pS returned bad count\n",
+dev_attr->show);
}
return ret;
}
diff --git a/Documentation/translations/zh_CN/filesystems/sysfs.txt
b/Documentation/translations/zh_CN/filesystems/sysfs.txt
index 7d3b05edb8ce..452271dda141 100644
--- a/Documentation/translations/zh_CN/filesystems/sysfs.txt
+++ b/Documentation/translations/zh_CN/filesystems/sysfs.txt
@@ -167,8 +167,8 @@ static ssize_t dev_attr_show(struct kobject *kobj, struct
attribute *attr,
if (dev_attr->show)
ret = dev_attr->show(dev, dev_attr, buf);
if (ret >= (ssize_t)PAGE_SIZE) {
-print_symbol("dev_attr_show: %s returned bad count\n",
-(unsigned long)dev_attr->show);
+printk("dev_attr_show: %pS returned bad count\n",
+dev_attr->show);
}
return ret;
}
diff --git a/include/linux/kallsyms.h b/include/linux/kallsyms.h
index 7288f9c395b6..d79d1e7486bd 100644
--- a/include/linux/kallsyms.h
+++ b/include/linux/kallsyms.h
@@ -94,9 +94,6 @@ extern int sprint_symbol(char *buffer, unsigned long address);
extern int sprint_symbol_no_offset(char *buffer, unsigned long address);
extern int sprint_backtrace(char *buffer, unsigned long address);
-/* Look up a kernel symbol and print it to the kernel messages. */
-extern void __print_symbol(const char *fmt, unsigned long address);
-
int lookup_symbol_name(unsigned long addr, char *symname);
int lookup_symbol_attrs(unsigned long addr, unsigned long *size, unsigned long
*offset, char *modname, char *name);
@@ -166,23 +163,8 @@ static inline int kallsyms_show_value(void)
return false;
}
-/* Stupid that this does nothing, but I didn't create this mess. */
-#define __print_symbol(fmt, addr)
#endif /*CONFIG_KALLSYMS*/
-/* This macro allows us to keep printk typechecking */
-static __printf(1, 2)
-void __check_printsym_format(const char *fmt, ...)
-{
-}
-
-static inline void print_symbol(const char *fmt, unsigned long addr)
-{
- __check_printsym_format(fmt, "");
- __print_symbol(fmt, (unsigned long)
- __builtin_extract_return_addr((void *)addr));
-}
-
static inline void print_ip_sym(unsigned long ip)
{
printk("[<%p>] %pS\n", (void *) ip, (void *) ip);
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index 24f456689f9c..a23e21ada81b 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -429,17 +429,6 @@ int sprint_backtrace(char *buffer, unsigned long address)
return __sprint_symbol(buffer, address, -1, 1);
}
-/* Look up a kernel symbol and print it to the kernel messages. */
-void __print_symbol(const char *fmt, unsigned long address)
-{
- char buffer[KSYM_SYMBOL_LEN];
-
- sprint_symbol(buffer, address);
-
- printk(fmt, buffer);
-}
-EXPORT_SYMBOL(__print_symbol);
-
/* To avoid using get_symbol_offset for every symbol, we carry prefix along. */
struct kallsym_iter {
loff_t pos;
--
2.15.1
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
