On Sun, Apr 13, 2014 at 12:12 AM, Kenneth Graunke <kenn...@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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
