Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 13 Jun 2023 10:39:04 +0800
From:   Changbin Du <changbin.du@huawei.com>
To:     Ian Rogers <irogers@google.com>
CC:     Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@kernel.org>,
        Namhyung Kim <namhyung@kernel.org>,
        Adrian Hunter <adrian.hunter@intel.com>,
        Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Tom Rix <trix@redhat.com>, <linux-perf-users@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <llvm@lists.linux.dev>,
        Changbin Du <changbin.du@huawei.com>
Subject: Re: [PATCH v1 1/2] perf srcline: Make addr2line configuration
 failure more verbose
Message-ID: <20230613023904.xlnfgzedfz4epjz3@M910t>
References: <20230609235419.204624-1-irogers@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230609235419.204624-1-irogers@google.com>
Precedence: bulk

On Fri, Jun 09, 2023 at 04:54:18PM -0700, Ian Rogers wrote:
> To aid debugging why it fails. Also, combine the loops for reading a
> line for the llvm/binutils cases.
> 
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/srcline.c | 34 +++++++++++++++++++++-------------
>  1 file changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/perf/util/srcline.c b/tools/perf/util/srcline.c
> index b8e596528d7e..fc85cdd6c8f9 100644
> --- a/tools/perf/util/srcline.c
> +++ b/tools/perf/util/srcline.c
> @@ -441,7 +441,7 @@ enum a2l_style {
>  	LLVM,
>  };
>  
> -static enum a2l_style addr2line_configure(struct child_process *a2l)
> +static enum a2l_style addr2line_configure(struct child_process *a2l, const char *dso_name)
>  {
>  	static bool cached;
>  	static enum a2l_style style;
> @@ -450,6 +450,7 @@ static enum a2l_style addr2line_configure(struct child_process *a2l)
>  		char buf[128];
>  		struct io io;
>  		int ch;
> +		int lines;
>  
>  		if (write(a2l->in, ",\n", 2) != 2)
>  			return BROKEN;
> @@ -459,19 +460,29 @@ static enum a2l_style addr2line_configure(struct child_process *a2l)
>  		if (ch == ',') {
>  			style = LLVM;
>  			cached = true;
> +			lines = 1;
>  		} else if (ch == '?') {
>  			style = GNU_BINUTILS;
>  			cached = true;
> +			lines = 2;
>  		} else {
> -			style = BROKEN;
> +			if (!symbol_conf.disable_add2line_warn) {
> +				char *output;
This 'output' should be initialized to NULL.

In file included from util/srcline.c:13:
util/srcline.c: In function ‘addr2line’:
/work/linux/tools/perf/libapi/include/api/io.h:130:2: error: ‘output’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  130 |  free(*line_out);
      |  ^~~~~~~~~~~~~~~
util/srcline.c:472:11: note: ‘output’ was declared here
  472 |     char *output;
      |           ^~~~~~
cc1: all warnings being treated as errors
make[4]: *** [/work/linux/tools/build/Makefile.build:97: util/srcline.o] Error 1


> +				size_t output_len;
> +
> +				io__getline(&io, &output, &output_len);
> +				pr_warning("%s %s: addr2line configuration failed\n",
> +					   __func__, dso_name);
> +				pr_warning("\t%c%s\n", ch, output);
> +			}
> +			return BROKEN;
>  		}
> -		do {
> +		while (lines) {
>  			ch = io__get_char(&io);
> -		} while (ch > 0 && ch != '\n');
> -		if (style == GNU_BINUTILS) {
> -			do {
> -				ch = io__get_char(&io);
> -			} while (ch > 0 && ch != '\n');
> +			if (ch <= 0)
> +				break;
> +			if (ch == '\n')
> +				lines--;
>  		}
>  		/* Ignore SIGPIPE in the event addr2line exits. */
>  		signal(SIGPIPE, SIG_IGN);
> @@ -591,12 +602,9 @@ static int addr2line(const char *dso_name, u64 addr,
>  			pr_warning("%s %s: addr2line_subprocess_init failed\n", __func__, dso_name);
>  		goto out;
>  	}
> -	a2l_style = addr2line_configure(a2l);
> -	if (a2l_style == BROKEN) {
> -		if (!symbol_conf.disable_add2line_warn)
> -			pr_warning("%s: addr2line configuration failed\n", __func__);
> +	a2l_style = addr2line_configure(a2l, dso_name);
> +	if (a2l_style == BROKEN)
>  		goto out;
> -	}
>  
>  	/*
>  	 * Send our request and then *deliberately* send something that can't be interpreted as
> -- 
> 2.41.0.162.gfafddb0af9-goog
> 

-- 
Cheers,
Changbin Du