On 04/13/2014 12:53 AM, Matt Turner wrote:
> 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.Oh, that makes sense. Adding extra overloads seems fine then. --Ken
signature.asc
Description: OpenPGP digital signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
