Enable and fix additional build warnings #655

NWilson · 2025-01-07T11:27:25Z

Part of #651

Use much stricter windows warnings (/W3 rather than /W1). This requires quite a few fixes for all the sloppy places where we do implicit assignment of 64-bit values to 32-bit storage.
Use and test CMake build & install on FreeBSD and Solaris
Add 64 bit Solaris build (cc -m64) and fix existing Solaris warnings
Make compile flags used in CI consistent across platforms. Previously Mac & Linux were building with different warning flags.
Add --enable-Werror to configure.ac. This means that you can build with -Werror in a clean way. Previously, you had to hackily override the CPPFLAGS when calling make since you can't pass -Werror as a CFLAG into ./configure (it messes with compiler feature detection).

src/pcre2_jit_char_inc.h

src/pcre2_jit_neon_inc.h

NWilson · 2025-01-07T13:30:30Z

src/pcre2grep.c

+    if (op->one_char > 0) snprintf(s, sizeof(s), "-%c,", op->one_char);
+      else snprintf(s, sizeof(s), "   ");


I have a fear of sprintf, strcpy and strcat. Perhaps we should ban them. All the current uses within the code seem harmless though - but they can go so badly wrong.

maint/manifest-cmakeinstall-freebsd

src/pcre2_jit_compile.c

zherczeg · 2025-01-08T12:55:53Z

src/pcre2_jit_compile.c

@@ -3380,7 +3381,7 @@ OP2(SLJIT_SUB | SLJIT_SET_Z, COUNT_MATCH, 0, COUNT_MATCH, 0, SLJIT_IMM, 1);
 add_jump(compiler, &common->calllimit, JUMP(SLJIT_ZERO));
 }

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


Size is a small number. Does it need to eliminate warnings?

Things like strlen, or subtraction of pointers, return a size_t, which is then passed to allocate_stack(). So I assumed that it could indeed be passed a size_t input, and that the various operations below would be able to handle it. But if it's a stack allocation, I guess that's not correct anyway, and the caller does need to sure it's only passing in "small" values.

I'll change it back, and update the callers not to cast away any large values.

@zherczeg What's the maximum amount you can pass to allocate_stack? There's one caller which passes in the callout_arg_size, which is 2 * sizeof(void*) * top_bracket. Because you can have 65536 capture groups, that's an entire 1MiB of data plus some extra few bytes.

Is the stack able to support enough for that?

It looks like I mixed this function up with sljit_alloc_memory. That is designed for small allocations.

This function should not be limited, although with very large numbers we could do a carry flag check.

src/pcre2_jit_neon_inc.h

NWilson · 2025-01-09T13:50:01Z

src/pcre2_jit_compile.c

@@ -1838,7 +1838,8 @@ if (*next == OP_BRAZERO || *next == OP_BRAMINZERO)

      if (i == max)
        {
-        common->private_data_ptrs[max_end - common->start - LINK_SIZE] = next_end - max_end;
+        /* Patterns must fit into an int32 even for link-size=4. */


I think this comment I have just added is correct.

I was worried about whether there's any case where a pattern can by 3GiB big (ie, bigger than an S32 can hold, but smaller than the U32 maximum for a pattern with link-size=4).

I can see there's some nice code elsewhere:

#define MAX_PATTERN_SIZE (1 << 30) /* Keep it positive */

("Keep it positive" clearly means, "don't use 1<<32" as the max size, but make sure it's comfortably smaller than that.)

This apparently ensures that you can't get in this situation, where you have a pattern whose compiled size is too large to fit in an int.

NWilson · 2025-01-09T13:52:03Z

src/pcre2_jit_compile.c

@@ -2460,7 +2461,7 @@ status->compiler = common->compiler;
 }

 static void delayed_mem_copy_move(delayed_mem_copy_status *status, int load_base, sljit_sw load_offset,
-  int store_base, sljit_sw store_offset)
+  int store_base, int store_offset)


The store_offset arg is placed in the status->store_offsets array, which is already an int in the current code. So I think it's safe to make the argument an int (and hence avoid truncation when assigning to status->store_offsets).

That just moves the problem elsewhere though: now we need to worry about truncating to a 32-bit int in all the code which calls delayed_mem_copy_move.

NWilson · 2025-01-09T13:57:28Z

src/pcre2_jit_compile.c

@@ -3184,7 +3185,7 @@ while (cc < ccend)
      SLJIT_ASSERT(private_srcw[i] != 0);

      if (!from_sp)
-        delayed_mem_copy_move(&status, base_reg, stackptr, SLJIT_SP, private_srcw[i]);
+        delayed_mem_copy_move(&status, base_reg, stackptr, SLJIT_SP, (int)private_srcw[i]); // XXX @zherczeg: cast OK?


@zherczeg I have added three casts here. I cannot verify whether these are OK. They are all truncation of an sljit_sw to an int.

It appears that all of the data placed inside kept_shared_srcw is within the range of an int; but private_srcw and shared_srcw are harder to check.

I'm hoping you "just know" already that these assignments are safe.

sljit_s32 could be better than int. The casts are ok, this should copy between (machine) stack and (backtracking) stack :)

Thank you very much for helping me out!

I have changed some uses of int to sljit_s32.

I think the changes to pcre2_jit_compile in this PR are now simply formatting - all the variables have the same type as before (since int and sljit_s32 are actually the same), and we've just added explicit casts for things that were previously implicit casts. So the runtime code should be identical.

I have changed some uses of int to sljit_s32.

historically (in the long obsolete PCRE1 codebase) all the interfaces used int and some might had not been updated since, so is usually safe to make this "conversion", but should also probably look for variables that were really meant to become PCRE2_SIZE, or in the case of JIT one of sljit_sw/sljit_uw.

NWilson mentioned this pull request Jan 7, 2025

Add some real EBCDIC testing #656

Draft

NWilson force-pushed the user/niwilson/more-warnings branch from c677841 to 989f5ae Compare January 7, 2025 12:44

NWilson commented Jan 7, 2025

View reviewed changes

src/pcre2_jit_char_inc.h Show resolved Hide resolved

NWilson commented Jan 7, 2025

View reviewed changes

src/pcre2_jit_neon_inc.h Show resolved Hide resolved

NWilson commented Jan 7, 2025

View reviewed changes

First attempts at adding more checking to Solaris and FreeBSD

327bd0c

NWilson force-pushed the user/niwilson/more-warnings branch from 989f5ae to 327bd0c Compare January 7, 2025 17:41

zherczeg reviewed Jan 8, 2025

View reviewed changes

Revert some size changes and simply add casts

9d1d2e8

NWilson commented Jan 9, 2025

View reviewed changes

Hopefully final round of tweaks

bf24f80

NWilson force-pushed the user/niwilson/more-warnings branch from 9a2231f to bf24f80 Compare January 9, 2025 20:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable and fix additional build warnings #655

Enable and fix additional build warnings #655

NWilson commented Jan 7, 2025

NWilson Jan 7, 2025

zherczeg Jan 8, 2025

NWilson Jan 8, 2025

NWilson Jan 9, 2025

zherczeg Jan 9, 2025

NWilson Jan 9, 2025

NWilson Jan 9, 2025

NWilson Jan 9, 2025

zherczeg Jan 9, 2025

NWilson Jan 9, 2025

carenas Jan 9, 2025

		if (op->one_char > 0) snprintf(s, sizeof(s), "-%c,", op->one_char);
		else snprintf(s, sizeof(s), " ");

Enable and fix additional build warnings #655

Are you sure you want to change the base?

Enable and fix additional build warnings #655

Conversation

NWilson commented Jan 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment