[Mesa-dev] [PATCH 20/25] anv: minor tweak in the generation script

[Dylan Baker]

Quoting Emil Velikov (2016-04-21 06:16:20)
> From: Emil Velikov <emil.velikov at collabora.com>
> 
> Rather than parsing through the same files (public headers) twice, tweak
> the python script to create both files at the same time.
> 
> Chances are that if the public headers change, both files will need to
> be regenerated.
> 
> Note to the python masters: this patch aims to be the least evasive
> change. Feel free to change/rewrite things to your liking.
> 
> Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
> ---
>  src/intel/vulkan/Makefile.am            |  8 +++----
>  src/intel/vulkan/anv_entrypoints_gen.py | 41 ++++++++++++++++++++++++---------
>  2 files changed, 33 insertions(+), 16 deletions(-)
> 
> diff --git a/src/intel/vulkan/Makefile.am b/src/intel/vulkan/Makefile.am
> index 360e97f..110961e 100644
> --- a/src/intel/vulkan/Makefile.am
> +++ b/src/intel/vulkan/Makefile.am
> @@ -126,11 +126,9 @@ VULKAN_LIB_DEPS += \
>  
>  libvulkan_intel_la_SOURCES = $(VULKAN_GEM_FILES)
>  
> -anv_entrypoints.h : anv_entrypoints_gen.py $(vulkan_include_HEADERS)
> -       $(AM_V_GEN) cat $(vulkan_include_HEADERS) | $(CPP) $(AM_CPPFLAGS) - | $(PYTHON2) $< header > $@
> -
> -anv_entrypoints.c : anv_entrypoints_gen.py $(vulkan_include_HEADERS)
> -       $(AM_V_GEN) cat $(vulkan_include_HEADERS) | $(CPP) $(AM_CPPFLAGS) - | $(PYTHON2) $< code > $@
> +anv_entrypoints.c anv_entrypoints.h : anv_entrypoints_gen.py $(vulkan_include_HEADERS)
> +       $(AM_V_GEN) cat $(vulkan_include_HEADERS) | $(CPP) $(AM_CPPFLAGS) - | \
> +       $(PYTHON2) $(srcdir)/anv_entrypoints_gen.py $(builddir)
>  
>  CLEANFILES = $(BUILT_SOURCES)
>  
> diff --git a/src/intel/vulkan/anv_entrypoints_gen.py b/src/intel/vulkan/anv_entrypoints_gen.py
> index cedecfe..19bfb93 100644
> --- a/src/intel/vulkan/anv_entrypoints_gen.py
> +++ b/src/intel/vulkan/anv_entrypoints_gen.py
> @@ -22,7 +22,7 @@
>  # IN THE SOFTWARE.
>  #
>  
> -import fileinput, re, sys
> +import fileinput, re, sys, os
>  
>  # Each function typedef in the vulkan.h header is all on one line and matches
>  # this regepx. We hope that won't change.
> @@ -51,15 +51,21 @@ def hash(name):
>  
>      return h
>  
> -opt_header = False
> -opt_code = False
> +if len(sys.argv[1:]) != 1:
> +    print "Usage: %s <output_directory>" % sys.argv[0]
> +    exit(1)
> +
> +output_dir = sys.argv[1]
> +if not os.path.isdir(output_dir):
> +    if os.path.exists(output_dir):
> +        print "ERROR: Invalid output directory: %s" % output_dir
> +        exit(1)
> +
> +sys.argv.pop()
> +# Output path exists, now just run the template
> +output_file = os.sep.join([output_dir, 'anv_entrypoints.c'])
> +output_header = os.sep.join([output_dir, 'anv_entrypoints.h'])
>  
> -if (sys.argv[1] == "header"):
> -    opt_header = True
> -    sys.argv.pop()
> -elif (sys.argv[1] == "code"):
> -    opt_code = True
> -    sys.argv.pop()
>  
>  # Parse the entry points in the header
>  
> @@ -77,7 +83,11 @@ for line in fileinput.input():
>  # For outputting entrypoints.h we generate a anv_EntryPoint() prototype
>  # per entry point.
>  
> -if opt_header:
> +def generate_header(output_header):
> +    orig_stdout = sys.stdout
> +    hdr = file(output_header, 'w')
> +    sys.stdout = hdr
> +
>      print "/* This file generated from vk_gen.py, don't edit directly. */\n"
>  
>      print "struct anv_dispatch_table {"
> @@ -100,9 +110,15 @@ if opt_header:
>          print "%s gen8_%s%s;" % (type, name, args)
>          print "%s gen9_%s%s;" % (type, name, args)
>          print "%s anv_validate_%s%s;" % (type, name, args)
> -    exit()
>  
> +    sys.stdout = orig_stdout
> +    hdr.close()
>  
> +generate_header(output_header)
> +
> +orig_stdout = sys.stdout
> +src = file(output_file, 'w')
> +sys.stdout = src

This is so ugly and prone to failure in odd ways. You really need a
try/finally block to to ensure that you're monkey patch is undone. This
could cause some really strange bugs.

I would implement it like this (well, I would write the patch like this):
if opt_header:
    with open(output_header, 'w') as f:
        print >>f, "/* This file generated from vk_gen.py, don't edit directly. */\n"

        print >>f, "struct anv_dispatch_table {"
        print >>f, "   union {"
        print >>f, "      void *entrypoints[%d];" % len(entrypoints)
        print >>f, "      struct {"

        for type, name, args, num, h in entrypoints:
            print >>f, "         %s (*%s)%s;" % (type, name, args)
        print >>f, "      };\n"
        print >>f, "   };\n"
        print >>f, "};\n"

        print >>f, "void anv_set_dispatch_devinfo(const struct brw_device_info *info);\n"

        for type, name, args, num, h in entrypoints:
            print >>f, "%s anv_%s%s;" % (type, name, args)
            print >>f, "%s gen7_%s%s;" % (type, name, args)
            print >>f, "%s gen75_%s%s;" % (type, name, args)
            print >>f, "%s gen8_%s%s;" % (type, name, args)
            print >>f, "%s gen9_%s%s;" % (type, name, args)
            print >>f, "%s anv_validate_%s%s;" % (type, name, args)
    exit()

Yes this is more invasive, but it does two things better. First it
avoids monkey patching sys.stdout, which (IMHO) is a bad idea for
production code (there are uses for it, like testing), and the second
that it uses the context manager which ensures that the file gets
closed.

Dylan

>  
>  print """/*
>   * Copyright © 2015 Intel Corporation
> @@ -321,3 +337,6 @@ anv_lookup_entrypoint(const char *name)
>     return anv_resolve_entrypoint(i);
>  }
>  """ % (prime_factor, prime_step, hash_mask)
> +
> +sys.stdout = orig_stdout
> +src.close()
> -- 
> 2.8.0
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: signature
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160427/871b3714/attachment.sig>