<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Wed, Feb 15, 2017 at 1:54 PM, Chad Versace <<a href="mailto:chadversary@chromium.org" target="_blank">chadversary@chromium.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Wed 15 Feb 2017, Emil Velikov wrote: > On 14 February 2017 at 21:02, Chad Versace <<a href="mailto:chadversary@chromium.org">chadversary@chromium.org</a>> 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" ? </div></div>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.</blockquote><div> </div><div>Yeah, I think you're worrying too much. Sure, it might be a nicer API if get_build_id did a copy into a provided array, but I wouldn't make any malloc arguments. </div></div> </div></div>