[Mesa-dev] [PATCH 4/6] gallium/u_queue: add an option to name threads

[Eirik Byrkjeflot Anonsen]

Nicolai Hähnle <nhaehnle at gmail.com> writes:

> On 21.06.2016 16:50, Marek Olšák wrote:
>> On Tue, Jun 21, 2016 at 4:40 PM, Nicolai Hähnle <nhaehnle at gmail.com> wrote:
>>> On 21.06.2016 14:17, Marek Olšák wrote:
>>>>
>>>> From: Marek Olšák <marek.olsak at amd.com>
>>>>
>>>> for debugging
>>>> ---
>>>>    src/gallium/auxiliary/util/u_queue.c              | 10 ++++++++++
>>>>    src/gallium/auxiliary/util/u_queue.h              |  2 ++
>>>>    src/gallium/winsys/amdgpu/drm/amdgpu_winsys.c     |  2 +-
>>>>    src/gallium/winsys/radeon/drm/radeon_drm_winsys.c |  2 +-
>>>>    4 files changed, 14 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/src/gallium/auxiliary/util/u_queue.c
>>>> b/src/gallium/auxiliary/util/u_queue.c
>>>> index d14d850..a0b0317 100644
>>>> --- a/src/gallium/auxiliary/util/u_queue.c
>>>> +++ b/src/gallium/auxiliary/util/u_queue.c
>>>> @@ -26,6 +26,7 @@
>>>>
>>>>    #include "u_queue.h"
>>>>    #include "u_memory.h"
>>>> +#include "u_string.h"
>>>>    #include "os/os_time.h"
>>>>
>>>>    static void
>>>> @@ -61,6 +62,13 @@ static PIPE_THREAD_ROUTINE(util_queue_thread_func,
>>>> input)
>>>>
>>>>       FREE(input);
>>>>
>>>> +   if (queue->name) {
>>>> +      char name[16] = {0};
>>>> +      util_snprintf(name, sizeof(name) - 1, "%s:%i",
>>>> +                    queue->name, thread_index);
>>>
>>>
>>> It should be safe to just say util_snprintf(name, sizeof(name), ...) without
>>> initializing name.
>>
>> It's not. snprintf doesn't write '\0' if the output string length is >= size.
>
> Not sure if there was ever some variant that didn't, but both the 
> manpage and a quick test agree that it does.
>

I'm pretty sure snprintf() is safe in this way. I think it is only
strncpy and strncat that are broken.

strncpy() never NUL-terminates anything, but it fills the unused part of
the destination with NULs, so if the source is smaller than the
destination, then the result will "accidentally" be NUL-terminated
anyway.

strncat() on the other hand. If you see it used, it is probably a bug.

Hmm, that makes me curious:

$ git grep strncat
src/compiler/glsl/glcpp/pp.c:                           ralloc_strncat(&clean, shader,
src/compiler/glsl/glcpp/pp.c:                   ralloc_strncat(&clean, shader, backslash - shader);
src/gallium/auxiliary/gallivm/lp_bld_printf.c:      util_strncat(format, type_fmt, sizeof(format) - strlen(format) - 1);
src/gallium/auxiliary/gallivm/lp_bld_printf.c:         util_strncat(format, type_fmt, sizeof(format) - strlen(format) - 1);
src/gallium/auxiliary/gallivm/lp_bld_printf.c:   util_strncat(format, "\n", sizeof(format) - strlen(format) - 1);
src/gallium/auxiliary/util/u_debug.c:       util_strncat(output, "|", sizeof(output) - strlen(output) - 1);
src/gallium/auxiliary/util/u_debug.c:    util_strncat(output, names->name, sizeof(output) - strlen(output) - 1);
src/gallium/auxiliary/util/u_debug.c:    util_strncat(output, "|", sizeof(output) - strlen(output) - 1);
src/gallium/auxiliary/util/u_debug.c:      util_strncat(output, rest, sizeof(output) - strlen(output) - 1);
src/gallium/auxiliary/util/u_string.h:util_strncat(char *dst, const char *src, size_t n)
src/gallium/auxiliary/util/u_string.h:#define util_strncat strncat
src/mesa/main/atifragshader.c:      strncat(ret_str, "|2X", 1024);
src/mesa/main/atifragshader.c:      strncat(ret_str, "|4X", 1024);
src/mesa/main/atifragshader.c:      strncat(ret_str, "|8X", 1024);
src/mesa/main/atifragshader.c:      strncat(ret_str, "|HA", 1024);
src/mesa/main/atifragshader.c:      strncat(ret_str, "|QU", 1024);
src/mesa/main/atifragshader.c:      strncat(ret_str, "|EI", 1024);
src/mesa/main/atifragshader.c:      strncat(ret_str, "|SAT", 1024);
src/mesa/main/atifragshader.c:      strncat(ret_str, "NONE", 1024);
src/util/ralloc.c:/* helper routine for strcat/strncat - n is the exact amount to copy */
src/util/ralloc.c:ralloc_strncat(char **dest, const char *str, size_t n)
src/util/ralloc.h:bool ralloc_strncat(char **dest, const char *str, size_t n);

Wow, looks like most of these are actually used correctly. I think
that's the first time I have seen that. But all of the ones in
atifragshader.c are surely wrong.

eirik