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

[Kenneth Graunke]

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.

>  
>     void visit_atomic_counter_intrinsic(ir_call *ir);

-------------- 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/c28d5c61/attachment.sig>