Em Mon, Oct 08, 2012 at 09:43:31AM +0300, Irina Tirdea escreveu:
> From: Namhyung Kim <namhyung....@lge.com>
>
> As we have architecture information of saved perf.data file, we can
> try to find cross-built objdump path.
>
> The triplets include support for Android (arm, x86 and mips architectures).
perf_session is too high an abstraction for this, all the uses in this
function are of the kind:
session->header.env.MEMBER
So why not rename it to:
perf_session_env__lookup_binutils_path(struct perf_session_env *env, const
char *name)
and then use just:
env->arch, etc?
More comments below
> +static char *try_binutils_path(struct perf_session *session, const char
> *name)
> +{
> + int idx;
> + char *arch, *env;
> + struct utsname uts;
> + const char *const *path_list;
> + char buf[PATH_MAX];
> +
> + if (uname(&uts) < 0)
> + return NULL;
> +
> + /*
> + * We don't need to try to find objdump path for native system.
> + * Just use default "objdump".
> + */
> + if (!strcmp(uts.machine, session->header.env.arch))
> + return NULL;
> +
> + env = getenv("CROSS_COMPILE");
> + if (env) {
> + scnprintf(buf, sizeof(buf), "%s%s", env, name);
> + if (buf[0] == '/') {
> + if (access(buf, F_OK) == 0)
> + return strdup(buf);
> +
> + return NULL;
> + }
> +
> + if (lookup_path(buf))
> + return strdup(buf);
> + }
> +
> + arch = session->header.env.arch;
> +
> + if (!strcmp(arch, "arm"))
> + path_list = arm_triplets;
> + else if (!strcmp(arch, "powerpc"))
> + path_list = powerpc_triplets;
> + else if (!strcmp(arch, "sh"))
> + path_list = sh_triplets;
> + else if (!strcmp(arch, "s390"))
> + path_list = s390_triplets;
> + else if (!strcmp(arch, "sparc"))
> + path_list = sparc_triplets;
> + else if (!strcmp(arch, "x86") || !strcmp(arch, "i386") ||
> + !strcmp(arch, "i486") || !strcmp(arch, "i586") ||
> + !strcmp(arch, "i686"))
> + path_list = x86_triplets;
> + else if (!strcmp(arch, "mips"))
> + path_list = mips_triplets;
> + else
> + BUG_ON(1);
> +
> + idx = lookup_triplets(path_list, name);
> + if (idx < 0)
> + return NULL;
> +
> + scnprintf(buf, sizeof(buf), "%s%s", path_list[idx], name);
> + return strdup(buf);
Please use instead:
char *s;
return asprintf(&s, "%s%s", path_list[idx], name) < 0 ? NULL : s;
Will produce the same result and is more compact.
> +}
> +
> +void try_objdump_path(struct perf_session *session)
> +{
> + objdump_path = try_binutils_path(session, "objdump");
> +}
> diff --git a/tools/perf/arch/common.h b/tools/perf/arch/common.h
> new file mode 100644
> index 0000000..d88ecc3
> --- /dev/null
> +++ b/tools/perf/arch/common.h
> @@ -0,0 +1,10 @@
> +#ifndef ARCH_PERF_COMMON_H
> +#define ARCH_PERF_COMMON_H
> +
> +#include "session.h"
> +
> +extern const char *objdump_path;
> +
> +void try_objdump_path(struct perf_session *session);
> +
> +#endif /* ARCH_PERF_COMMON_H */
> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> index 9ea3854..8d90ab5 100644
> --- a/tools/perf/builtin-annotate.c
> +++ b/tools/perf/builtin-annotate.c
> @@ -28,6 +28,7 @@
> #include "util/hist.h"
> #include "util/session.h"
> #include "util/tool.h"
> +#include "arch/common.h"
>
> #include <linux/bitmap.h>
>
> @@ -186,6 +187,9 @@ static int __cmd_annotate(struct perf_annotate *ann)
> goto out_delete;
> }
>
> + if (!objdump_path)
> + try_objdump_path(session);
Please do the test on perf_session_env__lookup_objdump() and handle
errors, i.e. what happens if that strdup/asprintf fails?
Ok, that is unlikely, but not impossible, but what happens if we don't
find the right objdump? Shouldn't we use ui__error("please install
required objdump" (or a better, more detailed message)?
> +
> ret = perf_session__process_events(session, &ann->tool);
> if (ret)
> goto out_delete;
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index a61725d..e1549bc 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -33,6 +33,7 @@
> #include "util/thread.h"
> #include "util/sort.h"
> #include "util/hist.h"
> +#include "arch/common.h"
>
> #include <linux/bitmap.h>
>
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index 39242dc..a4dd25a 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -154,6 +154,5 @@ static inline int symbol__tui_annotate(struct symbol *sym
> __maybe_unused,
> #endif
>
> extern const char *disassembler_style;
> -extern const char *objdump_path;
>
> #endif /* __PERF_ANNOTATE_H */
> --
> 1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
