[Mesa-dev] [PATCH mesa 00/13] Make standard function available on non-standard platforms

Ian Romanick idr at freedesktop.org
Wed Nov 21 21:17:08 UTC 2018 

On 11/21/2018 12:16 PM, Jose Fonseca wrote:
>>   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 . 

Right... I was more thinking about the str* functions than the *printf
functions.  Locale problems with scanf / printf are real on every
platform.  We had issues with this in the GLSL parser due to "." vs ","
as the ones to tenths separator in floating point numbers.  Other
*printf portability problems are real too.  I don't think there's any
variation with strchr or strstr, though. :)

Specifically, I (and I guess Emil also) propose removing the wrappers for:

 - strchr
 - strstr
 - strcmp (does this have locale issues?)
 - strncmp (does this have locale issues?)
 - strncat

And leave the rest for at least the time being.

> 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
> <https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.microsoft.com%2Fen-us%2Fprevious-versions%2Fvisualstudio%2Fvisual-studio-2012%2Feywx8zcx%28v%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(-)
>> 
>

More information about the mesa-dev mailing list