[Mesa-dev] [PATCH 4/5] i965: Let dump_instructions write to a file.

[Matt Turner]

On Sun, Apr 13, 2014 at 12:12 AM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On 04/11/2014 10:29 PM, Matt Turner wrote:
>> ---
>>  src/mesa/drivers/dri/i965/brw_fs.cpp              | 129 +++++++++++++---------
>>  src/mesa/drivers/dri/i965/brw_fs.h                |   2 +
>>  src/mesa/drivers/dri/i965/brw_fs_reg_allocate.cpp |   2 +-
>>  src/mesa/drivers/dri/i965/brw_shader.cpp          |  20 +++-
>>  src/mesa/drivers/dri/i965/brw_shader.h            |   2 +
>>  src/mesa/drivers/dri/i965/brw_vec4.cpp            | 104 +++++++++--------
>>  src/mesa/drivers/dri/i965/brw_vec4.h              |   1 +
>>  7 files changed, 154 insertions(+), 106 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> index 85a5463..c1d79e1 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> @@ -2810,197 +2810,218 @@ fs_visitor::lower_uniform_pull_constant_loads()
>>  void
>>  fs_visitor::dump_instructions()
>>  {
>> +   dump_instructions(NULL);
>> +}
>> +
>> +void
>> +fs_visitor::dump_instructions(const char *name)
>> +{
>> +   invalidate_live_intervals();
>>     calculate_register_pressure();
>> +   FILE *file = stderr;
>> +   if (name) {
>> +      file = fopen(name, "w");
>> +   }
>
> You're playing with fire here.  Opening files is dangerous, considering
> that the X server ofter runs as root and executes this code.  One
> mistake in debugging code, and...root exploit.  We may want to restrict
> this to non-root users by default, and maybe only in debug builds...
>
> At any rate, I'd like to see patches 3-5 respun as follows:
>
> 1. Make dump_instruction() and dump_instructions() take FILE *
> parameters, and do the s/stderr/file/g sed job you've done here.  This
> is the bulk of the change, and easily justifiable - we can always use
> stdout vs. stderr or the like.
>
> 2. Introduce any code that opens files for writing.  With this being
> separate, we can think through the security implications.
>
> 3. Your actual optimization debugging.  I would probably squash patch 3
> into this patch, since it doesn't really stand alone.  *shrug*
>
> [snip]
>
>> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.h b/src/mesa/drivers/dri/i965/brw_vec4.h
>> index 159a5bd..43c3f64 100644
>> --- a/src/mesa/drivers/dri/i965/brw_vec4.h
>> +++ b/src/mesa/drivers/dri/i965/brw_vec4.h
>> @@ -597,6 +597,7 @@ public:
>>     bool process_move_condition(ir_rvalue *ir);
>>
>>     void dump_instruction(backend_instruction *inst);
>> +   void dump_instruction(backend_instruction *inst, FILE *file);
>
> Usually, this is done via a default argument:
>
> void dump_instruction(backend_instruction *inst, FILE *file = stderr);
>
> That would probably be cleaner.

I didn't do that on purpose, because gdb doesn't handle default
parameters when you call functions from it.