-
Notifications
You must be signed in to change notification settings - Fork 262
Fix for strnlen #916
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix for strnlen #916
Conversation
What platform and compiler did strnlen not compile on? I think it has been part of Posix for many years. |
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. |
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. |
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?
"exclude the ... flag" and "set the GNU standard flag" seem to be conflicting statements. Please clarify. Thanks for investigating why it compiles under CMake.
I was unaware of this issue. If I've appeared hesitant it is because I was unaware of
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. |
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. |
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.
Thank you for the detective work and explanation why we haven't seen this issue before.
Where is this documented? I'm not doubting you, I just want to read it myself.
I wanted a check on the length of |
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.
Running gcc as follows
Generates a implicit declaration for strnlen (Atleast for me it does). Where as
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: The macros of note _POSIX_C_SOURCE, _GNU_SOURCE and _ISOC11_SOURCE |
Thank you for this @rGovers. |
Was having compiler errors with strnlen. Changing to using memchr fixed the compiler errors.