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

[Kenneth Graunke]

On 04/13/2014 12:53 AM, Matt Turner wrote:
> 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.

Oh, that makes sense.  Adding extra overloads seems fine then.

--Ken

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140413/0a2adbfd/attachment.sig>