add a C23 inspired checked integer multiplication helper #198
Conversation
|
Closes #196 |
carenas
force-pushed
the
overflow
branch
2 times, most recently
from
4c9945f to
368fc1b
Compare
|
I support this idea of making a common source for this check. However, as pcre2_ckd_smul() is an internal function, and not part of the external API, it should really be defined using the PRIV macro (see comments in pcre2_internal.h). |
|
I know, but it seems ackward, since unlike all other helper functions, this one doesn't require a different version per code unit size. Would it be better to get rid of the C file and put it directly in the header, that way it would be also inlined where needed and work almost like the original? |
|
The thing about the helper functions is that there may be environments in which they are not exported beyond the library. This means you cannot use them in pcre2test. That is one of the reasons why the PRIV stuff exists. |
|
Also, the use of the PRIV notation makes it clear that this is a library-wide helper function, and not something local. |
|
An alternative to the current proposal which doesn't use the Also included some changes raised in the review, but not sure if they are sufficient, either way let me know which way you would like to proceed. |
|
I prefer not to put code into .h files, and I'm happy with a separate function, but it must use the PRIV mechanism, for consistency if nothing else. I will do some work on this after I have dealt with #186. |
|
FWIW, both (this version with the code in the header) and the alternative use PRIV. There is also an additional branch with a patch on top that uses the compiler builtins if available (which works for recent gcc and clang) so the assert() might not be even compiled in most cases and that hopefully works also for Zoltan. |
carenas
force-pushed
the
overflow
branch
3 times, most recently
from
836a394 to
6d65dc6
Compare
Compilers had become far more creative about optimizing code that might be considered undefined behaviour, so improve our integer overflow checking to prevent any possible miscompilations. While at it, expand the use of the checks to pcre2test to fix a similar problem to the one that was tackled in pcre2_compile. Prefer (if available) a builtin provided by the compiler.
When support for Unity/Jumbo builds was added[1], the fact that cmake will need to be able to not mix files with different PCRE2_UCHAR sizes was missed, resulting in a possibly broken build by redefining LINK_SIZE as shown by warnings during compilation. Since 4678857 (add a C23 inspired checked integer multiplication helper (PCRE2Project#198), 2023-02-03), the build will fail if the linker wouldn't be able to merge the multiple implementations of pcre2_ckd_smul from each participating library. To avoid both problems, disable UNITY_BUILD for the non 8-bit libraries. [1] PCRE2Project#94
When support for Unity/Jumbo builds was added[1], the fact that cmake will need to be able to not mix files with different PCRE2_UCHAR sizes was missed, resulting in a possibly broken build by redefining LINK_SIZE as shown by warnings during compilation. Since 4678857 (add a C23 inspired checked integer multiplication helper (#198), 2023-02-03), the build will fail if the linker wouldn't be able to merge the multiple implementations of pcre2_ckd_smul from each participating library. To avoid both problems, disable UNITY_BUILD for the non 8-bit libraries. [1] #94
A combined approach that addresses #196 and replaces #177
Unlike the original It allows in both cases to handle larger sizes than INT_MAX when possible