Received-SPF: pass (google.com: domain of linux-kernel+bounces-28521-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Tue, 16 Jan 2024 20:47:12 -0800 (PST)
From: Ilkka Koskinen <ilkka@os.amperecomputing.com>
To: Namhyung Kim <namhyung@kernel.org>
cc: Ilkka Koskinen <ilkka@os.amperecomputing.com>, 
    James Clark <james.clark@arm.com>, linux-perf-users@vger.kernel.org, 
    linux-kernel@vger.kernel.org, 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>, Ian Rogers <irogers@google.com>, 
    Adrian Hunter <adrian.hunter@intel.com>
Subject: Re: [PATCH] perf data convert: Fix segfault when converting to json
 on arm64
In-Reply-To: <CAM9d7ch5b9XHzQ-Hb=kPu-6jLyESWqgizz6MRNWb0d-NnK9aqg@mail.gmail.com>
Message-ID: <1834f367-4bc4-4fda-fc89-d2252a76b76d@os.amperecomputing.com>
References: <20240111232923.8138-1-ilkka@os.amperecomputing.com> <2c9f5893-450c-012b-b748-a8fe8ddfae86@arm.com> <CAM9d7ch5b9XHzQ-Hb=kPu-6jLyESWqgizz6MRNWb0d-NnK9aqg@mail.gmail.com>
Content-Type: multipart/mixed; boundary="1372433909-1510186062-1705466852=:2464"
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?d290NFBzRkhkek9ZQmNaUmN0eTdoazJtWlBob2lTbjkvTXpBd1lYa1FGbW9E?=
 =?utf-8?B?YWZIZ0Z5eTcwcnM5TzBkZkk5ZEdKbzJMU1pQZTFLcFIwUitIQW1xaDdQMXd4?=
 =?utf-8?B?SE1QUllJd1ZPeVdpTEVVTFNMUjJ3aFVHTEE1M1lvdUhtTXNRU09rdHE0YXJV?=
 =?utf-8?B?R1lZUkROb2xTSzh5T3orODBUbUtVenZVRUw2SmovcTV4WUhTT0VQUzRneC9P?=
 =?utf-8?B?QisrOVA3VGpjQkFSd3JSY1dYQU9kMklOczk2QS9keW41YUttNDRiTDFHSXNu?=
 =?utf-8?B?Q1JUQ0oxTGJ5WTAxam1LSjdmTzN0a1hEdDdNMGJBT21DNjAzUEE1dmhVVDA3?=
 =?utf-8?B?WW03Uk1jZTB0RERETFpIS1pnS2s0VjloZ1JPTG44TzJjNGNheG9ta1lmczBC?=
 =?utf-8?B?Y2VRUzVrSnFNOC8ydDdzSUU4dklZUWZYOEFTVFNITW5lMGs0alRwM21naXVv?=
 =?utf-8?B?eDRxdFFwSy82YmFoaGpnTEx4RDgxemdWMjkwL2dleVN5YkpmMDVOb2FqZGY5?=
 =?utf-8?B?WWlydGZsODNQNm1rUFJMSnhEVGRJN1dOOWpJd1pMYlpTUlR0NllPdlJrdVFN?=
 =?utf-8?B?aEpPdXJZall5ZGwvWFI2UkdtODJ4QUQrbG94cWtmYlV5VFR5MklxMFJWbXRT?=
 =?utf-8?B?eEFudTlUZEFZdnEvQS90dC9UNmtCamtrcHNuVTVad2JqUXlGMFNOelI4dlly?=
 =?utf-8?B?bzEwbU8yd01YdGE3Qmc0bzBCWjl2R3VoNE00ZllZdmdnRjUxaWE0bCsxcVhY?=
 =?utf-8?B?dTl6SXU2TFMveDBmZjhTY0VoNjhLNFp6em5lZzJCS01QeTFvcjY1Nk9GM0dl?=
 =?utf-8?B?aWRvcmwvVU9DMk1ONVRobTBKZVlBSFpxem9DU2xWajNIcTFvUVhMS3ZHMnEy?=
 =?utf-8?B?Nk5EYXRYdWxOWW5vYmZLNlZaVHg0M1ZhazZoQVpsVzcxcWJVRWpVNkdsaitF?=
 =?utf-8?B?TlJhV2ROTUU2R3J6OW85ODVUUUd3OGpSclgzOU1XNUZXb25KUUJLZ3Q5LzRS?=
 =?utf-8?B?OUJ0U1h6WjhsNmprWnJDYVZDYXQwRHJsSXZrbHc2UVlZMWxRck9pdDRBbE82?=
 =?utf-8?B?MEFSMXBmUXJuM2FySkVZUzBZY2xRUVpoQVAzWXE5RkQyR2Z1ejFHUWhUYUdU?=
 =?utf-8?B?ZTBYSlFpTFJtdEVvZ1o4K1gzdm9NY044UnlDWFJwYWFMbXg3V1BXcGpNMDZE?=
 =?utf-8?B?WXF0S3NxOHgrc3QrQURvdEVRbDc4a3dHOTF4K0tsZjBLYlRsTlEwbDJvWEly?=
 =?utf-8?B?NGEzODZoRVZxelJSdm4yZTlGTDMyaHVsZ1pGOWdOTCtGTUFsYVhnTkZKSXhq?=
 =?utf-8?B?cFpLZ3RuMUM3c3Bub2IzR0RWWWhQUjFaNFpiRHFFTFdTSDBtYXczcTRxSlYy?=
 =?utf-8?B?UzFheUNEOVlndFBxQVNpY2U1dU95TmI3cCt3RTdyK0h6R1BGK1BZc2N1NWhn?=
 =?utf-8?B?UmF2anB3b0Y1cnYxOEVudm1BODNqeFJuQllVeUlWTDhVUERVZzZKRUFtVVNH?=
 =?utf-8?B?M0FlNVJaU0dST085dU1YZkd3c3VIWkRhTmd1TnVpT0cxQ1NURFFDNHdwRTRo?=
 =?utf-8?B?Nkx4c1BlTHY2eUJiRE5ldmhLdzdyMGxUYmVzTDlFRjVuMXRSNGxuVS9wUXUr?=
 =?utf-8?B?eEhtYS8xM2U2aVpSS0VUZk9JODJhUkdEbURDZ0M4cHFxWGRQTy9WNjg1N2FR?=
 =?utf-8?B?VTJmSzJRK0V2OElwMzQrWWRLNEZiMnNrTU1oWUhheWtTRzNSMGxNakxyR2tq?=
 =?utf-8?B?N0VmMC9ZODVOcVFkZHRaOHJWL0YvQk02UXFDSnhBU3lQUXBrVzVodG95cHkw?=
 =?utf-8?B?cFIxYm94RFdCZnBsakl4UlBJSmNINFc3ZzEwRmd5bzBQOHE0U0NUeVZVVXVM?=
 =?utf-8?B?RXZKOVJ1QVdvNXdZWVZrcDVBcldCUmxQcnc3dkVIellUMVZPZnBSK0w0T3hk?=
 =?utf-8?B?MG56TWJiVU0rZFAzSUhLT1hhN2JOVzJaTXRHbmNVQ1NxQWxrYjdEZmY5NUhx?=
 =?utf-8?B?MGtVR2xEbWxmNi9INXpVYzVHRU9sRWN3Vm92Y2E1QmNyTHVRVTF2WENMbG9j?=
 =?utf-8?B?eFlNamg3OU9LT25lUGNlUXVwY29MaHd0WDBWRHI2Q1FGK25jSDIrcUM3ZlR4?=
 =?utf-8?B?YXFJcDhxZzRvZlF0ZXhEZVRGSWJ5K0F6c29ZY1g0ZS81aEE4dXNhNUpQWUV5?=
 =?utf-8?Q?ywYLVTVVLgS1PFUWBiq3f3w=3D?=
X-OriginatorOrg: os.amperecomputing.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 61e757cb-fea9-4964-6106-08dc17176adb
X-MS-Exchange-CrossTenant-AuthSource: PH0PR01MB8166.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Jan 2024 04:47:32.5568
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3bc2b170-fd94-476d-b0ce-4229bdc904a7
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Pj5xJDt5hggTFoaG32waWnZqM+tLyX3FX8fBQB6DSPmSJ3FxxE/31NkY/ox1lUgoz/3uKAvPvs6H6V8GYs8tllXFbFIlUvw8gE6inCRsL88HAg9asCKYWZXDzSg9iulq
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY3PR01MB6562

--1372433909-1510186062-1705466852=:2464
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8BIT


On Tue, 16 Jan 2024, Namhyung Kim wrote:
> Hello,
>
> On Fri, Jan 12, 2024 at 3:35 AM James Clark <james.clark@arm.com> wrote:
>>
>>
>>
>> On 11/01/2024 23:29, Ilkka Koskinen wrote:
>>> Arm64 doesn't have Model in /proc/cpuinfo and, thus, cpu_desc doesn't get
>>> assigned.
>>>
>>> Running
>>>       $ perf data convert --to-json perf.data.json
>>>
>>> ends up calling output_json_string() with NULL pointer, which causes a
>>> segmentation fault.
>>>
>>> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
>>> ---
>>>  tools/perf/util/data-convert-json.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/tools/perf/util/data-convert-json.c b/tools/perf/util/data-convert-json.c
>>> index 5bb3c2ba95ca..5d6de1cef546 100644
>>> --- a/tools/perf/util/data-convert-json.c
>>> +++ b/tools/perf/util/data-convert-json.c
>>> @@ -97,6 +97,11 @@ static void output_json_format(FILE *out, bool comma, int depth, const char *for
>>>  static void output_json_key_string(FILE *out, bool comma, int depth,
>>>               const char *key, const char *value)
>>>  {
>>> +     if (!value) {
>>> +             pr_info("No value set for key %s\n", key);
>>> +             return;
>>> +     }
>>> +
>>>       output_json_delimiters(out, comma, depth);
>>>       output_json_string(out, key);
>>>       fputs(": ", out);
>>
>>
>> It looks like this would hide new errors on any of the other fields that
>> output_json_key_string() is called on. Maybe it would be better to only
>> wrap the call to output cpu_desc with the if? If that's the only one
>> that we think is optional, and even better only do it for arm64.
>>
>> I mention this because the test for 'perf data convert' only checks for
>> valid json syntax, but not any fields. So we might want to avoid others
>> going missing.
>
> Makes sense.  Ilkka, can you send v2 with this?

I initially considered the choice James suggested but I kind of thought
that pr_info() might be enough. However, I don't have strong prefence on
either. I'll send an updated version soon.

Cheers, Ilkka

>
> Thanks,
> Namhyung
>
--1372433909-1510186062-1705466852=:2464--