| Message ID | 20211123124348.31073-2-rpalethorpe@suse.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | tools/sparse: Add static check | expand |
Hi Richie, > This was adapted from Sparse's inbuilt check_duplicates (-Wdecl). The > original check appears to print a warning whenever a symbol is > non-static, but has no prototype. It appears to work because library > symbols are usually declared first in a header file and then again > with their definition in a source file. > The LTP version also checks for the various library prefixes, but > should otherwise be the same. LGTM, thanks! Kind regards, Petr
Hello, Richard Palethorpe <rpalethorpe@suse.com> writes: > This was adapted from Sparse's inbuilt check_duplicates (-Wdecl). The > original check appears to print a warning whenever a symbol is > non-static, but has no prototype. It appears to work because library > symbols are usually declared first in a header file and then again > with their definition in a source file. > > The LTP version also checks for the various library prefixes, but > should otherwise be the same. > > Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com> > --- > tools/sparse/sparse-ltp.c | 53 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 53 insertions(+) > > diff --git a/tools/sparse/sparse-ltp.c b/tools/sparse/sparse-ltp.c > index 45874e8eb..73725d191 100644 > --- a/tools/sparse/sparse-ltp.c > +++ b/tools/sparse/sparse-ltp.c > @@ -82,6 +82,57 @@ static void do_entrypoint_checks(struct entrypoint *ep) > } END_FOR_EACH_PTR(bb); > } > > +/* Check for LTP-003 and LTP-004 > + * > + * Try to find cases where the static keyword was forgotten. > + */ > +static void check_symbol_visibility(struct symbol *sym) > +{ > + struct symbol *next = sym; Ooops, we don't need next anymore.
Hi!
Looks good to me as well.
Acked-by: Cyril Hrubis <chrubis@suse.cz>
Hello Petr and Cyril, Thanks! pushed. Please pull and try it out. Petr Vorel <pvorel@suse.cz> writes: > Hi Richie, > >> This was adapted from Sparse's inbuilt check_duplicates (-Wdecl). The >> original check appears to print a warning whenever a symbol is >> non-static, but has no prototype. It appears to work because library >> symbols are usually declared first in a header file and then again >> with their definition in a source file. > >> The LTP version also checks for the various library prefixes, but >> should otherwise be the same. > LGTM, thanks! I took the liberty (;-)) of interpreting this as Acked-by. > > Kind regards, > Petr
Hi!
> Thanks! pushed. Please pull and try it out.
Looks like it fails on fuzzy sync since it uses tst_ but it's in an
header.
These definitions should be static inline and changing them so fixes the
warnings. It looks like static inline functions does not make it into
the symbol test at all.
Hi! > > Thanks! pushed. Please pull and try it out. > > Looks like it fails on fuzzy sync since it uses tst_ but it's in an > header. > > These definitions should be static inline and changing them so fixes the > warnings. It looks like static inline functions does not make it into > the symbol test at all. This is even stranger, the 'static inline void' functions does not make it into the check function, but anything that returns a non-void value does get there so we need: diff --git a/tools/sparse/sparse-ltp.c b/tools/sparse/sparse-ltp.c index 2f32bfa38..b1677d336 100644 --- a/tools/sparse/sparse-ltp.c +++ b/tools/sparse/sparse-ltp.c @@ -98,7 +98,7 @@ static void check_symbol_visibility(const struct symbol *const sym) if (!(mod & MOD_TOPLEVEL)) return; - if (has_lib_prefix && (mod & MOD_STATIC)) { + if (has_lib_prefix && (mod & MOD_STATIC) && !(mod & MOD_INLINE)) { warning(sym->pos, "LTP-003: Symbol '%s' has the LTP public library prefix, but is static (private).", name); And: diff --git a/include/tst_fuzzy_sync.h b/include/tst_fuzzy_sync.h index 8f97bb8f6..bc3450294 100644 --- a/include/tst_fuzzy_sync.h +++ b/include/tst_fuzzy_sync.h @@ -210,7 +210,7 @@ struct tst_fzsync_pair { * * @sa tst_fzsync_pair_reset() */ -static void tst_fzsync_pair_init(struct tst_fzsync_pair *pair) +static inline void tst_fzsync_pair_init(struct tst_fzsync_pair *pair) { CHK(avg_alpha, 0, 1, 0.25); CHK(min_samples, 20, INT_MAX, 1024); @@ -230,7 +230,7 @@ static void tst_fzsync_pair_init(struct tst_fzsync_pair *pair) * * Call this from your cleanup function. */ -static void tst_fzsync_pair_cleanup(struct tst_fzsync_pair *pair) +static inline void tst_fzsync_pair_cleanup(struct tst_fzsync_pair *pair) { if (pair->thread_b) { /* Revoke thread B if parent hits accidental break */ @@ -254,7 +254,7 @@ struct tst_fzsync_run_thread { * Wrap run_b for tst_fzsync_pair_reset to enable pthread cancel * at the start of the thread B. */ -static void *tst_fzsync_thread_wrapper(void *run_thread) +static inline void *tst_fzsync_thread_wrapper(void *run_thread) { struct tst_fzsync_run_thread t = *(struct tst_fzsync_run_thread *)run_thread; @@ -268,7 +268,7 @@ static void *tst_fzsync_thread_wrapper(void *run_thread) * * @relates tst_fzsync_stat */ -static void tst_init_stat(struct tst_fzsync_stat *s) +static inline void tst_init_stat(struct tst_fzsync_stat *s) { s->avg = 0; s->avg_dev = 0; @@ -292,7 +292,7 @@ static void tst_init_stat(struct tst_fzsync_stat *s) * * @sa tst_fzsync_pair_init() */ -static void tst_fzsync_pair_reset(struct tst_fzsync_pair *pair, +static inline void tst_fzsync_pair_reset(struct tst_fzsync_pair *pair, void *(*run_b)(void *)) { tst_fzsync_pair_cleanup(pair); @@ -340,7 +340,7 @@ static inline void tst_fzsync_stat_info(struct tst_fzsync_stat stat, * * @relates tst_fzsync_pair */ -static void tst_fzsync_pair_info(struct tst_fzsync_pair *pair) +static inline void tst_fzsync_pair_info(struct tst_fzsync_pair *pair) { tst_res(TINFO, "loop = %d, delay_bias = %d", pair->exec_loop, pair->delay_bias); @@ -493,7 +493,7 @@ static inline void tst_upd_diff_stat(struct tst_fzsync_stat *s, * * @relates tst_fzsync_pair */ -static void tst_fzsync_pair_update(struct tst_fzsync_pair *pair) +static inline void tst_fzsync_pair_update(struct tst_fzsync_pair *pair) { float alpha = pair->avg_alpha; float per_spin_time, time_delay;
Hello, Cyril Hrubis <chrubis@suse.cz> writes: > Hi! >> > Thanks! pushed. Please pull and try it out. >> >> Looks like it fails on fuzzy sync since it uses tst_ but it's in an >> header. >> >> These definitions should be static inline and changing them so fixes the >> warnings. It looks like static inline functions does not make it into >> the symbol test at all. Ah, I vaguely remember now that "static inline" is the correct way to include functions in header files. Otherwise they shouldn't be in header files. We have LTO now as well so possibly Fuzzy sync shouldn't be all in the header, but that's another matter. > > This is even stranger, the 'static inline void' functions does not make > it into the check function, but anything that returns a non-void value > does get there so we need: I have no idea... > > diff --git a/tools/sparse/sparse-ltp.c b/tools/sparse/sparse-ltp.c > index 2f32bfa38..b1677d336 100644 > --- a/tools/sparse/sparse-ltp.c > +++ b/tools/sparse/sparse-ltp.c > @@ -98,7 +98,7 @@ static void check_symbol_visibility(const struct symbol *const sym) > if (!(mod & MOD_TOPLEVEL)) > return; > > - if (has_lib_prefix && (mod & MOD_STATIC)) { > + if (has_lib_prefix && (mod & MOD_STATIC) && !(mod & MOD_INLINE)) { > warning(sym->pos, > "LTP-003: Symbol '%s' has the LTP public library prefix, but is static (private).", > name); > OK, this loooks good and below. Could you send a patch? Also can add Reviewed-by: Richard Palethorpe <rpalethorpe@suse.com> > And: > > diff --git a/include/tst_fuzzy_sync.h b/include/tst_fuzzy_sync.h > index 8f97bb8f6..bc3450294 100644 > --- a/include/tst_fuzzy_sync.h > +++ b/include/tst_fuzzy_sync.h > @@ -210,7 +210,7 @@ struct tst_fzsync_pair { > * > * @sa tst_fzsync_pair_reset() > */ > -static void tst_fzsync_pair_init(struct tst_fzsync_pair *pair) > +static inline void tst_fzsync_pair_init(struct tst_fzsync_pair *pair) > { > CHK(avg_alpha, 0, 1, 0.25); > CHK(min_samples, 20, INT_MAX, 1024); > @@ -230,7 +230,7 @@ static void tst_fzsync_pair_init(struct tst_fzsync_pair *pair) > * > * Call this from your cleanup function. > */ > -static void tst_fzsync_pair_cleanup(struct tst_fzsync_pair *pair) > +static inline void tst_fzsync_pair_cleanup(struct tst_fzsync_pair *pair) > { > if (pair->thread_b) { > /* Revoke thread B if parent hits accidental break */ > @@ -254,7 +254,7 @@ struct tst_fzsync_run_thread { > * Wrap run_b for tst_fzsync_pair_reset to enable pthread cancel > * at the start of the thread B. > */ > -static void *tst_fzsync_thread_wrapper(void *run_thread) > +static inline void *tst_fzsync_thread_wrapper(void *run_thread) > { > struct tst_fzsync_run_thread t = *(struct tst_fzsync_run_thread *)run_thread; > > @@ -268,7 +268,7 @@ static void *tst_fzsync_thread_wrapper(void *run_thread) > * > * @relates tst_fzsync_stat > */ > -static void tst_init_stat(struct tst_fzsync_stat *s) > +static inline void tst_init_stat(struct tst_fzsync_stat *s) > { > s->avg = 0; > s->avg_dev = 0; > @@ -292,7 +292,7 @@ static void tst_init_stat(struct tst_fzsync_stat *s) > * > * @sa tst_fzsync_pair_init() > */ > -static void tst_fzsync_pair_reset(struct tst_fzsync_pair *pair, > +static inline void tst_fzsync_pair_reset(struct tst_fzsync_pair *pair, > void *(*run_b)(void *)) > { > tst_fzsync_pair_cleanup(pair); > @@ -340,7 +340,7 @@ static inline void tst_fzsync_stat_info(struct tst_fzsync_stat stat, > * > * @relates tst_fzsync_pair > */ > -static void tst_fzsync_pair_info(struct tst_fzsync_pair *pair) > +static inline void tst_fzsync_pair_info(struct tst_fzsync_pair *pair) > { > tst_res(TINFO, "loop = %d, delay_bias = %d", > pair->exec_loop, pair->delay_bias); > @@ -493,7 +493,7 @@ static inline void tst_upd_diff_stat(struct tst_fzsync_stat *s, > * > * @relates tst_fzsync_pair > */ > -static void tst_fzsync_pair_update(struct tst_fzsync_pair *pair) > +static inline void tst_fzsync_pair_update(struct tst_fzsync_pair *pair) > { > float alpha = pair->avg_alpha; > float per_spin_time, time_delay;
Hi! > >> > Thanks! pushed. Please pull and try it out. > >> > >> Looks like it fails on fuzzy sync since it uses tst_ but it's in an > >> header. > >> > >> These definitions should be static inline and changing them so fixes the > >> warnings. It looks like static inline functions does not make it into > >> the symbol test at all. > > Ah, I vaguely remember now that "static inline" is the correct way to > include functions in header files. Otherwise they shouldn't be in header > files. We have LTO now as well so possibly Fuzzy sync shouldn't be all > in the header, but that's another matter. If I recall correctly LTO is currently broken on ARM and even if that was working we still have to support 10 years old compilers anyways...
Hi!
I guess that we can also silence some of the useless output by skipping
some old library crap as:
diff --git a/tools/sparse/sparse-ltp.c b/tools/sparse/sparse-ltp.c
index b1677d336..1a3b4089a 100644
--- a/tools/sparse/sparse-ltp.c
+++ b/tools/sparse/sparse-ltp.c
@@ -98,6 +98,9 @@ static void check_symbol_visibility(const struct symbol *const sym)
if (!(mod & MOD_TOPLEVEL))
return;
+ if (!strcmp(name, "TCID") || !strcmp(name, "TST_TOTAL"))
+ return;
+
if (has_lib_prefix && (mod & MOD_STATIC) && !(mod & MOD_INLINE)) {
warning(sym->pos,
"LTP-003: Symbol '%s' has the LTP public library prefix, but is static (private).",
Will send another patch.
diff --git a/tools/sparse/sparse-ltp.c b/tools/sparse/sparse-ltp.c index 45874e8eb..73725d191 100644 --- a/tools/sparse/sparse-ltp.c +++ b/tools/sparse/sparse-ltp.c @@ -82,6 +82,57 @@ static void do_entrypoint_checks(struct entrypoint *ep) } END_FOR_EACH_PTR(bb); } +/* Check for LTP-003 and LTP-004 + * + * Try to find cases where the static keyword was forgotten. + */ +static void check_symbol_visibility(struct symbol *sym) +{ + struct symbol *next = sym; + unsigned long mod = sym->ctype.modifiers; + const char *name = show_ident(sym->ident); + int has_lib_prefix = !strncmp("tst_", name, 4) || + !strncmp("TST_", name, 4) || + !strncmp("ltp_", name, 4) || + !strncmp("safe_", name, 5); + + if (!(mod & MOD_TOPLEVEL)) + return; + + if (has_lib_prefix && (mod & MOD_STATIC)) { + warning(sym->pos, + "LTP-003: Symbol '%s' has the LTP public library prefix, but is static (private).", + name); + return; + } + + if ((mod & MOD_STATIC)) + return; + + if (tu_kind == LTP_LIB && !has_lib_prefix) { + warning(sym->pos, + "LTP-003: Symbol '%s' is a public library function, but is missing the 'tst_' prefix", + name); + return; + } + + if (next->same_symbol) + return; + + if (sym->ident == &main_ident) + return; + + warning(sym->pos, + "Symbol '%s' has no prototype or library ('tst_') prefix. Should it be static?", + name); +} + +/* AST level checks */ +static void do_symbol_checks(struct symbol *sym) +{ + check_symbol_visibility(sym); +} + /* Compile the AST into a graph of basicblocks */ static void process_symbols(struct symbol_list *list) { @@ -90,6 +141,8 @@ static void process_symbols(struct symbol_list *list) FOR_EACH_PTR(list, sym) { struct entrypoint *ep; + do_symbol_checks(sym); + expand_symbol(sym); ep = linearize_symbol(sym); if (!ep || !ep->entry)
This was adapted from Sparse's inbuilt check_duplicates (-Wdecl). The original check appears to print a warning whenever a symbol is non-static, but has no prototype. It appears to work because library symbols are usually declared first in a header file and then again with their definition in a source file. The LTP version also checks for the various library prefixes, but should otherwise be the same. Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com> --- tools/sparse/sparse-ltp.c | 53 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)