[PATCH] perf: fix -Wstring-compare

[Nick Desaulniers]

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.des...@gmail.com>
---
Note: was generated off of mainline; can rebase on -next if it doesn't
apply cleanly.


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

[Ian Rogers]

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).

> --
> 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-li...@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.

[David Laight]

From: Ian Rogers
> Sent: 24 February 2020 05:56

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)

[Ian Rogers]

Reviewed-by: Ian Rogers <iro...@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

[Nick Desaulniers]

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

[David Laight]

From: Nick Desaulniers
> Sent: 24 February 2020 22:06

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

[Nick Desaulniers]

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

[Arnaldo Carvalho de Melo]

Fair enough, applying it with Ian's reviewed-by,

- Arnaldo

[tip-bot2 for Nick Desaulniers]

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.des...@gmail.com>
AuthorDate: Sun, 23 Feb 2020 11:34:49 -08:00
Committer: Arnaldo Carvalho de Melo <ac...@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:

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.des...@gmail.com>
Reviewed-by: Ian Rogers <iro...@google.com>
Cc: Alexander Shishkin <alexander...@linux.intel.com>
Cc: Changbin Du <chang...@intel.com>
Cc: Jin Yao <yao...@linux.intel.com>
Cc: Jiri Olsa <jo...@redhat.com>
Cc: John Keeping <jo...@metanate.com>
Cc: Mark Rutland <mark.r...@arm.com>
Cc: Namhyung Kim <namh...@kernel.org>
Cc: Peter Zijlstra <pet...@infradead.org>
Cc: Song Liu <songliu...@fb.com>
Cc: clang-bu...@googlegroups.com
Link: https://github.com/ClangBuiltLinux/linux/issues/900
Link: http://lore.kernel.org/lkml/20200223193456.2529...@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <ac...@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

[tip-bot2 for Nick Desaulniers]

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.des...@gmail.com>
AuthorDate: Sun, 23 Feb 2020 11:34:49 -08:00
Committer: Arnaldo Carvalho de Melo <ac...@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:

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.des...@gmail.com>
Reviewed-by: Ian Rogers <iro...@google.com>
Cc: Alexander Shishkin <alexander...@linux.intel.com>
Cc: Changbin Du <chang...@intel.com>
Cc: Jin Yao <yao...@linux.intel.com>
Cc: Jiri Olsa <jo...@redhat.com>
Cc: John Keeping <jo...@metanate.com>
Cc: Mark Rutland <mark.r...@arm.com>
Cc: Namhyung Kim <namh...@kernel.org>
Cc: Peter Zijlstra <pet...@infradead.org>
Cc: Song Liu <songliu...@fb.com>
Cc: clang-bu...@googlegroups.com
Link: https://github.com/ClangBuiltLinux/linux/issues/900
Link: http://lore.kernel.org/lkml/20200223193456.2529...@gmail.com
Signed-off-by: Arnaldo Carvalho de Melo <ac...@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