Address most implicit 64-to-32-bit conversion warnings. #525

ucko · 2024-10-11T20:13:39Z

Visual Studio and Xcode both issue such warnings by default, and Clang also offers a -Wshorten-64-to-32 option to expose them elsewhere. To address them, harmonize types where practical and add casts as appropriate (first formally capping executables' read request sizes as necessary). NB: detect_repeat in pcre2_jit_compile.c still yields two such warnings that look plausibly legitimate but not so easy to fix. :-/

carenas

Presume from your commit message you were trying to remove warnings from the 8-bit library code when built in a 64bit system, could you elaborate on what is the driver behind that?

carenas · 2024-10-11T21:31:17Z

src/pcre2_chkdint.c


 BOOL
-PRIV(ckd_smul)(PCRE2_SIZE *r, int a, int b)
+PRIV(ckd_smul)(PCRE2_SIZE *r, ptrdiff_t a, ptrdiff_t b)


the whole point of this function is to make sure those 2 parameters are intso we can check for overflow

Ah, yeah, the hand-coded branch would need reworking. (__builtin_mul_overflow should DTRT regardless.) However, without this change, pcre2test's process_data could in principle pass in a truncated replen, yielding incorrect results. Should it instead bail if replen > INT_MAX?

pcre2test is a test application, AFAIK replen was made a size_t just to avoid having to do a cast, and it is expected never to have a value larger than int either, so a truncation there shouldn't be possible, but feel free to poke at it and prove me wrong.

the call for ckd_smul was indeed how we are making sure that the value of the multiplication is always correct.

FWIW you could NOT build this test application if all you want is the library an want it to be warning free, there is a configure and cmake option for that. You haven't answered my original question so I am still not sure what the end goal is.

carenas · 2024-10-11T21:35:18Z

src/pcre2_jit_compile.c

 }

-static SLJIT_INLINE void allocate_stack(compiler_common *common, int size)
+static SLJIT_INLINE void allocate_stack(compiler_common *common, sljit_uw size)


probably should had been PCRE2_SIZE but were missed when PCRE2 was created from PCRE1 (which used int for all offsets)

PCRE2_SIZE substituted, thanks!

carenas · 2024-10-11T21:40:37Z

src/pcre2_jit_neon_inc.h

 __attribute__((no_sanitize_address))
 #endif
-static sljit_u8* SLJIT_FUNC FF_FUN(sljit_u8 *str_end, sljit_u8 **str_ptr, sljit_uw offs1, sljit_uw offs2, sljit_uw chars)
+static sljit_u8* SLJIT_FUNC FF_FUN(sljit_u8 *str_end, sljit_u8 **str_ptr, sljit_uw offs1, sljit_uw offs2, sljit_u32 chars)


this should break the 16bit library

Not AFAICT, though I must admit I'd initially focused on the 8-bit one.

zherczeg

Please be very careful when changing jit types.

zherczeg · 2024-10-12T09:22:55Z

src/pcre2_jit_compile.c

  struct sljit_compiler *compiler;
  int store_bases[RECURSE_TMP_REG_COUNT];
-  int store_offsets[RECURSE_TMP_REG_COUNT];
+  sljit_sw store_offsets[RECURSE_TMP_REG_COUNT];


These are intentionally small values.

Without this change, delayed_mem_copy_move's assignment into store_offsets can wind up truncated. Attempting to narrow the type of its store_offset argument instead reveals that three call sites pass in 64-bit values.

zherczeg · 2024-10-12T09:23:34Z

src/pcre2_jit_compile.c

+/* These values should never be negative, suggesting the use of sljit_u32.
+However, they should also remain (far) below 1 << 31, and (would-be)
+negative *differences* won't otherwise undergo correct sign extension. */
+sljit_s32 c, charoffset, max = 256, min = READ_CHAR_MAX;


c can be UINT32_MAX, so this is also a bad change.

Good point. I've scrapped all changes to compile_xclass_matchingpath in favor of formally making read_char's min and max sljit_uw.

Visual Studio and Xcode both issue such warnings by default, and Clang also offers a -Wshorten-64-to-32 option to expose them elsewhere. To address them, harmonize types where practical and add casts as appropriate (first formally capping executables' read request sizes as necessary). NB: detect_repeat in pcre2_jit_compile.c still yields two such warnings that look plausibly legitimate but not so easy to fix. :-/ Signed-off-by: Aaron M. Ucko <[email protected]>

zherczeg · 2024-10-15T14:59:50Z

src/pcre2.h.generic

  pcre2_set_depth_limit(pcre2_match_context *, uint32_t); \
 PCRE2_EXP_DECL int PCRE2_CALL_CONVENTION \
-  pcre2_set_heap_limit(pcre2_match_context *, uint32_t); \
+  pcre2_set_heap_limit(pcre2_match_context *, PCRE2_SIZE); \


API functions cannot be changed, even if they are not the best.

Oops, yeah, I didn't mean to introduce an ABI break. :-/

Ah, right, I'll need to revert the POSIX API changes too.

ucko · 2024-10-15T15:36:03Z

Thanks for the quick feedback! I've force-pushed a largely corrected (and rebased) version and will reply on individual points shortly. AFAICT, some of these changes really should help in corner cases; although the remainder are formalities, the (configuration-dependent) warnings they address could bury other warnings (none here at present, I'm happy to report).

I acknowledge that this PR still needs more work.

NWilson · 2025-01-12T11:11:06Z

Thank you Aaron (@ucko) for pushing us to fix these warnings.

I have addressed them in PR #655, hopefully. I took a slightly different direction to you on some of the fixes. We now pass with /W3 warnings on Windows (MSVC).

I have not addressed all the warnings generated by -Wshorten-64-to-32 (GCC, Clang) since that is far spammier than MSVC's equivalent.

Let me know if you are satisfied with the level of warnings suppression on master. I would be happy to fix any additional warnings that are causing you problems with your company's build policy, if you can point out where they are.

ucko · 2025-01-15T15:59:50Z

@NWilson Great, thank you very much! Your fixes are a big improvement, but I'd appreciate it if you could please take another look at pcre2_jit_neon_inc.h. I also still see a handful of warnings from pcre2grep.c and pcre2test.c, but they're less of a concern. I'll elaborate on your #655.

NWilson · 2025-02-10T10:16:11Z

Thank you for this PR. I think we have now incorporated all the changes here. Many thanks!

carenas reviewed Oct 11, 2024

View reviewed changes

zherczeg requested changes Oct 12, 2024

View reviewed changes

ucko force-pushed the master branch from 53f2269 to 59a946b Compare October 15, 2024 14:55

zherczeg reviewed Oct 15, 2024

View reviewed changes

ucko mentioned this pull request Jan 15, 2025

Enable and fix additional build warnings #655

Merged

NWilson closed this Feb 10, 2025

Address most implicit 64-to-32-bit conversion warnings. #525

Address most implicit 64-to-32-bit conversion warnings. #525

Uh oh!

Conversation

ucko commented Oct 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carenas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carenas Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ucko Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ucko commented Oct 15, 2024

Uh oh!

NWilson commented Jan 12, 2025

Uh oh!

ucko commented Jan 15, 2025

Uh oh!

NWilson commented Feb 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ucko commented Oct 11, 2024 •

edited

Loading

carenas left a comment •

edited

Loading

carenas Oct 17, 2024 •

edited

Loading

ucko Oct 15, 2024 •

edited

Loading