Re: [PATCH -next V16 4/7] riscv: entry: Convert to generic entry

[Conor Dooley]

Hey!

On Sun, Feb 05, 2023 at 03:04:37PM +0100, Conor Dooley wrote:
> On 5 February 2023 14:56:01 GMT+01:00, Guo Ren <guoren@xxxxxxxxxx> wrote:
> >On Sun, Feb 5, 2023 at 6:43 PM Conor Dooley <conor@xxxxxxxxxx> wrote:
> >> On 4 February 2023 08:02:10 GMT+01:00, guoren@xxxxxxxxxx wrote:
> >> >From: Guo Ren <guoren@xxxxxxxxxxxxxxxxx>
> >> >
> >> >This patch converts riscv to use the generic entry infrastructure from
> >> >kernel/entry/*. The generic entry makes maintainers' work easier and
> >> >codes more elegant. Here are the changes:
> >> >
> >> > - More clear entry.S with handle_exception and ret_from_exception
> >> > - Get rid of complex custom signal implementation
> >> > - Move syscall procedure from assembly to C, which is much more
> >> >   readable.
> >> > - Connect ret_from_fork & ret_from_kernel_thread to generic entry.
> >> > - Wrap with irqentry_enter/exit and syscall_enter/exit_from_user_mode
> >> > - Use the standard preemption code instead of custom
> >> >
> >> >Suggested-by: Huacai Chen <chenhuacai@xxxxxxxxxx>
> >> >Reviewed-by: Björn Töpel <bjorn@xxxxxxxxxxxx>
> >> >Tested-by: Yipeng Zou <zouyipeng@xxxxxxxxxx>
> >> >Tested-by: Jisheng Zhang <jszhang@xxxxxxxxxx>
> >> >Signed-off-by: Guo Ren <guoren@xxxxxxxxxxxxxxxxx>
> >> >Signed-off-by: Guo Ren <guoren@xxxxxxxxxx>
> >> >Cc: Ben Hutchings <ben@xxxxxxxxxxxxxxx>
> >> >---
> >>
> >> Got some new errors added by this patch:
> >> https://gist.github.com/conor-pwbot/3b300050a7a4a197bca809935584d809
> >>
> >> Unfortunately I'm away from a computer at FOSDEM, so I haven't done any investigation
> >> of the warnings.
> >> Should be reproduceable with gcc-12 allmodconfig.
> >Thx for report, but:
> >The spin_shadow_stack is from '7e1864332fbc ("riscv: fix race when
> >vmap stack overflow")'. Not this patch.
> >
> >New errors added:
> >--- /tmp/tmp.nyMxgc6CGx 2023-02-05 05:12:59.949595120 +0000
> >+++ /tmp/tmp.5td5fIdaHX 2023-02-05 05:12:59.961595119 +0000
> >@@ -10 +10 @@
> >- 1 ../arch/riscv/kernel/traps.c:231:15: warning: symbol
> >'spin_shadow_stack' was not declared. Should it be static?
> >+ 1 ../arch/riscv/kernel/traps.c:335:15: warning: symbol
> >'spin_shadow_stack' was not declared. Should it be static?
> >@@ -9109 +9109 @@
> >- 37 ../include/linux/fortify-string.h:522:25: warning: call to
> >'__read_overflow2_field' declared with attribute warning: detected
> >read beyond size of field (2nd parameter); maybe use struct_group()?
> >[-Wattribute-warning]
> >+ 38 ../include/linux/fortify-string.h:522:25: warning: call to
> >'__read_overflow2_field' declared with attribute warning: detected
> >read beyond size of field (2nd parameter); maybe use struct_group()?
> >[-Wattribute-warning]
> >Per-file breakdown
> >--- /tmp/tmp.bHiHUVMzmZ 2023-02-05 05:13:00.109595117 +0000
> >+++ /tmp/tmp.kUkOd6TrGj 2023-02-05 05:13:00.257595114 +0000
> >@@ -1197 +1197 @@
> >- 65 ../include/linux/fortify-string.h
> >+ 66 ../include/linux/fortify-string.h
> >
> >Seems the line number change would cause your script to report old
> >errors as new. So it would be best to improve the check script, such
> >as ignoring the first column line number :)
> 
> I thought it already did!
> I might've messed up in a refactoring of the script.
> I'll fix it up when I get home so, sorry for the noise!

So I finally got around to trying to sort this out.

>- 1 ../arch/riscv/kernel/traps.c:231:15: warning: symbol
>'spin_shadow_stack' was not declared. Should it be static?
>+ 1 ../arch/riscv/kernel/traps.c:335:15: warning: symbol
>'spin_shadow_stack' was not declared. Should it be static?

As you pointed out, this one is just the movement of an existing error
but isn't why the automation complained about the patch.
That said, should probably be fixed by declaring it in thread-info.h
alongside shadow_stack?

>- 37 ../include/linux/fortify-string.h:522:25: warning: call to
>'__read_overflow2_field' declared with attribute warning: detected
>read beyond size of field (2nd parameter); maybe use struct_group()?
>[-Wattribute-warning]
>+ 38 ../include/linux/fortify-string.h:522:25: warning: call to
>'__read_overflow2_field' declared with attribute warning: detected
>read beyond size of field (2nd parameter); maybe use struct_group()?
>[-Wattribute-warning]

The 37 and 38 here is the source of the complaint though, this series
added an extra one of these warnings, so I don't think the automation
has done anything wrong here.

The number comes from the output of:
grep "\(warning\|error\):" $tmpfile_n | sort | uniq -c > $tmpfile_errors_now

And that appears to be correctly reflected in the report:
build_rv64_gcc_allmodconfig	fail	Errors and warnings before: 17343 this patch: 17344

However, I should probably go and do something to display the LoC that
caused the issue, since knowing it came from fortify-string.h doesn't do
all that much to help you know which change caused it to appear.

Thanks,
Conor.

Attachment: signature.asc
Description: PGP signature