[Mesa-dev] [PATCH 2/2] anv: Use build-id for pipeline cache UUID.

[Chad Versace]

On Wed 15 Feb 2017, Emil Velikov wrote:
> On 14 February 2017 at 21:02, Chad Versace <chadversary at chromium.org> wrote:
> > On Tue 14 Feb 2017, Kenneth Graunke wrote:
> >> On Tuesday, February 14, 2017 12:38:45 PM PST Chad Versace wrote:
> >> > On Tue 14 Feb 2017, Matt Turner wrote:
> >> >
> >> >
> >> > >  static bool
> >> > > -anv_get_function_timestamp(void *ptr, uint32_t* timestamp)
> >> > > +anv_device_get_cache_uuid(void *uuid)
> >> > >  {
> >> > > -   Dl_info info;
> >> > > -   struct stat st;
> >> > > -   if (!dladdr(ptr, &info) || !info.dli_fname)
> >> > > +   const struct note *note = build_id_find_nhdr("libvulkan_intel.so");
> >> > > +   if (!note)
> >> > >        return false;
> >> > >
> >> > > -   if (stat(info.dli_fname, &st))
> >> > > +   unsigned len = build_id_length(note);
> >> > > +   if (len < VK_UUID_SIZE)
> >> > >        return false;
> >> > >
> >> > > -   *timestamp = st.st_mtim.tv_sec;
> >> > > -   return true;
> >> > > -}
> >> > > -
> >> > > -static bool
> >> > > -anv_device_get_cache_uuid(void *uuid)
> >> > > -{
> >> > > -   uint32_t timestamp;
> >> > > -
> >> > > -   memset(uuid, 0, VK_UUID_SIZE);
> >> > > -   if (!anv_get_function_timestamp(anv_device_get_cache_uuid, &timestamp))
> >> > > +   unsigned char *build_id = malloc(len);
> >> > > +   if (!build_id)
> >> > >        return false;
> >> > >
> >> > > -   snprintf(uuid, VK_UUID_SIZE, "anv-%d", timestamp);
> >> > > +   build_id_read(note, build_id);
> >> > > +
> >> > > +   memcpy(uuid, build_id, VK_UUID_SIZE);
> >> > > +   free(build_id);
> >> >
> >> > The Vulkan spec frowns on memory allocations when not needed. If you
> >> > must allocate memory here, then it should be through the VkInstance
> >> > allocation callbacks. However, it's best to avoid the allocation by
> >> > adding a size_t parameter, à la snprintf, to build_id_read().
> >> >
> >> > Otherwise, the patch looks good to me.
> >>
> >> You're worried about the performance of anv_physical_device_init()?
> >> This doesn't happen on lookup...just driver start up...
> >
> > I'm not worried about performance here. That would be foolish.
> >
> > I'm concerned about complying the expectations provided to advanced
> > users by the Vulkan spec.

> Hi Chad, pardon for dropping in uninvited:
> 
> Spec mandates that only the implementation should use the allocation
> callbacks, and not any of it's dependencies (say something in the C
> runtime/other library), correct ?

> Or does it implicitly that "Thou Shalt override the weak
> [mc]alloc/friends symbols provided by your C runtime" ?

The spec actually says neither. It is vague on this topic, but vague in
a hyper-detailed fashion, which is typical of Khronos specs.

I interpret "Chapter 10. Memory Allocation" as saying:

    - If the user supplies allocation callbacks, then the spec
      recommends that the implementation use them. (The spec doesn't say
      *should*, *suggest*, *must*, *shall*, or anything like that here,
      as far as I can tell. It's my interpretation that Ch10 is primarily a
      a recommendation.

    - If the implementation does use an allocation callback, Ch10
      defines precise constraints on how the implementation must use it.

    - Ch10 admits that the implementation may need to allocate internal
      memory, without using the callbacks. However, Ch10 seem to say
      that such internal allocations are soley for executable memory.
      Again, one could interpret the spec differently here.

> Although the src/intel is [almost] perfect, the i965 compiler still
> explicitly and implicitly uses [mc]alloc. Not mean to be bashful, just
> pointing out what may not be obvious at first.

Yep. The compiler doesn't use the allocation callbacks. For that matter,
even printf calls malloc on some libc implementations.