| Message ID | 20200202170052.14012-1-git@xen0n.name (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [1/2] MIPS: loongson3-llsc-check: Downgrade failures to notices | expand |
Hi Wang & Huacai, On Mon, Feb 03, 2020 at 01:00:51AM +0800, Wang Xuerui wrote: > From: Huacai Chen <chenhc@lemote.com> > > Local ops (and other similar cases) don't need SYNCs before LL/SC > because there is only one writer, so don't always fail on missing SYNCs. > Print a notice instead. > > [git@xen0n.name: Massaged commit message and symbol naming.] I really dislike this patch; the whole point of the compile-time check is to make problems fatal & easy to find. By turning them into mere warnings you make it possible to ignore real problems, and by making some of the warnings expected with patch 2 you make it much more likely that real issues will be ignored. I'd suggest that if you want to remove the SYNCs in asm/local.h then you could come up with some way to annotate those places in a way that the loongson3-llsc-check tool can detect & avoid erroring on. That way real bugs will still be found, presuming the annotations are used correctly. For example, you could come up with a macro to use instead of __SYNC() that generates an ELF section listing the PC values of locations that don't require the sync. The checker can take those into account, and we can remove the section from the final VDSO image. As it is, I'd much rather have asm/local.h incur the slight overhead from extra sync instructions than make the checker useless. Thanks, Paul > Signed-off-by: Huacai Chen <chenhc@lemote.com> > Signed-off-by: Wang Xuerui <git@xen0n.name> > --- > arch/mips/tools/loongson3-llsc-check.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/arch/mips/tools/loongson3-llsc-check.c b/arch/mips/tools/loongson3-llsc-check.c > index 0ebddd0ae46f..c485950b7a36 100644 > --- a/arch/mips/tools/loongson3-llsc-check.c > +++ b/arch/mips/tools/loongson3-llsc-check.c > @@ -138,6 +138,12 @@ static bool is_branch(uint32_t insn, int *off) > } > } > > +#define REPORT_OK 0x0 > +#define REPORT_LL 0x1 > +#define REPORT_BRANCH_TARGET 0x2 > + > +static int err_report = REPORT_OK; > + > static int check_ll(uint64_t pc, uint32_t *code, size_t sz) > { > ssize_t i, max, sc_pos; > @@ -149,8 +155,8 @@ static int check_ll(uint64_t pc, uint32_t *code, size_t sz) > * execute after the LL & cause erroneous results. > */ > if (!is_sync(le32toh(code[-1]))) { > + err_report |= REPORT_LL; > fprintf(stderr, "%" PRIx64 ": LL not preceded by sync\n", pc); > - return -EINVAL; > } > > /* Find the matching SC instruction */ > @@ -185,9 +191,9 @@ static int check_ll(uint64_t pc, uint32_t *code, size_t sz) > continue; > > /* ...but if not, we have a problem */ > + err_report |= REPORT_BRANCH_TARGET; > fprintf(stderr, "%" PRIx64 ": Branch target not a sync\n", > pc + (i * 4)); > - return -EINVAL; > } > > return 0; > @@ -297,6 +303,13 @@ int main(int argc, char *argv[]) > goto out_munmap; > } > > + if (err_report & REPORT_LL) > + fprintf(stderr, "Notice: there are LLs not preceded by" > + " syncs, please confirm manually.\n"); > + if (err_report & REPORT_BRANCH_TARGET) > + fprintf(stderr, "Notice: there are branches within LL/SC blocks" > + " not targeting syncs, please confirm manually.\n"); > + > status = EXIT_SUCCESS; > out_munmap: > munmap(vmlinux, st.st_size); > -- > 2.21.0 >
diff --git a/arch/mips/tools/loongson3-llsc-check.c b/arch/mips/tools/loongson3-llsc-check.c index 0ebddd0ae46f..c485950b7a36 100644 --- a/arch/mips/tools/loongson3-llsc-check.c +++ b/arch/mips/tools/loongson3-llsc-check.c @@ -138,6 +138,12 @@ static bool is_branch(uint32_t insn, int *off) } } +#define REPORT_OK 0x0 +#define REPORT_LL 0x1 +#define REPORT_BRANCH_TARGET 0x2 + +static int err_report = REPORT_OK; + static int check_ll(uint64_t pc, uint32_t *code, size_t sz) { ssize_t i, max, sc_pos; @@ -149,8 +155,8 @@ static int check_ll(uint64_t pc, uint32_t *code, size_t sz) * execute after the LL & cause erroneous results. */ if (!is_sync(le32toh(code[-1]))) { + err_report |= REPORT_LL; fprintf(stderr, "%" PRIx64 ": LL not preceded by sync\n", pc); - return -EINVAL; } /* Find the matching SC instruction */ @@ -185,9 +191,9 @@ static int check_ll(uint64_t pc, uint32_t *code, size_t sz) continue; /* ...but if not, we have a problem */ + err_report |= REPORT_BRANCH_TARGET; fprintf(stderr, "%" PRIx64 ": Branch target not a sync\n", pc + (i * 4)); - return -EINVAL; } return 0; @@ -297,6 +303,13 @@ int main(int argc, char *argv[]) goto out_munmap; } + if (err_report & REPORT_LL) + fprintf(stderr, "Notice: there are LLs not preceded by" + " syncs, please confirm manually.\n"); + if (err_report & REPORT_BRANCH_TARGET) + fprintf(stderr, "Notice: there are branches within LL/SC blocks" + " not targeting syncs, please confirm manually.\n"); + status = EXIT_SUCCESS; out_munmap: munmap(vmlinux, st.st_size);