> util: use standard name for strncat() > util: use standard name for strncmp() > util: use standard name for strcmp() > util: use standard name for strchr() > util: use standard name for sprintf() > util: use standard name for vasprintf() > util: use standard name for vsprintf() > util: use standard name for snprintf() > util: use standard name for vsnprintf()
Generally I agree with the principle of using standard functions, and provide drop-in replacements for the systems where they are broken. It leads to less friction, less need to learn new things. But C string functions is IMO a special case, because the standard functions are so poorly designed, and make writing correct and secure code so dawm difficult: - snprintf doesn't write the null char - strncat n parameter is awkard to use - vasprintf have different return codes on different systems. According to GNU manpages, it's not even C or POSIX. And a standard for the sake of standard is just silly. So IMO, if people care and have the time to uniformize these things, I think they should move away standard C functions, and write some sane C string manipulation abstraction/helpers. And we should make GCC throw warnings every time people try to use the standard C string functions. > All of these functions appear to have been added by 131a1fbc91725. I've added Jose to CC since he wrote it. [...] > That doesn't give much insight. :( > :( *sigh* The reason for adding was in b1922de9f3478869c6788ef4e954c06c20e7aa9c . We needed those because XP kernel driver environment didn't provide a complete CRT. But we dropped XP kernel support long time ago, so that's no longer relevant. I believe the reason these functions became widespread since then was the fact that MSVC functions didn't follow C99 (e.g., ll), but that might no longer be an issue with MSVC 2017 runtime. This needs to be verified. Finally there's also the fact that standard snprintf is sensitive to current locale, which is a bad idea for driver code. I suppose this is not an issue for Linux, which has always used snprintf underneath. But we need to double check all other util_snprintf . It's possible that the mere fact we statically link CRT is sufficient to protect us from that (since it ends up being like a completely different CRT instant from the application), but I'm not 100% sure. Jose ________________________________ From: Ian Romanick <[email protected]> Sent: Wednesday, November 21, 2018 18:47 To: Eric Engestrom; [email protected] Cc: Jose Fonseca Subject: Re: [Mesa-dev] [PATCH mesa 00/13] Make standard function available on non-standard platforms On 11/20/2018 05:11 AM, Eric Engestrom wrote: > ... instead of making standard platforms use non-standard functions. I haven't looked at the specific patches, so this comment may not apply. When we first headed down the path of adding a billion wrapper functions, I campaigned pretty strongly to give them the standard names. The problem is that some platforms have functions with the standard names that deviate from the standard behavior in ways that make them unusable. I think one of the printf-like functions was the main problem here, but it was a long time ago. Either way, you can't give the wrapper the standard name in this case. Once you have to name one wrapper function _foo_standard_name, you might as well name them all like that. > This also reduces the likelihood of someone forgetting to use the > non-standard function, and reduces the fix to a simple #include. If we cared, I bet we could write a 'make check' test that would just grep through the tree for functions that are supposed to be wrapped. The test would fail if a non-wrapped version was used. We'd probably have to use Python, so I don't know how much effort it would be. > Changes generated using this shell function for each function name: > fix() { > files=$(ag -lw util_$1) > sed s/'\<util_'$1'\>'/$1/g -i $files > git add -up $files > git commit -sm 'util: use standard name for '$1'()' > } > > Eric Engestrom (13): > util: use standard name for strchrnul() > util: use standard name for strcasecmp() > util: use standard name for strdup() > util: use standard name for strstr() What the... ? strstr is part of C89. What platform actually needs a wrapper for this? I see zero uses in the code base of util_strstr. Ditto for strdup, strncmp, strcmp, strchr, and possibly others. I looked, and Microsoft has strncmp at least as far back as Visual Studio 2012. https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.microsoft.com%2Fen-us%2Fprevious-versions%2Fvisualstudio%2Fvisual-studio-2012%2Feywx8zcx(v%3Dvs.110&data=02%7C01%7Cjfonseca%40vmware.com%7Cc87f73c43298407e4aeb08d64fe1bc31%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636784228289598865&sdata=Gg%2B%2BlQmc4wmh7k0zohBEdUatGjdCkPDnmCOxse6NSFE%3D&reserved=0) I don't see any of these functions on the list of functions that aren't always available: https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.microsoft.com%2Fen-us%2Fcpp%2Fcppcx%2Fcrt-functions-not-supported-in-universal-windows-platform-apps&data=02%7C01%7Cjfonseca%40vmware.com%7Cc87f73c43298407e4aeb08d64fe1bc31%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636784228289598865&sdata=wcTCCWiMFwy%2BpC0aTP%2FIqWEiv4LUhK%2B0eY0RhDPvu5g%3D&reserved=0 All of these functions appear to have been added by 131a1fbc91725. I've added Jose to CC since he wrote it. The whole commit message there is: util: Alternative implementation for standard c library string functions. That doesn't give much insight. :( Give that patch was 10 years ago, some of the wrappers that were needed in 2008 may not be needed now. I'd prefer to see wrappers that aren't needed in 2018 be replaced with the standard functions as a first step. We can sort out the remainder after that. > util: use standard name for strncat() > util: use standard name for strncmp() > util: use standard name for strcmp() > util: use standard name for strchr() > util: use standard name for sprintf() > util: use standard name for vasprintf() > util: use standard name for vsprintf() > util: use standard name for snprintf() > util: use standard name for vsnprintf() > > src/amd/common/ac_debug.c | 2 +- > .../glsl/ir_builder_print_visitor.cpp | 2 +- > src/compiler/glsl/ir_print_visitor.cpp | 12 ++--- > src/compiler/glsl/link_interface_blocks.cpp | 4 +- > src/compiler/glsl/linker.cpp | 2 +- > .../glsl/opt_dead_builtin_varyings.cpp | 10 ++-- > src/compiler/glsl_types.cpp | 10 ++-- > src/gallium/auxiliary/draw/draw_llvm.c | 10 ++-- > src/gallium/auxiliary/driver_ddebug/dd_util.h | 4 +- > src/gallium/auxiliary/driver_trace/tr_dump.c | 2 +- > .../auxiliary/gallivm/lp_bld_arit_overflow.c | 2 +- > .../auxiliary/gallivm/lp_bld_debug.cpp | 4 +- > src/gallium/auxiliary/gallivm/lp_bld_debug.h | 2 +- > src/gallium/auxiliary/gallivm/lp_bld_gather.c | 2 +- > src/gallium/auxiliary/gallivm/lp_bld_init.c | 4 +- > src/gallium/auxiliary/gallivm/lp_bld_intr.c | 4 +- > src/gallium/auxiliary/gallivm/lp_bld_printf.c | 8 +-- > .../auxiliary/gallivm/lp_bld_sample_soa.c | 2 +- > .../auxiliary/gallivm/lp_bld_tgsi_soa.c | 2 +- > src/gallium/auxiliary/hud/hud_context.c | 4 +- > .../auxiliary/pipe-loader/pipe_loader.c | 6 +-- > src/gallium/auxiliary/postprocess/pp_mlaa.c | 2 +- > src/gallium/auxiliary/tgsi/tgsi_dump.c | 2 +- > src/gallium/auxiliary/util/u_async_debug.c | 2 +- > src/gallium/auxiliary/util/u_debug_describe.c | 22 ++++---- > src/gallium/auxiliary/util/u_debug_flush.c | 2 +- > src/gallium/auxiliary/util/u_debug_image.c | 2 +- > src/gallium/auxiliary/util/u_debug_symbol.c | 10 ++-- > src/gallium/auxiliary/util/u_dump_state.c | 2 +- > src/gallium/auxiliary/util/u_log.c | 2 +- > src/gallium/auxiliary/util/u_network.c | 2 +- > src/gallium/auxiliary/util/u_simple_shaders.c | 2 +- > src/gallium/auxiliary/util/u_tests.c | 4 +- > src/gallium/drivers/etnaviv/etnaviv_screen.c | 2 +- > .../drivers/freedreno/freedreno_batch.c | 2 +- > .../drivers/freedreno/freedreno_screen.c | 2 +- > src/gallium/drivers/i915/i915_fpc_translate.c | 2 +- > src/gallium/drivers/i915/i915_screen.c | 2 +- > src/gallium/drivers/llvmpipe/lp_flush.c | 4 +- > src/gallium/drivers/llvmpipe/lp_rast.c | 2 +- > src/gallium/drivers/llvmpipe/lp_screen.c | 2 +- > src/gallium/drivers/llvmpipe/lp_state_fs.c | 4 +- > src/gallium/drivers/llvmpipe/lp_state_setup.c | 2 +- > src/gallium/drivers/llvmpipe/lp_test_arit.c | 2 +- > src/gallium/drivers/llvmpipe/lp_test_format.c | 2 +- > src/gallium/drivers/nouveau/nouveau_screen.c | 2 +- > src/gallium/drivers/radeonsi/si_debug.c | 2 +- > src/gallium/drivers/radeonsi/si_shader.c | 2 +- > src/gallium/drivers/softpipe/sp_flush.c | 4 +- > src/gallium/drivers/svga/svga_msg.c | 2 +- > src/gallium/drivers/svga/svga_pipe_flush.c | 4 +- > src/gallium/drivers/svga/svga_sampler_view.c | 2 +- > src/gallium/drivers/svga/svga_screen.c | 8 +-- > src/gallium/drivers/swr/swr_screen.cpp | 2 +- > src/gallium/drivers/vc4/vc4_bufmgr.c | 2 +- > src/intel/vulkan/anv_device.c | 4 +- > src/mesa/program/symbol_table.c | 4 +- > src/util/string_buffer.c | 2 +- > src/util/u_debug.c | 38 +++++++------- > src/util/u_queue.c | 6 +-- > src/util/u_string.h | 52 ++++++++----------- > 61 files changed, 154 insertions(+), 162 deletions(-) >
_______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
