* Re: [PATCH] perf: fix -Wstring-compare
2020-02-23 19:34 [PATCH] perf: fix -Wstring-compare Nick Desaulniers
@ 2020-02-24 5:55 ` Ian Rogers
2020-02-24 16:03 ` David Laight
2020-03-07 7:36 ` [tip: perf/urgent] perf diff: Fix undefined string comparision spotted by clang's -Wstring-compare tip-bot2 for Nick Desaulniers
2020-03-19 14:10 ` [tip: perf/core] perf diff: Fix undefined string comparison " tip-bot2 for Nick Desaulniers
2 siblings, 1 reply; 10+ messages in thread
From: Ian Rogers @ 2020-02-24 5:55 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Arnaldo Carvalho de Melo, Ingo Molnar, Peter Zijlstra,
clang-built-linux, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, Jin Yao, Changbin Du, John Keeping, Song Liu, LKML
On Sun, Feb 23, 2020 at 11:35 AM Nick Desaulniers
<nick.desaulniers@gmail.com> wrote:
>
> Clang warns:
>
> util/block-info.c:298:18: error: result of comparison against a string
> literal is unspecified (use an explicit string comparison function
> instead) [-Werror,-Wstring-compare]
> if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
> ^ ~~~~~~~~~~~~~~~
> util/block-info.c:298:51: error: result of comparison against a string
> literal is unspecified (use an explicit string comparison function
> instead) [-Werror,-Wstring-compare]
> if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
> ^ ~~~~~~~~~~~~~~~
> util/block-info.c:298:18: error: result of comparison against a string
> literal is unspecified (use an explicit string
> comparison function instead) [-Werror,-Wstring-compare]
> if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
> ^ ~~~~~~~~~~~~~~~
> util/block-info.c:298:51: error: result of comparison against a string
> literal is unspecified (use an explicit string comparison function
> instead) [-Werror,-Wstring-compare]
> if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
> ^ ~~~~~~~~~~~~~~~
> util/map.c:434:15: error: result of comparison against a string literal
> is unspecified (use an explicit string comparison function instead)
> [-Werror,-Wstring-compare]
> if (srcline != SRCLINE_UNKNOWN)
> ^ ~~~~~~~~~~~~~~~
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/900
> Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
> ---
> Note: was generated off of mainline; can rebase on -next if it doesn't
> apply cleanly.
Looks good to me. Some more context:
https://clang.llvm.org/docs/DiagnosticsReference.html#wstring-compare
The spec says:
J.1 Unspecified behavior
The following are unspecified:
.. Whether two string literals result in distinct arrays (6.4.5).
> tools/perf/builtin-diff.c | 3 ++-
> tools/perf/util/block-info.c | 3 ++-
> tools/perf/util/map.c | 2 +-
> 3 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
> index f8b6ae557d8b..c03c36fde7e2 100644
> --- a/tools/perf/builtin-diff.c
> +++ b/tools/perf/builtin-diff.c
> @@ -1312,7 +1312,8 @@ static int cycles_printf(struct hist_entry *he, struct hist_entry *pair,
> end_line = map__srcline(he->ms.map, bi->sym->start + bi->end,
> he->ms.sym);
>
> - if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
> + if ((strncmp(start_line, SRCLINE_UNKNOWN, strlen(SRCLINE_UNKNOWN)) != 0) &&
> + (strncmp(end_line, SRCLINE_UNKNOWN, strlen(SRCLINE_UNKNOWN)) != 0)) {
> scnprintf(buf, sizeof(buf), "[%s -> %s] %4ld",
> start_line, end_line, block_he->diff.cycles);
> } else {
> diff --git a/tools/perf/util/block-info.c b/tools/perf/util/block-info.c
> index c4b030bf6ec2..fbbb6d640dad 100644
> --- a/tools/perf/util/block-info.c
> +++ b/tools/perf/util/block-info.c
> @@ -295,7 +295,8 @@ static int block_range_entry(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
> end_line = map__srcline(he->ms.map, bi->sym->start + bi->end,
> he->ms.sym);
>
> - if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
> + if ((strncmp(start_line, SRCLINE_UNKNOWN, strlen(SRCLINE_UNKNOWN)) != 0) &&
> + (strncmp(end_line, SRCLINE_UNKNOWN, strlen(SRCLINE_UNKNOWN)) != 0)) {
> scnprintf(buf, sizeof(buf), "[%s -> %s]",
> start_line, end_line);
> } else {
> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> index a08ca276098e..95428511300d 100644
> --- a/tools/perf/util/map.c
> +++ b/tools/perf/util/map.c
> @@ -431,7 +431,7 @@ int map__fprintf_srcline(struct map *map, u64 addr, const char *prefix,
>
> if (map && map->dso) {
> char *srcline = map__srcline(map, addr, NULL);
> - if (srcline != SRCLINE_UNKNOWN)
> + if (strncmp(srcline, SRCLINE_UNKNOWN, strlen(SRCLINE_UNKNOWN)) != 0)
> ret = fprintf(fp, "%s%s", prefix, srcline);
> free_srcline(srcline);
> }
> --
> 2.17.1
>
> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20200223193456.25291-1-nick.desaulniers%40gmail.com.
^ permalink raw reply [flat|nested] 10+ messages in thread* RE: [PATCH] perf: fix -Wstring-compare
2020-02-24 5:55 ` Ian Rogers
@ 2020-02-24 16:03 ` David Laight
2020-02-24 18:19 ` Ian Rogers
0 siblings, 1 reply; 10+ messages in thread
From: David Laight @ 2020-02-24 16:03 UTC (permalink / raw)
To: 'Ian Rogers', Nick Desaulniers
Cc: Arnaldo Carvalho de Melo, Ingo Molnar, Peter Zijlstra,
clang-built-linux, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, Jin Yao, Changbin Du, John Keeping, Song Liu, LKML
From: Ian Rogers
> Sent: 24 February 2020 05:56
> On Sun, Feb 23, 2020 at 11:35 AM Nick Desaulniers
> <nick.desaulniers@gmail.com> wrote:
> >
> > Clang warns:
> >
> > util/block-info.c:298:18: error: result of comparison against a string
> > literal is unspecified (use an explicit string comparison function
> > instead) [-Werror,-Wstring-compare]
> > if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
> > ^ ~~~~~~~~~~~~~~~
> > util/block-info.c:298:51: error: result of comparison against a string
> > literal is unspecified (use an explicit string comparison function
> > instead) [-Werror,-Wstring-compare]
> > if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
> > ^ ~~~~~~~~~~~~~~~
> > util/block-info.c:298:18: error: result of comparison against a string
> > literal is unspecified (use an explicit string
> > comparison function instead) [-Werror,-Wstring-compare]
> > if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
> > ^ ~~~~~~~~~~~~~~~
> > util/block-info.c:298:51: error: result of comparison against a string
> > literal is unspecified (use an explicit string comparison function
> > instead) [-Werror,-Wstring-compare]
> > if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
> > ^ ~~~~~~~~~~~~~~~
> > util/map.c:434:15: error: result of comparison against a string literal
> > is unspecified (use an explicit string comparison function instead)
> > [-Werror,-Wstring-compare]
> > if (srcline != SRCLINE_UNKNOWN)
> > ^ ~~~~~~~~~~~~~~~
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/900
> > Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
> > ---
> > Note: was generated off of mainline; can rebase on -next if it doesn't
> > apply cleanly.
>
> Looks good to me. Some more context:
> https://clang.llvm.org/docs/DiagnosticsReference.html#wstring-compare
> The spec says:
> J.1 Unspecified behavior
> The following are unspecified:
> .. Whether two string literals result in distinct arrays (6.4.5).
Just change the (probable):
#define SRCLINE_UNKNOWN "unknown"
with
static const char SRC_LINE_UNKNOWN[] = "unk";
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] perf: fix -Wstring-compare
2020-02-24 16:03 ` David Laight
@ 2020-02-24 18:19 ` Ian Rogers
2020-02-24 22:05 ` Nick Desaulniers
0 siblings, 1 reply; 10+ messages in thread
From: Ian Rogers @ 2020-02-24 18:19 UTC (permalink / raw)
To: David Laight
Cc: Nick Desaulniers, Arnaldo Carvalho de Melo, Ingo Molnar,
Peter Zijlstra, clang-built-linux, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Jin Yao, Changbin Du,
John Keeping, Song Liu, LKML
On Mon, Feb 24, 2020 at 8:03 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: Ian Rogers
> > Sent: 24 February 2020 05:56
> > On Sun, Feb 23, 2020 at 11:35 AM Nick Desaulniers
> > <nick.desaulniers@gmail.com> wrote:
> > >
> > > Clang warns:
> > >
> > > util/block-info.c:298:18: error: result of comparison against a string
> > > literal is unspecified (use an explicit string comparison function
> > > instead) [-Werror,-Wstring-compare]
> > > if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
> > > ^ ~~~~~~~~~~~~~~~
> > > util/block-info.c:298:51: error: result of comparison against a string
> > > literal is unspecified (use an explicit string comparison function
> > > instead) [-Werror,-Wstring-compare]
> > > if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
> > > ^ ~~~~~~~~~~~~~~~
> > > util/block-info.c:298:18: error: result of comparison against a string
> > > literal is unspecified (use an explicit string
> > > comparison function instead) [-Werror,-Wstring-compare]
> > > if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
> > > ^ ~~~~~~~~~~~~~~~
> > > util/block-info.c:298:51: error: result of comparison against a string
> > > literal is unspecified (use an explicit string comparison function
> > > instead) [-Werror,-Wstring-compare]
> > > if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
> > > ^ ~~~~~~~~~~~~~~~
> > > util/map.c:434:15: error: result of comparison against a string literal
> > > is unspecified (use an explicit string comparison function instead)
> > > [-Werror,-Wstring-compare]
> > > if (srcline != SRCLINE_UNKNOWN)
> > > ^ ~~~~~~~~~~~~~~~
> > >
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/900
> > > Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
> > > ---
> > > Note: was generated off of mainline; can rebase on -next if it doesn't
> > > apply cleanly.
Reviewed-by: Ian Rogers <irogers@google.com>
> > Looks good to me. Some more context:
> > https://clang.llvm.org/docs/DiagnosticsReference.html#wstring-compare
> > The spec says:
> > J.1 Unspecified behavior
> > The following are unspecified:
> > .. Whether two string literals result in distinct arrays (6.4.5).
>
> Just change the (probable):
> #define SRCLINE_UNKNOWN "unknown"
> with
> static const char SRC_LINE_UNKNOWN[] = "unk";
>
> David
The SRCLINE_UNKNOWN is used to convey information. Having multiple
distinct pointers (static) would mean the compiler could likely remove
all comparisons as the compiler could prove that pointer is never
returned by a function - ie comparisons are either known to be true
(!=) or false (==).
Thanks,
Ian
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] perf: fix -Wstring-compare
2020-02-24 18:19 ` Ian Rogers
@ 2020-02-24 22:05 ` Nick Desaulniers
2020-02-25 9:35 ` David Laight
0 siblings, 1 reply; 10+ messages in thread
From: Nick Desaulniers @ 2020-02-24 22:05 UTC (permalink / raw)
To: Ian Rogers, David Laight, Arnaldo Carvalho de Melo
Cc: Nick Desaulniers, Ingo Molnar, Peter Zijlstra, clang-built-linux,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Jin Yao, Changbin Du, John Keeping, Song Liu, LKML
On Mon, Feb 24, 2020 at 10:20 AM 'Ian Rogers' via Clang Built Linux
<clang-built-linux@googlegroups.com> wrote:
>
> On Mon, Feb 24, 2020 at 8:03 AM David Laight <David.Laight@aculab.com> wrote:
> >
> > From: Ian Rogers
> > > Sent: 24 February 2020 05:56
> > > On Sun, Feb 23, 2020 at 11:35 AM Nick Desaulniers
> > > <nick.desaulniers@gmail.com> wrote:
> > > >
> > > > Clang warns:
> > > >
> > > > util/block-info.c:298:18: error: result of comparison against a string
> > > > literal is unspecified (use an explicit string comparison function
> > > > instead) [-Werror,-Wstring-compare]
> > > > if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
> > > > ^ ~~~~~~~~~~~~~~~
> > > > util/block-info.c:298:51: error: result of comparison against a string
> > > > literal is unspecified (use an explicit string comparison function
> > > > instead) [-Werror,-Wstring-compare]
> > > > if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
> > > > ^ ~~~~~~~~~~~~~~~
> > > > util/block-info.c:298:18: error: result of comparison against a string
> > > > literal is unspecified (use an explicit string
> > > > comparison function instead) [-Werror,-Wstring-compare]
> > > > if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
> > > > ^ ~~~~~~~~~~~~~~~
> > > > util/block-info.c:298:51: error: result of comparison against a string
> > > > literal is unspecified (use an explicit string comparison function
> > > > instead) [-Werror,-Wstring-compare]
> > > > if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
> > > > ^ ~~~~~~~~~~~~~~~
> > > > util/map.c:434:15: error: result of comparison against a string literal
> > > > is unspecified (use an explicit string comparison function instead)
> > > > [-Werror,-Wstring-compare]
> > > > if (srcline != SRCLINE_UNKNOWN)
> > > > ^ ~~~~~~~~~~~~~~~
> > > >
> > > > Link: https://github.com/ClangBuiltLinux/linux/issues/900
> > > > Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
> > > > ---
> > > > Note: was generated off of mainline; can rebase on -next if it doesn't
> > > > apply cleanly.
>
> Reviewed-by: Ian Rogers <irogers@google.com>
>
> > > Looks good to me. Some more context:
> > > https://clang.llvm.org/docs/DiagnosticsReference.html#wstring-compare
> > > The spec says:
> > > J.1 Unspecified behavior
> > > The following are unspecified:
> > > .. Whether two string literals result in distinct arrays (6.4.5).
> >
> > Just change the (probable):
> > #define SRCLINE_UNKNOWN "unknown"
> > with
> > static const char SRC_LINE_UNKNOWN[] = "unk";
> >
> > David
>
>
> The SRCLINE_UNKNOWN is used to convey information. Having multiple
> distinct pointers (static) would mean the compiler could likely remove
> all comparisons as the compiler could prove that pointer is never
> returned by a function - ie comparisons are either known to be true
> (!=) or false (==).
I wouldn't define a static string in a header. Though I could:
1. forward declare it in the header with extern linkage.
2. define it in *one* .c source file.
Thoughts on that vs the current approach?
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 10+ messages in thread* RE: [PATCH] perf: fix -Wstring-compare
2020-02-24 22:05 ` Nick Desaulniers
@ 2020-02-25 9:35 ` David Laight
2020-02-25 21:28 ` Nick Desaulniers
0 siblings, 1 reply; 10+ messages in thread
From: David Laight @ 2020-02-25 9:35 UTC (permalink / raw)
To: 'Nick Desaulniers', Ian Rogers, Arnaldo Carvalho de Melo
Cc: Nick Desaulniers, Ingo Molnar, Peter Zijlstra, clang-built-linux,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Jin Yao, Changbin Du, John Keeping, Song Liu, LKML
From: Nick Desaulniers
> Sent: 24 February 2020 22:06
> On Mon, Feb 24, 2020 at 10:20 AM 'Ian Rogers' via Clang Built Linux
> <clang-built-linux@googlegroups.com> wrote:
> >
> > On Mon, Feb 24, 2020 at 8:03 AM David Laight <David.Laight@aculab.com> wrote:
> > >
> > > From: Ian Rogers
> > > > Sent: 24 February 2020 05:56
> > > > On Sun, Feb 23, 2020 at 11:35 AM Nick Desaulniers
> > > > <nick.desaulniers@gmail.com> wrote:
> > > > >
> > > > > Clang warns:
> > > > >
> > > > > util/block-info.c:298:18: error: result of comparison against a string
> > > > > literal is unspecified (use an explicit string comparison function
> > > > > instead) [-Werror,-Wstring-compare]
> > > > > if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
> > > > > ^ ~~~~~~~~~~~~~~~
> > > > > util/block-info.c:298:51: error: result of comparison against a string
> > > > > literal is unspecified (use an explicit string comparison function
> > > > > instead) [-Werror,-Wstring-compare]
> > > > > if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
> > > > > ^ ~~~~~~~~~~~~~~~
> > > > > util/block-info.c:298:18: error: result of comparison against a string
> > > > > literal is unspecified (use an explicit string
> > > > > comparison function instead) [-Werror,-Wstring-compare]
> > > > > if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
> > > > > ^ ~~~~~~~~~~~~~~~
> > > > > util/block-info.c:298:51: error: result of comparison against a string
> > > > > literal is unspecified (use an explicit string comparison function
> > > > > instead) [-Werror,-Wstring-compare]
> > > > > if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
> > > > > ^ ~~~~~~~~~~~~~~~
> > > > > util/map.c:434:15: error: result of comparison against a string literal
> > > > > is unspecified (use an explicit string comparison function instead)
> > > > > [-Werror,-Wstring-compare]
> > > > > if (srcline != SRCLINE_UNKNOWN)
> > > > > ^ ~~~~~~~~~~~~~~~
> > > > >
> > > > > Link: https://github.com/ClangBuiltLinux/linux/issues/900
> > > > > Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
> > > > > ---
> > > > > Note: was generated off of mainline; can rebase on -next if it doesn't
> > > > > apply cleanly.
> >
> > Reviewed-by: Ian Rogers <irogers@google.com>
> >
> > > > Looks good to me. Some more context:
> > > > https://clang.llvm.org/docs/DiagnosticsReference.html#wstring-compare
> > > > The spec says:
> > > > J.1 Unspecified behavior
> > > > The following are unspecified:
> > > > .. Whether two string literals result in distinct arrays (6.4.5).
> > >
> > > Just change the (probable):
> > > #define SRCLINE_UNKNOWN "unknown"
> > > with
> > > static const char SRC_LINE_UNKNOWN[] = "unk";
> > >
> > > David
> >
> >
> > The SRCLINE_UNKNOWN is used to convey information. Having multiple
> > distinct pointers (static) would mean the compiler could likely remove
> > all comparisons as the compiler could prove that pointer is never
> > returned by a function - ie comparisons are either known to be true
> > (!=) or false (==).
>
> I wouldn't define a static string in a header. Though I could:
> 1. forward declare it in the header with extern linkage.
> 2. define it in *one* .c source file.
>
> Thoughts on that vs the current approach?
The string compares are just stupid.
If the 'fake' strings are not printed you could use:
#define SRCLINE_UNKNOWN ((const char *)1)
Otherwise defining the strings in one of the C files is better.
Relying on the linker to merge the strings from different compilation
units is so broken...
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] perf: fix -Wstring-compare
2020-02-25 9:35 ` David Laight
@ 2020-02-25 21:28 ` Nick Desaulniers
2020-03-03 20:06 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 10+ messages in thread
From: Nick Desaulniers @ 2020-02-25 21:28 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar
Cc: Ian Rogers, Nick Desaulniers, clang-built-linux, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Namhyung Kim, Jin Yao, Changbin Du,
John Keeping, Song Liu, LKML, David Laight
On Tue, Feb 25, 2020 at 1:35 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: Nick Desaulniers
> > Sent: 24 February 2020 22:06
> > On Mon, Feb 24, 2020 at 10:20 AM 'Ian Rogers' via Clang Built Linux
> > <clang-built-linux@googlegroups.com> wrote:
> > >
> > > On Mon, Feb 24, 2020 at 8:03 AM David Laight <David.Laight@aculab.com> wrote:
> > > >
> > > > From: Ian Rogers
> > > > > Sent: 24 February 2020 05:56
> > > > > On Sun, Feb 23, 2020 at 11:35 AM Nick Desaulniers
> > > > > <nick.desaulniers@gmail.com> wrote:
> > > > > >
> > > > > > Clang warns:
> > > > > >
> > > > > > util/block-info.c:298:18: error: result of comparison against a string
> > > > > > literal is unspecified (use an explicit string comparison function
> > > > > > instead) [-Werror,-Wstring-compare]
> > > > > > if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
> > > > > > ^ ~~~~~~~~~~~~~~~
> > > > > > util/block-info.c:298:51: error: result of comparison against a string
> > > > > > literal is unspecified (use an explicit string comparison function
> > > > > > instead) [-Werror,-Wstring-compare]
> > > > > > if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
> > > > > > ^ ~~~~~~~~~~~~~~~
> > > > > > util/block-info.c:298:18: error: result of comparison against a string
> > > > > > literal is unspecified (use an explicit string
> > > > > > comparison function instead) [-Werror,-Wstring-compare]
> > > > > > if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
> > > > > > ^ ~~~~~~~~~~~~~~~
> > > > > > util/block-info.c:298:51: error: result of comparison against a string
> > > > > > literal is unspecified (use an explicit string comparison function
> > > > > > instead) [-Werror,-Wstring-compare]
> > > > > > if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
> > > > > > ^ ~~~~~~~~~~~~~~~
> > > > > > util/map.c:434:15: error: result of comparison against a string literal
> > > > > > is unspecified (use an explicit string comparison function instead)
> > > > > > [-Werror,-Wstring-compare]
> > > > > > if (srcline != SRCLINE_UNKNOWN)
> > > > > > ^ ~~~~~~~~~~~~~~~
> > > > > >
> > > > > > Link: https://github.com/ClangBuiltLinux/linux/issues/900
> > > > > > Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
> > > > > > ---
> > > > > > Note: was generated off of mainline; can rebase on -next if it doesn't
> > > > > > apply cleanly.
> > >
> > > Reviewed-by: Ian Rogers <irogers@google.com>
> > >
> > > > > Looks good to me. Some more context:
> > > > > https://clang.llvm.org/docs/DiagnosticsReference.html#wstring-compare
> > > > > The spec says:
> > > > > J.1 Unspecified behavior
> > > > > The following are unspecified:
> > > > > .. Whether two string literals result in distinct arrays (6.4.5).
> > > >
> > > > Just change the (probable):
> > > > #define SRCLINE_UNKNOWN "unknown"
> > > > with
> > > > static const char SRC_LINE_UNKNOWN[] = "unk";
> > > >
> > > > David
> > >
> > >
> > > The SRCLINE_UNKNOWN is used to convey information. Having multiple
> > > distinct pointers (static) would mean the compiler could likely remove
> > > all comparisons as the compiler could prove that pointer is never
> > > returned by a function - ie comparisons are either known to be true
> > > (!=) or false (==).
> >
> > I wouldn't define a static string in a header. Though I could:
> > 1. forward declare it in the header with extern linkage.
> > 2. define it in *one* .c source file.
> >
> > Thoughts on that vs the current approach?
>
> The string compares are just stupid.
> If the 'fake' strings are not printed you could use:
> #define SRCLINE_UNKNOWN ((const char *)1)
>
> Otherwise defining the strings in one of the C files is better.
> Relying on the linker to merge the strings from different compilation
> units is so broken...
Note: it looks like free_srcline() already does strcmp, so my patch
basically does a more consistent job for string comparisons. Forward
declaring then defining in tools/perf/util/srcline.c involves changing
the function signatures and struct members to `const char*` rather
than `char*`, which is of questionable value. I wouldn't mind
changing my patch to just use strcmp instead of strncmp, or convert
free_srcline() to use strncmp instead, if folks felt strongly about
being consistent. Otherwise I think my patch with Ian's Reviewed-by is
the best approach.
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] perf: fix -Wstring-compare
2020-02-25 21:28 ` Nick Desaulniers
@ 2020-03-03 20:06 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 10+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-03-03 20:06 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Peter Zijlstra, Ingo Molnar, Ian Rogers, Nick Desaulniers,
clang-built-linux, Mark Rutland, Alexander Shishkin, Jiri Olsa,
Namhyung Kim, Jin Yao, Changbin Du, John Keeping, Song Liu, LKML,
David Laight
Em Tue, Feb 25, 2020 at 01:28:50PM -0800, Nick Desaulniers escreveu:
> On Tue, Feb 25, 2020 at 1:35 AM David Laight <David.Laight@aculab.com> wrote:
> >
> > From: Nick Desaulniers
> > > Sent: 24 February 2020 22:06
> > > On Mon, Feb 24, 2020 at 10:20 AM 'Ian Rogers' via Clang Built Linux
> > > <clang-built-linux@googlegroups.com> wrote:
> > > >
> > > > On Mon, Feb 24, 2020 at 8:03 AM David Laight <David.Laight@aculab.com> wrote:
> > > > >
> > > > > From: Ian Rogers
> > > > > > Sent: 24 February 2020 05:56
> > > > > > On Sun, Feb 23, 2020 at 11:35 AM Nick Desaulniers
> > > > > > <nick.desaulniers@gmail.com> wrote:
> > > > > > >
> > > > > > > Clang warns:
> > > > > > >
> > > > > > > util/block-info.c:298:18: error: result of comparison against a string
> > > > > > > literal is unspecified (use an explicit string comparison function
> > > > > > > instead) [-Werror,-Wstring-compare]
> > > > > > > if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
> > > > > > > ^ ~~~~~~~~~~~~~~~
> > > > > > > util/block-info.c:298:51: error: result of comparison against a string
> > > > > > > literal is unspecified (use an explicit string comparison function
> > > > > > > instead) [-Werror,-Wstring-compare]
> > > > > > > if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
> > > > > > > ^ ~~~~~~~~~~~~~~~
> > > > > > > util/block-info.c:298:18: error: result of comparison against a string
> > > > > > > literal is unspecified (use an explicit string
> > > > > > > comparison function instead) [-Werror,-Wstring-compare]
> > > > > > > if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
> > > > > > > ^ ~~~~~~~~~~~~~~~
> > > > > > > util/block-info.c:298:51: error: result of comparison against a string
> > > > > > > literal is unspecified (use an explicit string comparison function
> > > > > > > instead) [-Werror,-Wstring-compare]
> > > > > > > if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
> > > > > > > ^ ~~~~~~~~~~~~~~~
> > > > > > > util/map.c:434:15: error: result of comparison against a string literal
> > > > > > > is unspecified (use an explicit string comparison function instead)
> > > > > > > [-Werror,-Wstring-compare]
> > > > > > > if (srcline != SRCLINE_UNKNOWN)
> > > > > > > ^ ~~~~~~~~~~~~~~~
> > > > > > >
> > > > > > > Link: https://github.com/ClangBuiltLinux/linux/issues/900
> > > > > > > Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
> > > > > > > ---
> > > > > > > Note: was generated off of mainline; can rebase on -next if it doesn't
> > > > > > > apply cleanly.
> > > >
> > > > Reviewed-by: Ian Rogers <irogers@google.com>
> > > >
> > > > > > Looks good to me. Some more context:
> > > > > > https://clang.llvm.org/docs/DiagnosticsReference.html#wstring-compare
> > > > > > The spec says:
> > > > > > J.1 Unspecified behavior
> > > > > > The following are unspecified:
> > > > > > .. Whether two string literals result in distinct arrays (6.4.5).
> > > > >
> > > > > Just change the (probable):
> > > > > #define SRCLINE_UNKNOWN "unknown"
> > > > > with
> > > > > static const char SRC_LINE_UNKNOWN[] = "unk";
> > > > >
> > > > > David
> > > >
> > > >
> > > > The SRCLINE_UNKNOWN is used to convey information. Having multiple
> > > > distinct pointers (static) would mean the compiler could likely remove
> > > > all comparisons as the compiler could prove that pointer is never
> > > > returned by a function - ie comparisons are either known to be true
> > > > (!=) or false (==).
> > >
> > > I wouldn't define a static string in a header. Though I could:
> > > 1. forward declare it in the header with extern linkage.
> > > 2. define it in *one* .c source file.
> > >
> > > Thoughts on that vs the current approach?
> >
> > The string compares are just stupid.
> > If the 'fake' strings are not printed you could use:
> > #define SRCLINE_UNKNOWN ((const char *)1)
> >
> > Otherwise defining the strings in one of the C files is better.
> > Relying on the linker to merge the strings from different compilation
> > units is so broken...
>
> Note: it looks like free_srcline() already does strcmp, so my patch
> basically does a more consistent job for string comparisons. Forward
> declaring then defining in tools/perf/util/srcline.c involves changing
> the function signatures and struct members to `const char*` rather
> than `char*`, which is of questionable value. I wouldn't mind
> changing my patch to just use strcmp instead of strncmp, or convert
> free_srcline() to use strncmp instead, if folks felt strongly about
> being consistent. Otherwise I think my patch with Ian's Reviewed-by is
> the best approach.
Fair enough, applying it with Ian's reviewed-by,
- Arnaldo
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tip: perf/urgent] perf diff: Fix undefined string comparision spotted by clang's -Wstring-compare
2020-02-23 19:34 [PATCH] perf: fix -Wstring-compare Nick Desaulniers
2020-02-24 5:55 ` Ian Rogers
@ 2020-03-07 7:36 ` tip-bot2 for Nick Desaulniers
2020-03-19 14:10 ` [tip: perf/core] perf diff: Fix undefined string comparison " tip-bot2 for Nick Desaulniers
2 siblings, 0 replies; 10+ messages in thread
From: tip-bot2 for Nick Desaulniers @ 2020-03-07 7:36 UTC (permalink / raw)
To: linux-tip-commits
Cc: Nick Desaulniers, Ian Rogers, Alexander Shishkin, Changbin Du,
Jin Yao, Jiri Olsa, John Keeping, Mark Rutland, Namhyung Kim,
Peter Zijlstra, Song Liu, clang-built-linux,
Arnaldo Carvalho de Melo, x86, LKML
The following commit has been merged into the perf/urgent branch of tip:
Commit-ID: cfd3bc752a3f5529506d279deb42e3bc8055695b
Gitweb: https://git.kernel.org/tip/cfd3bc752a3f5529506d279deb42e3bc8055695b
Author: Nick Desaulniers <nick.desaulniers@gmail.com>
AuthorDate: Sun, 23 Feb 2020 11:34:49 -08:00
Committer: Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Fri, 06 Mar 2020 08:30:29 -03:00
perf diff: Fix undefined string comparision spotted by clang's -Wstring-compare
clang warns:
util/block-info.c:298:18: error: result of comparison against a string
literal is unspecified (use an explicit string comparison function
instead) [-Werror,-Wstring-compare]
if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
^ ~~~~~~~~~~~~~~~
util/block-info.c:298:51: error: result of comparison against a string
literal is unspecified (use an explicit string comparison function
instead) [-Werror,-Wstring-compare]
if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
^ ~~~~~~~~~~~~~~~
util/block-info.c:298:18: error: result of comparison against a string
literal is unspecified (use an explicit string
comparison function instead) [-Werror,-Wstring-compare]
if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
^ ~~~~~~~~~~~~~~~
util/block-info.c:298:51: error: result of comparison against a string
literal is unspecified (use an explicit string comparison function
instead) [-Werror,-Wstring-compare]
if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
^ ~~~~~~~~~~~~~~~
util/map.c:434:15: error: result of comparison against a string literal
is unspecified (use an explicit string comparison function instead)
[-Werror,-Wstring-compare]
if (srcline != SRCLINE_UNKNOWN)
^ ~~~~~~~~~~~~~~~
Reviewer Notes:
Looks good to me. Some more context:
https://clang.llvm.org/docs/DiagnosticsReference.html#wstring-compare
The spec says:
J.1 Unspecified behavior
The following are unspecified:
.. Whether two string literals result in distinct arrays (6.4.5).
Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
Reviewed-by: Ian Rogers <irogers@google.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Changbin Du <changbin.du@intel.com>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: John Keeping <john@metanate.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Song Liu <songliubraving@fb.com>
Cc: clang-built-linux@googlegroups.com
Link: https://github.com/ClangBuiltLinux/linux/issues/900
Link: http://lore.kernel.org/lkml/20200223193456.25291-1-nick.desaulniers@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/builtin-diff.c | 3 ++-
tools/perf/util/block-info.c | 3 ++-
tools/perf/util/map.c | 2 +-
3 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index f8b6ae5..c03c36f 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -1312,7 +1312,8 @@ static int cycles_printf(struct hist_entry *he, struct hist_entry *pair,
end_line = map__srcline(he->ms.map, bi->sym->start + bi->end,
he->ms.sym);
- if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
+ if ((strncmp(start_line, SRCLINE_UNKNOWN, strlen(SRCLINE_UNKNOWN)) != 0) &&
+ (strncmp(end_line, SRCLINE_UNKNOWN, strlen(SRCLINE_UNKNOWN)) != 0)) {
scnprintf(buf, sizeof(buf), "[%s -> %s] %4ld",
start_line, end_line, block_he->diff.cycles);
} else {
diff --git a/tools/perf/util/block-info.c b/tools/perf/util/block-info.c
index c4b030b..fbbb6d6 100644
--- a/tools/perf/util/block-info.c
+++ b/tools/perf/util/block-info.c
@@ -295,7 +295,8 @@ static int block_range_entry(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
end_line = map__srcline(he->ms.map, bi->sym->start + bi->end,
he->ms.sym);
- if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
+ if ((strncmp(start_line, SRCLINE_UNKNOWN, strlen(SRCLINE_UNKNOWN)) != 0) &&
+ (strncmp(end_line, SRCLINE_UNKNOWN, strlen(SRCLINE_UNKNOWN)) != 0)) {
scnprintf(buf, sizeof(buf), "[%s -> %s]",
start_line, end_line);
} else {
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index a08ca27..9542851 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -431,7 +431,7 @@ int map__fprintf_srcline(struct map *map, u64 addr, const char *prefix,
if (map && map->dso) {
char *srcline = map__srcline(map, addr, NULL);
- if (srcline != SRCLINE_UNKNOWN)
+ if (strncmp(srcline, SRCLINE_UNKNOWN, strlen(SRCLINE_UNKNOWN)) != 0)
ret = fprintf(fp, "%s%s", prefix, srcline);
free_srcline(srcline);
}
^ permalink raw reply related [flat|nested] 10+ messages in thread* [tip: perf/core] perf diff: Fix undefined string comparison spotted by clang's -Wstring-compare
2020-02-23 19:34 [PATCH] perf: fix -Wstring-compare Nick Desaulniers
2020-02-24 5:55 ` Ian Rogers
2020-03-07 7:36 ` [tip: perf/urgent] perf diff: Fix undefined string comparision spotted by clang's -Wstring-compare tip-bot2 for Nick Desaulniers
@ 2020-03-19 14:10 ` tip-bot2 for Nick Desaulniers
2 siblings, 0 replies; 10+ messages in thread
From: tip-bot2 for Nick Desaulniers @ 2020-03-19 14:10 UTC (permalink / raw)
To: linux-tip-commits
Cc: Nick Desaulniers, Ian Rogers, Alexander Shishkin, Changbin Du,
Jin Yao, Jiri Olsa, John Keeping, Mark Rutland, Namhyung Kim,
Peter Zijlstra, Song Liu, clang-built-linux,
Arnaldo Carvalho de Melo, x86, LKML
The following commit has been merged into the perf/core branch of tip:
Commit-ID: c395c3553d6870541f1c283479aea2a6f26364d5
Gitweb: https://git.kernel.org/tip/c395c3553d6870541f1c283479aea2a6f26364d5
Author: Nick Desaulniers <nick.desaulniers@gmail.com>
AuthorDate: Sun, 23 Feb 2020 11:34:49 -08:00
Committer: Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Wed, 04 Mar 2020 10:28:08 -03:00
perf diff: Fix undefined string comparison spotted by clang's -Wstring-compare
clang warns:
util/block-info.c:298:18: error: result of comparison against a string
literal is unspecified (use an explicit string comparison function
instead) [-Werror,-Wstring-compare]
if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
^ ~~~~~~~~~~~~~~~
util/block-info.c:298:51: error: result of comparison against a string
literal is unspecified (use an explicit string comparison function
instead) [-Werror,-Wstring-compare]
if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
^ ~~~~~~~~~~~~~~~
util/block-info.c:298:18: error: result of comparison against a string
literal is unspecified (use an explicit string
comparison function instead) [-Werror,-Wstring-compare]
if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
^ ~~~~~~~~~~~~~~~
util/block-info.c:298:51: error: result of comparison against a string
literal is unspecified (use an explicit string comparison function
instead) [-Werror,-Wstring-compare]
if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
^ ~~~~~~~~~~~~~~~
util/map.c:434:15: error: result of comparison against a string literal
is unspecified (use an explicit string comparison function instead)
[-Werror,-Wstring-compare]
if (srcline != SRCLINE_UNKNOWN)
^ ~~~~~~~~~~~~~~~
Reviewer Notes:
Looks good to me. Some more context:
https://clang.llvm.org/docs/DiagnosticsReference.html#wstring-compare
The spec says:
J.1 Unspecified behavior
The following are unspecified:
.. Whether two string literals result in distinct arrays (6.4.5).
Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
Reviewed-by: Ian Rogers <irogers@google.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Changbin Du <changbin.du@intel.com>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: John Keeping <john@metanate.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Song Liu <songliubraving@fb.com>
Cc: clang-built-linux@googlegroups.com
Link: https://github.com/ClangBuiltLinux/linux/issues/900
Link: http://lore.kernel.org/lkml/20200223193456.25291-1-nick.desaulniers@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
tools/perf/builtin-diff.c | 3 ++-
tools/perf/util/block-info.c | 3 ++-
tools/perf/util/map.c | 2 +-
3 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index f8b6ae5..c03c36f 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -1312,7 +1312,8 @@ static int cycles_printf(struct hist_entry *he, struct hist_entry *pair,
end_line = map__srcline(he->ms.map, bi->sym->start + bi->end,
he->ms.sym);
- if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
+ if ((strncmp(start_line, SRCLINE_UNKNOWN, strlen(SRCLINE_UNKNOWN)) != 0) &&
+ (strncmp(end_line, SRCLINE_UNKNOWN, strlen(SRCLINE_UNKNOWN)) != 0)) {
scnprintf(buf, sizeof(buf), "[%s -> %s] %4ld",
start_line, end_line, block_he->diff.cycles);
} else {
diff --git a/tools/perf/util/block-info.c b/tools/perf/util/block-info.c
index c4b030b..fbbb6d6 100644
--- a/tools/perf/util/block-info.c
+++ b/tools/perf/util/block-info.c
@@ -295,7 +295,8 @@ static int block_range_entry(struct perf_hpp_fmt *fmt, struct perf_hpp *hpp,
end_line = map__srcline(he->ms.map, bi->sym->start + bi->end,
he->ms.sym);
- if ((start_line != SRCLINE_UNKNOWN) && (end_line != SRCLINE_UNKNOWN)) {
+ if ((strncmp(start_line, SRCLINE_UNKNOWN, strlen(SRCLINE_UNKNOWN)) != 0) &&
+ (strncmp(end_line, SRCLINE_UNKNOWN, strlen(SRCLINE_UNKNOWN)) != 0)) {
scnprintf(buf, sizeof(buf), "[%s -> %s]",
start_line, end_line);
} else {
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index a08ca27..9542851 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -431,7 +431,7 @@ int map__fprintf_srcline(struct map *map, u64 addr, const char *prefix,
if (map && map->dso) {
char *srcline = map__srcline(map, addr, NULL);
- if (srcline != SRCLINE_UNKNOWN)
+ if (strncmp(srcline, SRCLINE_UNKNOWN, strlen(SRCLINE_UNKNOWN)) != 0)
ret = fprintf(fp, "%s%s", prefix, srcline);
free_srcline(srcline);
}
^ permalink raw reply related [flat|nested] 10+ messages in thread