From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTPS id 8630A89F45 for ; Fri, 21 Feb 2020 10:17:27 +0000 (UTC) References: <20200220133416.1194071-1-lionel.g.landwerlin@intel.com> <20200221100908.GA25209@platvala-desk.ger.corp.intel.com> From: Lionel Landwerlin Message-ID: <18a8e969-c475-2c17-c249-bc16b5f7fb00@intel.com> Date: Fri, 21 Feb 2020 12:17:24 +0200 MIME-Version: 1.0 In-Reply-To: <20200221100908.GA25209@platvala-desk.ger.corp.intel.com> Content-Language: en-US Subject: Re: [igt-dev] [PATCH i-g-t v2] tools/i915-perf: workaround overzelous compiler warnings List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" To: Petri Latvala Cc: igt-dev@lists.freedesktop.org List-ID: On 21/02/2020 12:09, Petri Latvala wrote: > On Thu, Feb 20, 2020 at 03:34:16PM +0200, Lionel Landwerlin wrote: >> Take 2. >> >> Signed-off-by: Lionel Landwerlin >> --- >> tools/i915-perf/i915_perf_recorder_commands.h | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/tools/i915-perf/i915_perf_recorder_commands.h b/tools/i915-perf/i915_perf_recorder_commands.h >> index 4855d80f..44dd4438 100644 >> --- a/tools/i915-perf/i915_perf_recorder_commands.h >> +++ b/tools/i915-perf/i915_perf_recorder_commands.h >> @@ -35,5 +35,6 @@ struct recorder_command_base { >> }; >> >> struct recorder_command_dump { >> - uint8_t path[0]; >> + uint8_t unused; >> + uint8_t path[]; >> }; > And now all your mallocs need + 1. You're right, I forgot the sizeof(struct recorder_command_dump). > > Why do you insist on having two structs? I want the person reading this code to understand what the packets exchanged look like. > The recorder_command_dump > struct only acts as "here be an array" object and you don't do > anything to it as an object anywhere. > > A drive-by note, i915_perf_recorder_commands.h lacks include guards. > > > > diff --git a/tools/i915-perf/i915_perf_control.c b/tools/i915-perf/i915_perf_control.c > index a8d0d30f..be5996c0 100644 > --- a/tools/i915-perf/i915_perf_control.c > +++ b/tools/i915-perf/i915_perf_control.c > @@ -93,12 +93,12 @@ main(int argc, char *argv[]) > sizeof(struct recorder_command_base) + strlen(dump_file) + 1; > struct { > struct recorder_command_base base; > - struct recorder_command_dump dump; > + uint8_t dump[]; > } *data = malloc(total_len); > > data->base.command = RECORDER_COMMAND_DUMP; > data->base.size = total_len; > - snprintf((char *) data->dump.path, strlen(dump_file) + 1, "%s", dump_file); > + snprintf((char *) data->dump, strlen(dump_file) + 1, "%s", dump_file); > > fwrite(data, total_len, 1, command_fifo_file); > } else { > @@ -107,12 +107,12 @@ main(int argc, char *argv[]) > uint32_t total_len = sizeof(struct recorder_command_base) + path_len; > struct { > struct recorder_command_base base; > - struct recorder_command_dump dump; > + uint8_t dump[]; > } *data = malloc(total_len); > > data->base.command = RECORDER_COMMAND_DUMP; > data->base.size = total_len; > - snprintf((char *) data->dump.path, path_len, "%s/%s", cwd, dump_file); > + snprintf((char *) data->dump, path_len, "%s/%s", cwd, dump_file); > > fwrite(data, total_len, 1, command_fifo_file); > } > diff --git a/tools/i915-perf/i915_perf_recorder.c b/tools/i915-perf/i915_perf_recorder.c > index 760cabf1..6bbc451e 100644 > --- a/tools/i915-perf/i915_perf_recorder.c > +++ b/tools/i915-perf/i915_perf_recorder.c > @@ -605,7 +605,7 @@ read_command_file(struct recording_context *ctx) > switch (header.command) { > case RECORDER_COMMAND_DUMP: { > uint32_t len = header.size - sizeof(header), offset = 0; > - struct recorder_command_dump *dump = malloc(len); > + uint8_t *dump = malloc(len); > FILE *file; > > while (offset < len && > @@ -616,9 +616,9 @@ read_command_file(struct recording_context *ctx) > offset += ret; > } > > - fprintf(stdout, "Writing circular buffer to %s\n", dump->path); > + fprintf(stdout, "Writing circular buffer to %s\n", dump); > > - file = fopen((const char *) dump->path, "w+"); > + file = fopen((const char *) dump, "w+"); > if (file) { > struct chunk chunks[2]; > > @@ -634,11 +634,11 @@ read_command_file(struct recording_context *ctx) > fwrite(chunks[1].data, chunks[1].len, 1, file) != 1) || > !write_correlation_timestamps(file, ctx->drm_fd)) { > fprintf(stderr, "Unable to write circular buffer data in file '%s'\n", > - dump->path); > + dump); > } > fclose(file); > } else > - fprintf(stderr, "Unable to write dump file '%s'\n", dump->path); > + fprintf(stderr, "Unable to write dump file '%s'\n", dump); > > free(dump); > break; > diff --git a/tools/i915-perf/i915_perf_recorder_commands.h b/tools/i915-perf/i915_perf_recorder_commands.h > index 4855d80f..3db11327 100644 > --- a/tools/i915-perf/i915_perf_recorder_commands.h > +++ b/tools/i915-perf/i915_perf_recorder_commands.h > @@ -33,7 +33,3 @@ struct recorder_command_base { > uint32_t command; > uint32_t size; > }; > - > -struct recorder_command_dump { > - uint8_t path[0]; > -}; > Can you leave this structure in there but commented? Just so that there is a trace of what you is exchanged between the 2 processes. Thanks, -Lionel _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev