[Mesa-dev] [PATCH mesa 00/13] Make standard function available on non-standard platforms
Jose Fonseca
jfonseca at vmware.com
Wed Nov 21 20:28:27 UTC 2018
> vasprintf have different return codes on different systems.
Forget this. I was mixing up with the other function that calculates how many chars a xxxprintf function would write, which we use to implement vasprintf.
Jose
________________________________
From: Jose Fonseca
Sent: Wednesday, November 21, 2018 8:16:35 PM
To: Ian Romanick; Eric Engestrom; mesa-dev at lists.freedesktop.org
Cc: Brian Paul; Roland Scheidegger; Neha Bhende; Charmaine Lee
Subject: Re: [Mesa-dev] [PATCH mesa 00/13] Make standard function available on non-standard platforms
> 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 <idr at freedesktop.org>
Sent: Wednesday, November 21, 2018 18:47
To: Eric Engestrom; mesa-dev at lists.freedesktop.org
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(-)
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181121/c23cdd41/attachment-0001.html>
More information about the mesa-dev
mailing list