KhronosGroup / KTX-Software

KTX (Khronos Texture) Library and Tools
Other
834 stars 222 forks source link

Fix for strnlen #916

Closed rGovers closed 1 month ago

rGovers commented 1 month ago

Was having compiler errors with strnlen. Changing to using memchr fixed the compiler errors.

CLAassistant commented 1 month ago

CLA assistant check
All committers have signed the CLA.

MarkCallow commented 1 month ago

What platform and compiler did strnlen not compile on? I think it has been part of Posix for many years.

rGovers commented 1 month ago

Compiling on Arch Linux with gcc 14.1.1 and glibc 2.39. I did manage to get it to compile again by removing the C standard C Flag.

Should probably note that I am not using cmake for building and cmake version does compile. And had to change to C11 in non cmake version due to the use of string literals apparently before it broke with a system update.

rGovers commented 1 month ago

After further digging maybe on my end. Had a look at the verbose output for cmake and noticed it does not specify a C standard. After further digging in cmake docs see mentions that if the compiler default meets the standard it will exclude the standard flag. After looking at gcc it defaults to gnu17 on my system which would enable the use of strnlen as it set the GNU standard flag. So cmake and gcc interaction weirdness is allowing it to compile in cmake.

If this is intended can close.

MarkCallow commented 1 month ago

removing the C standard C Flag.

When you write "C" do you mean "c"? (Back when c++ first appeared it was suggested to refer to it as "C" and people got in the habit but when MS adopted C++ they were still stuck with an OS whose file system could not distinguish lower case and upper case and could not support '+' in file names so "cpp" became the norm.)

What standard c flag are you referring to?

After further digging in cmake docs see mentions that if the compiler default meets the standard it will exclude the standard flag. After looking at gcc it defaults to gnu17 on my system which would enable the use of strnlen as it set the GNU standard flag. So cmake and gcc interaction weirdness is allowing it to compile in cmake.

"exclude the ... flag" and "set the GNU standard flag" seem to be conflicting statements. Please clarify. Thanks for investigating why it compiles under CMake.

If this is intended can close.

I was unaware of this issue. If I've appeared hesitant it is because I was unaware of memchr and to hear it is apparently more standard than the strnlen I have been using for decades is a surprise.

And had to change to C11 in non cmake version due to the use of string literals apparently before it broke with a system update.

Which string literals? Perhaps we should just set the flag to use c11. We're already specifying c++11 or c++17 for those sources.

rGovers commented 1 month ago

I am not the best with explaining stuff so bear with me. This came out of left field for me as well as it just worked for a while and also got surprised that it requires GNU extensions to libc after digging around in the string.h header file.

For string literals I am referring to the unicode32(U) prefix for example in info.c:365 which is a c11 feature if memory serves in the C standard and I see in the CMakeLists.txt:534 file it targets c99 as the minimum.

As for the standards it would seem with CMakeLists.txt:534 if I am understanding CMakes docs correctly (CMake Doc at Requiring Language Standards) is that if the compilers default standard can meet the feature set of of the specified standard it does not pass through the standard and lets the compiler decide the standard. In the case of my system CMake is using the gnu17 standard instead of the c99 as that is the default gcc uses for C code and gnu17 contains all the features of c99. This is allowing it to compile with strnlen as it enables the GNU extensions of libc which strnlen is a part of(strnlen is disabled if compiling against for example c11 instead of gnu11 as it is a POSIX function).

The reason removing the standard(c11) worked for me is that gcc then started using gnu17(gccs default) which enabled the GNU extensions(which the c standard verisons eg. c11 and c17 do not) that strnlen is in.

As for the use of memchr can be replaced with normal strlen just used as you seemed to want it in the range of len. Can also be re-implemented however just saw it in the one place so just used the closest equivalent. Could also just explicitly enable GNU extensions with gcc when compiling against std instead of gnu.

MarkCallow commented 1 month ago

For string literals I am referring to the unicode32(U) prefix for example in info.c:365 which is a c11 feature if memory serves in the C standard and I see in the CMakeLists.txt:534 file it targets c99 as the minimum.

This seems reason enough to me to target c11. All metadata is UTF-8 and the KTX signature includes non-ASCII characters. Please include a change to c11 in this PR.

As for the standards it would seem with CMakeLists.txt:534 if I am understanding CMakes docs correctly (CMake Doc at Requiring Language Standards) ...

Thank you for the detective work and explanation why we haven't seen this issue before.

... strnlen is disabled if compiling against for example c11 instead of gnu11 as it is a POSIX function).

Where is this documented? I'm not doubting you, I just want to read it myself.

As for the use of memchr can be replaced with normal strlen just used as you seemed to want it in the range of len. Can also be re-implemented however just saw it in the one place so just used the closest equivalent. Could also just explicitly enable GNU extensions with gcc when compiling against std instead of gnu.

I wanted a check on the length of needle without changing the signature of strnstr so using the length of haystack seemed the best choice. If memchr is a standard c11 function lets stick with that.

rGovers commented 1 month ago

A bit hard to explain and poorly documented gotcha that I am still going through myself(At least without going through multiple pages of documentation). If you look at the man page for strnlen it states that _POSIX_C_SOURCE >= 200809L which running GCC with the C standard(in this case c11) does not set, but the GNU standard(in this case gnu11) does (Whereas something like strlen does not have any requirements). As it is a magic macro it is best demonstrated like so on a Linux machine.

#include <string.h>
#include <stdio.h>

int main(int a_argc, char* a_argv[])
{
    const char str[] = "test string";

    int len = strnlen(str, sizeof(str));
    printf("%d \n", len);
}

Running gcc as follows

gcc -std=c11 main.c

Generates a implicit declaration for strnlen (Atleast for me it does).

Where as

gcc -std=gnu11 main.c

Compiles and outputs 11 when run.

Not a problem in 99.99% of cases(Most platforms implement it anyway hence probably never been a problem) but non portable. Technically can be worked around by defining the macro(_GNU_SOURCE) to my understanding to allow GCC to use POSIX functions as well from string.h. The issue does not pop up in your case as CMake lets GCC use the GNU source instead of the C source by what I have seen CMake do when compiling.

If you want further reading: man Feature Test Macros gcc Sources gcc Standards

The macros of note _POSIX_C_SOURCE, _GNU_SOURCE and _ISOC11_SOURCE

MarkCallow commented 1 month ago

Thank you for this @rGovers.